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

John Garry
2024-03-07T10:35:38Z

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