Re: [PATCH] xfs: don't limit software atomic writes by the group alignment

[John Garry <john.garry@linux.dev>]

On 10/5/26 17:24, Pankaj Raghav (Samsung) wrote:
>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>>> index d1306e723899..8c8b14f94ede 100644
>>> --- a/fs/xfs/xfs_iops.c
>>> +++ b/fs/xfs/xfs_iops.c
>>> @@ -620,6 +620,12 @@ xfs_get_atomic_write_min(
>>>    	return 0;
>>>    }
>>> +static inline enum xfs_group_type
>>> +xfs_inode_group_type(struct xfs_inode *ip)
>>> +{
>>> +	return XFS_IS_REALTIME_INODE(ip) ? XG_TYPE_RTG : XG_TYPE_AG;
>>> +}
>>
>> would this be better is a common location (so that it could be reused)?
>>
> 
> Probably to xfs_mount.h?
> 

I'm not sure. Darrick may be able to give a good suggestion.

>>> +
>>>    unsigned int
>>>    xfs_get_atomic_write_max(
>>>    	struct xfs_inode	*ip)
>>> @@ -642,19 +648,20 @@ xfs_get_atomic_write_max(
>>>    	 * then advertise a maximum size of whatever we can complete through
>>>    	 * that means.  Hardware support is reported via max_opt, not here.
>>>    	 */
>>> -	if (XFS_IS_REALTIME_INODE(ip))
>>> -		return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_RTG].awu_max);
>>> -	return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_AG].awu_max);
>>> +	return XFS_FSB_TO_B(mp, mp->m_groups[xfs_inode_group_type(ip)].awu_max);
>>>    }
>>>    unsigned int
>>>    xfs_get_atomic_write_max_opt(
>>>    	struct xfs_inode	*ip)
>>>    {
>>> +	struct xfs_mount	*mp = ip->i_mount;
>>>    	unsigned int		awu_max = xfs_get_atomic_write_max(ip);
>>
>> xfs_get_atomic_write_max() value is calculated based on HW atomic support. I
>> am wondering if we should add a function to just give the max CoW-based
>> atomic, and have it called here and from xfs_get_atomic_write_max(). Not a
>> big deal, though.
> 
> Could you elaborate this comment?
> 
> I do remove any HW dependency in xfs_get_atomic_write_max() calculation
> as a part of this patch.

xfs_get_atomic_write_max() does still have a 
xfs_inode_can_hw_atomic_write() call.

It just seems a bit awkward that xfs_get_atomic_write_max_opt() calls 
xfs_get_atomic_write_max(), when it should be able to do the full 
calculation itself.

This is not a deal deal which I am mentioning.

> 
>>
>>> +	xfs_extlen_t		align_max_fsb;
>>> +	unsigned int		opt;
>>>    	/* if the max is 1x block, then just keep behaviour that opt is 0 */
>>> -	if (awu_max <= ip->i_mount->m_sb.sb_blocksize)
>>> +	if (awu_max <= mp->m_sb.sb_blocksize)
>>>    		return 0;
>>>    	/*
>>> @@ -663,7 +670,17 @@ xfs_get_atomic_write_max_opt(
>>>    	 * less than our out of place write limit, but we don't want to exceed
>>>    	 * the awu_max.
>>>    	 */
>>> -	return min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
>>> +	opt = min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
>>> +
>>> +	/*
>>> +	 * REQ_ATOMIC writes also have to be naturally aligned on disk, so we
>>> +	 * cannot promise more than the largest extent that the allocator is
>>> +	 * able to align within a group.
>>> +	 */
>>> +	align_max_fsb = xfs_calc_group_awu_align_max(mp,
>>> +						xfs_inode_group_type(ip));
>>> +
>>> +	return min_t(xfs_fsize_t, opt, XFS_FSB_TO_B(mp, align_max_fsb));
>>
>> unsigned int? But is there a possibility that the value in XFS_FSB_TO_B(mp,
>> align_max_fsb) can exceed an unsigned int?
> 
> We are limited by `opt` length which is an unsigned int. I do use
> min_t(xfs_fsize_t, ..) for calculation to avoid any truncation error.
> 

ok, fine

BTW, maybe call the variable max_opt, and not just opt.