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

From: John Garry
Date: Thu Mar 07 2024 - 07:43:40 EST



#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