Re: [PATCH 6/6] fs: xfs: Set FMODE_CAN_ATOMIC_WRITE for FS_XFLAG_ATOMICWRITES set

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

On 21/02/2024 17:00, Darrick J. Wong wrote:
>>> Hmm.  Well, if we move towards pushing all the hardware checks out of
>>> xfs/iomap and into whatever goes on underneath submit_bio then I guess
>>> we don't need to check device support here at all.
>> Yeah, I have been thinking about this. But I was still planning on putting a
>> "bdev on atomic write" check here, as you mentioned.
>>
>> But is this a proper method to access the bdev for an xfs inode:
>>
>> STATIC bool
>> xfs_file_can_atomic_write(
>> struct xfs_inode *inode)
>> {
>> 	struct xfs_buftarg *target = xfs_inode_buftarg(inode);
>> 	struct block_device *bdev = target->bt_bdev;
>>
>> 	if (!xfs_inode_atomicwrites(inode))
>> 		return false;
>>
>> 	return bdev_can_atomic_write(bdev);
>> }
> There's still a TOCTOU race problem if the bdev gets reconfigured
> between xfs_file_can_atomic_write and submit_bio.

If that is the case then a check in the bio submit path is required to 
catch any such reconfigure problems - and we effectively have that in 
this series.

I am looking at change some of these XFS bdev_can_atomic_write() checks, 
but would still have a check in the bio submit path.

> 
> However, if you're only using this to advertise the capability via statx
> then I suppose that's fine -- userspace has to have some means of
> discovering the ability at all.  Userspace is also inherently racy.
> 
>> I do notice the dax check in xfs_bmbt_to_iomap() when assigning iomap->bdev,
>> which is creating some doubt?
> Do you mean this?
> 
> 	if (mapping_flags & IOMAP_DAX)
> 		iomap->dax_dev = target->bt_daxdev;
> 	else
> 		iomap->bdev = target->bt_bdev;
> 
> The dax path wants dax_dev set so that it can do the glorified memcpy
> operation, and it doesn't need (or want) a block device.

Yes, so proper to use target->bt_bdev for checks for bdev atomic write 
capability, right?

Thanks,
John