Re: [PATCH v3 3/6] xfs: add per-inode AG prediction map and dirty-AG bitmap

["Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com>]

On 2/6/26 11:27, Darrick J. Wong wrote:
> On Fri, Feb 06, 2026 at 11:06:03AM +0530, Nirjhar Roy (IBM) wrote:
>> On Thu, 2026-02-05 at 08:36 -0800, Darrick J. Wong wrote:
>>> On Thu, Feb 05, 2026 at 12:06:19PM +0530, Nirjhar Roy (IBM) wrote:
>>>> On Fri, 2026-01-16 at 15:38 +0530, Kundan Kumar wrote:
>>>>> Add per-inode structures to track predicted AGs of dirty folios using
>>>>> an xarray and bitmap. This enables efficient identification of AGs
>>>>> involved in writeback.
>>>>>
>>>>> Signed-off-by: Kundan Kumar <kundan.kumar@samsung.com>
>>>>> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
>>>>> ---
>>>>>   fs/xfs/xfs_icache.c | 27 +++++++++++++++++++++++++++
>>>>>   fs/xfs/xfs_inode.h  |  5 +++++
>>>>>   2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
>>>>> index e44040206851..f97aa6d66271 100644
>>>>> --- a/fs/xfs/xfs_icache.c
>>>>> +++ b/fs/xfs/xfs_icache.c
>>>>> @@ -80,6 +80,25 @@ static inline xa_mark_t ici_tag_to_mark(unsigned int tag)
>>>>>   	return XFS_PERAG_BLOCKGC_MARK;
>>>>>   }
>>>>>   
>>>>> +static int xfs_inode_init_ag_bitmap(struct xfs_inode *ip)
>>>> Similar comment as before:
>>>> static int
>>>> xfs_inode_init...()
>>>>> +{
>>>>> +	unsigned int bits = ip->i_mount->m_sb.sb_agcount;
>>>>> +	unsigned int nlongs;
>>>>> +
>>>>> +	xa_init_flags(&ip->i_ag_pmap, XA_FLAGS_LOCK_IRQ);
>>>> Nit: The name of the functions suggests that it is initializing the tracking bitmap which it does -
>>>> however, the above line does slightly different thing? Maybe move the xarray init outside the bitmap
>>>> init function?
>>> Or just call it something else?  xfs_inode_init_perag_wb?
>>>
>>>>> +	ip->i_ag_dirty_bitmap = NULL;
>>>>> +	ip->i_ag_dirty_bits = bits;
>>>>> +
>>>>> +	if (!bits)
>>>> Umm, !bits means agcount is 0. Shouldn't we ASSERT that bits >= 2? Or am I missing something?
>>> Technically you can have 1 AG, but you definitely can't mount a zero AG
>>> filesystem.
>> Okay, but:
>> /home/ubuntu$ mkfs.xfs -f  -d agcount=1 /dev/loop0
>> Filesystem must have at least 2 superblocks for redundancy!
>> Usage: mkfs.xfs
>> Or maybe this restriction is just at the userspace tool level?
> Yeah.  If the only super dies then the filesystem is completely
> unrecoverable, which is why you have to really fight mkfs to spit out
> single-AG filesystems.

Okay. But I think there are certain places in the kernel code where 1 AG 
is treated as an -EINVAL. For instance [1]

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/xfs/xfs_fsops.c#n140

--NR

>
> --D
>
>>>>> +		return 0;
>>>>> +
>>>>> +	nlongs = BITS_TO_LONGS(bits);
>>>>> +	ip->i_ag_dirty_bitmap = kcalloc(nlongs, sizeof(unsigned long),
>>>>> +					GFP_NOFS);
>>>>> +
>>>>> +	return ip->i_ag_dirty_bitmap ? 0 : -ENOMEM;
>>>>> +}
>>>>> +
>>>>>   /*
>>>>>    * Allocate and initialise an xfs_inode.
>>>>>    */
>>>>> @@ -131,6 +150,8 @@ xfs_inode_alloc(
>>>>>   	ip->i_next_unlinked = NULLAGINO;
>>>>>   	ip->i_prev_unlinked = 0;
>>>>>   
>>>>> +	xfs_inode_init_ag_bitmap(ip);
>>>> xfs_inode_init_ag_bitmap() returns int - error handling for -ENOMEM?
>>>>> +
>>>>>   	return ip;
>>>>>   }
>>>>>   
>>>>> @@ -194,6 +215,12 @@ xfs_inode_free(
>>>>>   	ip->i_ino = 0;
>>>>>   	spin_unlock(&ip->i_flags_lock);
>>>>>   
>>>>> +	/* free xarray contents (values are immediate packed ints) */
>>>>> +	xa_destroy(&ip->i_ag_pmap);
>>>> Nit:Maybe have a small wrapper for freeing it the prediction map? No hard preferences though.
>>>>> +	kfree(ip->i_ag_dirty_bitmap);
>>>>> +	ip->i_ag_dirty_bitmap = NULL;
>>>> Nit: Usually while freeing the pointers I prefer:
>>>> t = ip->i_ag_dirty_bitmap;
>>>> ip->i_ag_dirty_bitmap = NULL;
>>>> kfree(t);
>>>> In this way, the pointer(i_ag_dirty_bitmap in this case) that I am freeing never points to an
>>>> already freed address.
>>>>
>>>>> +	ip->i_ag_dirty_bits = 0;
>>>>> +
>>>>>   	__xfs_inode_free(ip);
>>>>>   }
>>>>>   
>>>>> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
>>>>> index bd6d33557194..dee449168605 100644
>>>>> --- a/fs/xfs/xfs_inode.h
>>>>> +++ b/fs/xfs/xfs_inode.h
>>>>> @@ -99,6 +99,11 @@ typedef struct xfs_inode {
>>>>>   	spinlock_t		i_ioend_lock;
>>>>>   	struct work_struct	i_ioend_work;
>>>>>   	struct list_head	i_ioend_list;
>>>>> +
>>>>> +	/* AG prediction map: pgoff_t -> packed u32 */
>>>>> +	struct xarray           i_ag_pmap;
>>>>> +	unsigned long           *i_ag_dirty_bitmap;
>>>>> +	unsigned int            i_ag_dirty_bits;
>>>> Not sure but, I mostly see the typedefed versions of data types being used like uint32 etc. Darrick,
>>>> hch, are the above fine?
>>> Yes, please don't mix types.  Pick one type and stick with it.
>>>
>>> (and yes I wish we could struct bitmap_t(unsigned long))
>>>
>>> --D
>>>
>>>> --NR
>>>>>   } xfs_inode_t;
>>>>>   
>>>>>   static inline bool xfs_inode_on_unlinked_list(const struct xfs_inode *ip)
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore