Re: [PATCH v2 11/14] fs: xfs: Support FS_XFLAG_ATOMICWRITES for forcealign

John Garry
2024-03-07T12:43:18Z

>> #define XFS_SB_FEAT_RO_COMPAT_ALL \
>> (XFS_SB_FEAT_RO_COMPAT_FINOBT | \
>> XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
>> XFS_SB_FEAT_RO_COMPAT_REFLINK| \
>> - XFS_SB_FEAT_RO_COMPAT_INOBTCNT | \
>> - XFS_SB_FEAT_RO_COMPAT_FORCEALIGN)
>> + XFS_SB_FEAT_RO_COMPAT_INOBTCNT| \
>> + XFS_SB_FEAT_RO_COMPAT_FORCEALIGN| \
>
> Please leave a spave between the feature name and the '| \'.
>

ok

>> + XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES)
>> +

...

>> }
>>
>> +static inline bool xfs_inode_atomicwrites(struct xfs_inode *ip)
>> +{
>> + return ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES;
>> +}
>
> I'd really like this to be more readable:
> xfs_inode_has_atomic_writes().
>
> Same for the force align check, now that I notice it:
> xfs_inode_has_force_align().

ok, will change

>
>> +
>> /*
>> * Return the buftarg used for data allocations on a given inode.
>> */
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 867d8d51a3d0..f118a1ae39b5 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -1112,6 +1112,8 @@ xfs_flags2diflags2(
>> di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>> if (xflags & FS_XFLAG_FORCEALIGN)
>> di_flags2 |= XFS_DIFLAG2_FORCEALIGN;
>> + if (xflags & FS_XFLAG_ATOMICWRITES)
>> + di_flags2 |= XFS_DIFLAG2_ATOMICWRITES;
>>
>> return di_flags2;
>> }
>> @@ -1124,10 +1126,12 @@ xfs_ioctl_setattr_xflags(
>> {
>> struct xfs_mount *mp = ip->i_mount;
>> bool rtflag = (fa->fsx_xflags & FS_XFLAG_REALTIME);
>> + bool atomic_writes = fa->fsx_xflags & FS_XFLAG_ATOMICWRITES;
>> uint64_t i_flags2;
>>
>> - if (rtflag != XFS_IS_REALTIME_INODE(ip)) {
>> - /* Can't change realtime flag if any extents are allocated. */
>> + /* Can't change RT or atomic flags if any extents are allocated. */
>> + if (rtflag != XFS_IS_REALTIME_INODE(ip) ||
>> + atomic_writes != xfs_inode_atomicwrites(ip)) {
>> if (ip->i_df.if_nextents || ip->i_delayed_blks)
>> return -EINVAL;
>> }
>> @@ -1164,6 +1168,13 @@ xfs_ioctl_setattr_xflags(
>> return -EINVAL;
>> }
>>
>> + if (atomic_writes) {
>> + if (!xfs_has_atomicwrites(mp))
>> + return -EINVAL;
>
> That looks wrong - if we are trying to turn on atomic writes, then
> shouldn't this be returning an error if atomic writes are already
> configured?

I think that you are talking about a xfs_inode_atomicwrites() check.

>
>> + if (!(fa->fsx_xflags & FS_XFLAG_FORCEALIGN))
>> + return -EINVAL;

>
> Where's the check for xfs_has_atomicwrites(mp) here?

please see above

> We can't allow
> this inode flag to be set if the superblock does not have the
> feature bit that says it's a known feature bit set.
>
> Which reminds me: both the forcealign and the atomicwrite inode flag
> need explicit checking in the inode verifier. i.e. checking that if
> the inode flag bit is set, the relevant superblock feature bit is
> set.

We do have that in the xfs_has_atomicwrites() and xfs_has_forcealign()
checks - is that ok?

>
> ....
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index 74dcafddf6a9..efe4b4234b2e 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1712,6 +1712,10 @@ xfs_fs_fill_super(
>> xfs_warn(mp,
>> "EXPERIMENTAL forced data extent alignment feature in use. Use at your own risk!");
>>
>> + if (xfs_has_atomicwrites(mp))
>> + xfs_warn(mp,
>> +"EXPERIMENTAL atomicwrites feature in use. Use at your own risk!");
>
> "EXPERIMENTAL atomic write IO feature is in use. Use at your own risk!");
>

ok

Thanks,
John