Re: [PATCH v6 4/7] xfs: Support FS_XFLAG_ATOMICWRITES

From: John Garry
Date: Tue Oct 01 2024 - 09:36:25 EST


}
+static xfs_failaddr_t
+xfs_inode_validate_atomicwrites(
+ struct xfs_mount *mp,
+ uint32_t cowextsize,
+ uint16_t mode,
+ int64_t flags2)
+{
+ /* superblock rocompat feature flag */
+ if (!xfs_has_atomicwrites(mp))
+ return __this_address;
+
+ /* Only regular files and directories */
+ if (!S_ISREG(mode) && !(S_ISDIR(mode)))
+ return __this_address;
+
+ /* COW extsize disallowed */
+ if (flags2 & XFS_DIFLAG2_COWEXTSIZE)
+ return __this_address;
+
+ /* cowextsize must be zero */
+ if (cowextsize)
+ return __this_address;
+
+ /* reflink is disallowed */
+ if (flags2 & XFS_DIFLAG2_REFLINK)
+ return __this_address;

If we're only allowing atomic writes that are 1 fsblock or less, then
copy on write will work correctly because CoWs are always done with
fsblock granularity. The ioend remap is also committed atomically.

IOWs, it's forcealign that isn't compatible with reflink and you can
drop this incompatibility.

ok, understood


+
+ return NULL;
+}
+
xfs_failaddr_t
xfs_dinode_verify(
struct xfs_mount *mp,
@@ -663,6 +693,14 @@ xfs_dinode_verify(
!xfs_has_bigtime(mp))
return __this_address;
+ if (flags2 & XFS_DIFLAG2_ATOMICWRITES) {
+ fa = xfs_inode_validate_atomicwrites(mp,
+ be32_to_cpu(dip->di_cowextsize),

Technically speaking, the space used by di_cowextsize isn't defined on
!reflink filesystems. The contents are supposed to be zero, but nobody
actually checks that, so you might want to special case this:

fa = xfs_inode_validate_atomicwrites(mp,
xfs_has_reflink(mp) ?
be32_to_cpu(dip->di_cowextsize) : 0,
mode, flags2);

(inasmuch as this code is getting ugly and maybe you want to use a
temporary variable)

As discussed later, I will drop the cowextsize check (so need to pass this at all)


+ mode, flags2);
+ if (fa)
+ return fa;
+ }
+
return NULL;
}
diff --git a/fs/xfs/libxfs/xfs_inode_util.c b/fs/xfs/libxfs/xfs_inode_util.c
index cc38e1c3c3e1..e59e98783bf7 100644
--- a/fs/xfs/libxfs/xfs_inode_util.c
+++ b/fs/xfs/libxfs/xfs_inode_util.c
@@ -80,6 +80,8 @@ xfs_flags2diflags2(
di_flags2 |= XFS_DIFLAG2_DAX;
if (xflags & FS_XFLAG_COWEXTSIZE)
di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
+ if (xflags & FS_XFLAG_ATOMICWRITES)
+ di_flags2 |= XFS_DIFLAG2_ATOMICWRITES;
return di_flags2;
}
@@ -126,6 +128,8 @@ xfs_ip2xflags(
flags |= FS_XFLAG_DAX;
if (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE)
flags |= FS_XFLAG_COWEXTSIZE;
+ if (ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES)
+ flags |= FS_XFLAG_ATOMICWRITES;
}
if (xfs_inode_has_attr_fork(ip))
@@ -224,6 +228,8 @@ xfs_inode_inherit_flags2(
}
if (pip->i_diflags2 & XFS_DIFLAG2_DAX)
ip->i_diflags2 |= XFS_DIFLAG2_DAX;
+ if (pip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES)
+ ip->i_diflags2 |= XFS_DIFLAG2_ATOMICWRITES;
/* Don't let invalid cowextsize hints propagate. */
failaddr = xfs_inode_validate_cowextsize(ip->i_mount, ip->i_cowextsize,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index d95409f3cba6..dd819561d0a5 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -164,6 +164,8 @@ xfs_sb_version_to_features(
features |= XFS_FEAT_REFLINK;
if (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_INOBTCNT)
features |= XFS_FEAT_INOBTCNT;
+ if (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES)
+ features |= XFS_FEAT_ATOMICWRITES;
if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_FTYPE)
features |= XFS_FEAT_FTYPE;
if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_SPINODES)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index aa4dbda7b536..44bee3e2b2bb 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2060,6 +2060,8 @@ int
xfs_init_buftarg(
struct xfs_buftarg *btp,
size_t logical_sectorsize,
+ unsigned int awu_min,
+ unsigned int awu_max,
const char *descr)
{
/* Set up device logical sector size mask */
@@ -2086,6 +2088,9 @@ xfs_init_buftarg(
btp->bt_shrinker->scan_objects = xfs_buftarg_shrink_scan;
btp->bt_shrinker->private_data = btp;
shrinker_register(btp->bt_shrinker);
+
+ btp->bt_bdev_awu_min = awu_min;
+ btp->bt_bdev_awu_max = awu_max;
return 0;
out_destroy_io_count:
@@ -2102,6 +2107,7 @@ xfs_alloc_buftarg(
{
struct xfs_buftarg *btp;
const struct dax_holder_operations *ops = NULL;
+ unsigned int awu_min = 0, awu_max = 0;
#if defined(CONFIG_FS_DAX) && defined(CONFIG_MEMORY_FAILURE)
ops = &xfs_dax_holder_operations;
@@ -2115,6 +2121,13 @@ xfs_alloc_buftarg(
btp->bt_daxdev = fs_dax_get_by_bdev(btp->bt_bdev, &btp->bt_dax_part_off,
mp, ops);
+ if (bdev_can_atomic_write(btp->bt_bdev)) {
+ struct request_queue *q = bdev_get_queue(btp->bt_bdev);
+
+ awu_min = queue_atomic_write_unit_min_bytes(q);
+ awu_max = queue_atomic_write_unit_max_bytes(q);
+ }
+
/*
* When allocating the buftargs we have not yet read the super block and
* thus don't know the file system sector size yet.
@@ -2122,7 +2135,7 @@ xfs_alloc_buftarg(
if (xfs_setsize_buftarg(btp, bdev_logical_block_size(btp->bt_bdev)))
goto error_free;
if (xfs_init_buftarg(btp, bdev_logical_block_size(btp->bt_bdev),
- mp->m_super->s_id))
+ awu_min, awu_max, mp->m_super->s_id))
goto error_free;

Rather than passing this into the constructor and making the xmbuf code
pass zeroes, why not set the awu values here in xfs_alloc_buftarg just
before returning btp?

ok, I can do that instead.

Thanks,
John