Re: [PATCH v2 12/14] fs: xfs: Support atomic write for statx

[John Garry <john.g.garry@oracle.com>]

On 06/03/2024 21:31, Dave Chinner wrote:
>> +	xfs_extlen_t		extsz = xfs_get_extsz(ip);
>> +	struct xfs_buftarg	*target = xfs_inode_buftarg(ip);
>> +	struct block_device	*bdev = target->bt_bdev;
>> +	struct request_queue	*q = bdev->bd_queue;
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +	struct xfs_sb		*sbp = &mp->m_sb;
>> +	unsigned int		awu_min, awu_max;
>> +	unsigned int		extsz_bytes = XFS_FSB_TO_B(mp, extsz);
>> +
>> +	awu_min = queue_atomic_write_unit_min_bytes(q);
>> +	awu_max = queue_atomic_write_unit_max_bytes(q);
> We really should be storing these in the xfs_buftarg at mount time,
> like we do logical and physical sector sizes. 

This has been mentioned previously, and Darrick thought that it was not 
safe. Please see first response in:
https://lore.kernel.org/linux-xfs/20231003161029.GG21298@frogsfrogsfrogs/#t

So if this really is true, then I'll stick with something like what I 
have here and add a comment on that.

However, in this series the block layer does check for out-of-range 
atomic write BIOs in 1/14. So we could store the values in xfs_buftarg, 
as you suggest for the lookup here. If the bdev geometry does really 
change beneath us, worse thing that happens is that we may report 
incorrect values for statx.

> Similar to sector
> sizes, they*must not change*  once the filesystem has been created
> on the device, let alone during an active mount. The whole point of
> the xfs_buftarg is to store the information the filesystem
> needs to do IO to the underlying block device so we don't have to
> chase pointers deep into the block device whenever we need to use
> static geometry information.....
> 
>> +	if (sbp->sb_blocksize > awu_max || awu_min > sbp->sb_blocksize ||
>> +	    !xfs_inode_atomicwrites(ip)) {
>> +		*unit_min = 0;
>> +		*unit_max = 0;
>> +		return;
>> +	}
> Again, this is comparing static geometry - if the block size doesn't
> allow atomic writes, then the inode flag should never be set. i.e.
> geometry is checked when configuring atomic writes, not in every
> place we need to check if atomic writes are supported. Hence this
> should simply be:
> 
> 	if (!xfs_inode_has_atomic_writes(ip)) {
> 		*unit_min = 0;
> 		*unit_max = 0;
> 		return;
> 	} >
> before we even look at the xfs_buftarg to get the supported min/max
> values for the given device.

Thanks,
John