[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6dbb5eac-1e3a-4d2d-8e74-38f6aac1b06b@oracle.com>
Date: Thu, 7 Mar 2024 12:42:58 +0000
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>
Cc: djwong@...nel.org, hch@....de, viro@...iv.linux.org.uk, brauner@...nel.org,
jack@...e.cz, chandan.babu@...cle.com, axboe@...nel.dk,
martin.petersen@...cle.com, linux-kernel@...r.kernel.org,
linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
tytso@....edu, jbongio@...gle.com, ojaswin@...ux.ibm.com,
ritesh.list@...il.com, linux-block@...r.kernel.org
Subject: Re: [PATCH v2 11/14] fs: xfs: Support FS_XFLAG_ATOMICWRITES for
forcealign
>> #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
Powered by blists - more mailing lists