* [PATCH v12 02/17] xfs: only call xfs_setsize_buftarg once per buffer target
From: John Garry @ 2025-05-06 9:04 UTC (permalink / raw)
To: brauner, djwong, hch, viro, jack, cem
Cc: linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, John Garry
In-Reply-To: <20250506090427.2549456-1-john.g.garry@oracle.com>
From: "Darrick J. Wong" <djwong@kernel.org>
It's silly to call xfs_setsize_buftarg from xfs_alloc_buftarg with the
block device LBA size because we don't need to ask the block layer to
validate a geometry number that it provided us. Instead, set the
preliminary bt_meta_sector* fields to the LBA size in preparation for
reading the primary super.
However, we still want to flush and invalidate the pagecache for all
three block devices before we start reading metadata from those devices,
so call sync_blockdev() per bdev in xfs_alloc_buftarg().
This will enable a subsequent patch to validate hw atomic write geometry
against the filesystem geometry.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: John Garry <john.g.garry@oracle.com>
[jpg: call sync_blockdev() from xfs_alloc_buftarg()]
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_buf.c | 28 ++++++++++++++++++----------
fs/xfs/xfs_super.c | 16 ++++++++++++----
2 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 5ae77ffdc947..d8f90bdd2a33 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1733,11 +1733,7 @@ xfs_setsize_buftarg(
return -EINVAL;
}
- /*
- * Flush the block device pagecache so our bios see anything dirtied
- * before mount.
- */
- return sync_blockdev(btp->bt_bdev);
+ return 0;
}
int
@@ -1786,6 +1782,8 @@ xfs_alloc_buftarg(
{
struct xfs_buftarg *btp;
const struct dax_holder_operations *ops = NULL;
+ int error;
+
#if defined(CONFIG_FS_DAX) && defined(CONFIG_MEMORY_FAILURE)
ops = &xfs_dax_holder_operations;
@@ -1806,21 +1804,31 @@ xfs_alloc_buftarg(
btp->bt_bdev);
}
+ /*
+ * Flush and invalidate all devices' pagecaches before reading any
+ * metadata because XFS doesn't use the bdev pagecache.
+ */
+ error = sync_blockdev(btp->bt_bdev);
+ if (error)
+ goto error_free;
+
/*
* When allocating the buftargs we have not yet read the super block and
* thus don't know the file system sector size yet.
*/
- 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))
+ btp->bt_meta_sectorsize = bdev_logical_block_size(btp->bt_bdev);
+ btp->bt_meta_sectormask = btp->bt_meta_sectorsize - 1;
+
+ error = xfs_init_buftarg(btp, btp->bt_meta_sectorsize,
+ mp->m_super->s_id);
+ if (error)
goto error_free;
return btp;
error_free:
kfree(btp);
- return NULL;
+ return ERR_PTR(error);
}
static inline void
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5e456a6073ca..6eba90eb7297 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -482,21 +482,29 @@ xfs_open_devices(
/*
* Setup xfs_mount buffer target pointers
*/
- error = -ENOMEM;
mp->m_ddev_targp = xfs_alloc_buftarg(mp, sb->s_bdev_file);
- if (!mp->m_ddev_targp)
+ if (IS_ERR(mp->m_ddev_targp)) {
+ error = PTR_ERR(mp->m_ddev_targp);
+ mp->m_ddev_targp = NULL;
goto out_close_rtdev;
+ }
if (rtdev_file) {
mp->m_rtdev_targp = xfs_alloc_buftarg(mp, rtdev_file);
- if (!mp->m_rtdev_targp)
+ if (IS_ERR(mp->m_rtdev_targp)) {
+ error = PTR_ERR(mp->m_rtdev_targp);
+ mp->m_rtdev_targp = NULL;
goto out_free_ddev_targ;
+ }
}
if (logdev_file && file_bdev(logdev_file) != ddev) {
mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
- if (!mp->m_logdev_targp)
+ if (IS_ERR(mp->m_logdev_targp)) {
+ error = PTR_ERR(mp->m_logdev_targp);
+ mp->m_logdev_targp = NULL;
goto out_free_rtdev_targ;
+ }
} else {
mp->m_logdev_targp = mp->m_ddev_targp;
/* Handle won't be used, drop it */
--
2.31.1
^ permalink raw reply related
* [PATCH v12 04/17] xfs: separate out setting buftarg atomic writes limits
From: John Garry @ 2025-05-06 9:04 UTC (permalink / raw)
To: brauner, djwong, hch, viro, jack, cem
Cc: linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, John Garry
In-Reply-To: <20250506090427.2549456-1-john.g.garry@oracle.com>
From: "Darrick J. Wong" <djwong@kernel.org>
Separate out setting buftarg atomic writes limits into a dedicated
function, xfs_configure_buftarg_atomic_writes(), to keep the specific
functionality self-contained.
For naming consistency, rename xfs_setsize_buftarg() ->
xfs_configure_buftarg().
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
[jpg: separate out from patch "xfs: ignore HW which ..."]
Signed-off-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf.c | 32 ++++++++++++++++++++++++--------
fs/xfs/xfs_buf.h | 2 +-
fs/xfs/xfs_super.c | 6 +++---
3 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index d8f90bdd2a33..e2374c503e79 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1714,13 +1714,33 @@ xfs_free_buftarg(
kfree(btp);
}
+/*
+ * Configure this buffer target for hardware-assisted atomic writes if the
+ * underlying block device supports is congruent with the filesystem geometry.
+ */
+static inline void
+xfs_configure_buftarg_atomic_writes(
+ struct xfs_buftarg *btp)
+{
+ unsigned int min_bytes, max_bytes;
+
+ min_bytes = bdev_atomic_write_unit_min_bytes(btp->bt_bdev);
+ max_bytes = bdev_atomic_write_unit_max_bytes(btp->bt_bdev);
+
+ btp->bt_bdev_awu_min = min_bytes;
+ btp->bt_bdev_awu_max = max_bytes;
+}
+
+/* Configure a buffer target that abstracts a block device. */
int
-xfs_setsize_buftarg(
+xfs_configure_buftarg(
struct xfs_buftarg *btp,
unsigned int sectorsize)
{
int error;
+ ASSERT(btp->bt_bdev != NULL);
+
/* Set up metadata sector size info */
btp->bt_meta_sectorsize = sectorsize;
btp->bt_meta_sectormask = sectorsize - 1;
@@ -1733,6 +1753,9 @@ xfs_setsize_buftarg(
return -EINVAL;
}
+ if (bdev_can_atomic_write(btp->bt_bdev))
+ xfs_configure_buftarg_atomic_writes(btp);
+
return 0;
}
@@ -1797,13 +1820,6 @@ 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)) {
- btp->bt_bdev_awu_min = bdev_atomic_write_unit_min_bytes(
- btp->bt_bdev);
- btp->bt_bdev_awu_max = bdev_atomic_write_unit_max_bytes(
- btp->bt_bdev);
- }
-
/*
* Flush and invalidate all devices' pagecaches before reading any
* metadata because XFS doesn't use the bdev pagecache.
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index d0b065a9a9f0..a7026fb255c4 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -374,7 +374,7 @@ struct xfs_buftarg *xfs_alloc_buftarg(struct xfs_mount *mp,
extern void xfs_free_buftarg(struct xfs_buftarg *);
extern void xfs_buftarg_wait(struct xfs_buftarg *);
extern void xfs_buftarg_drain(struct xfs_buftarg *);
-extern int xfs_setsize_buftarg(struct xfs_buftarg *, unsigned int);
+int xfs_configure_buftarg(struct xfs_buftarg *btp, unsigned int sectorsize);
#define xfs_getsize_buftarg(buftarg) block_size((buftarg)->bt_bdev)
#define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6eba90eb7297..77a3c003fc4f 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -537,7 +537,7 @@ xfs_setup_devices(
{
int error;
- error = xfs_setsize_buftarg(mp->m_ddev_targp, mp->m_sb.sb_sectsize);
+ error = xfs_configure_buftarg(mp->m_ddev_targp, mp->m_sb.sb_sectsize);
if (error)
return error;
@@ -546,7 +546,7 @@ xfs_setup_devices(
if (xfs_has_sector(mp))
log_sector_size = mp->m_sb.sb_logsectsize;
- error = xfs_setsize_buftarg(mp->m_logdev_targp,
+ error = xfs_configure_buftarg(mp->m_logdev_targp,
log_sector_size);
if (error)
return error;
@@ -560,7 +560,7 @@ xfs_setup_devices(
}
mp->m_rtdev_targp = mp->m_ddev_targp;
} else if (mp->m_rtname) {
- error = xfs_setsize_buftarg(mp->m_rtdev_targp,
+ error = xfs_configure_buftarg(mp->m_rtdev_targp,
mp->m_sb.sb_sectsize);
if (error)
return error;
--
2.31.1
^ permalink raw reply related
* [PATCH v12 03/17] xfs: rename xfs_inode_can_atomicwrite() -> xfs_inode_can_hw_atomic_write()
From: John Garry @ 2025-05-06 9:04 UTC (permalink / raw)
To: brauner, djwong, hch, viro, jack, cem
Cc: linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, John Garry
In-Reply-To: <20250506090427.2549456-1-john.g.garry@oracle.com>
In future we will want to be able to check if specifically HW offload-based
atomic writes are possible, so rename xfs_inode_can_atomicwrite() ->
xfs_inode_can_hw_atomicwrite().
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
[djwong: add an underscore to be consistent with everything else]
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_file.c | 2 +-
fs/xfs/xfs_inode.h | 4 +---
fs/xfs/xfs_iops.c | 2 +-
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 84f08c976ac4..55bdae44e42a 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1488,7 +1488,7 @@ xfs_file_open(
if (xfs_is_shutdown(XFS_M(inode->i_sb)))
return -EIO;
file->f_mode |= FMODE_NOWAIT | FMODE_CAN_ODIRECT;
- if (xfs_inode_can_atomicwrite(XFS_I(inode)))
+ if (xfs_inode_can_hw_atomic_write(XFS_I(inode)))
file->f_mode |= FMODE_CAN_ATOMIC_WRITE;
return generic_file_open(inode, file);
}
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index eae0159983ca..d3471a7418b9 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -356,9 +356,7 @@ static inline bool xfs_inode_has_bigrtalloc(const struct xfs_inode *ip)
(XFS_IS_REALTIME_INODE(ip) ? \
(ip)->i_mount->m_rtdev_targp : (ip)->i_mount->m_ddev_targp)
-static inline bool
-xfs_inode_can_atomicwrite(
- struct xfs_inode *ip)
+static inline bool xfs_inode_can_hw_atomic_write(const struct xfs_inode *ip)
{
struct xfs_mount *mp = ip->i_mount;
struct xfs_buftarg *target = xfs_inode_buftarg(ip);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f0e5d83195df..22432c300fd7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -608,7 +608,7 @@ xfs_report_atomic_write(
{
unsigned int unit_min = 0, unit_max = 0;
- if (xfs_inode_can_atomicwrite(ip))
+ if (xfs_inode_can_hw_atomic_write(ip))
unit_min = unit_max = ip->i_mount->m_sb.sb_blocksize;
generic_fill_statx_atomic_writes(stat, unit_min, unit_max, 0);
}
--
2.31.1
^ permalink raw reply related
* [PATCH v12 01/17] fs: add atomic write unit max opt to statx
From: John Garry @ 2025-05-06 9:04 UTC (permalink / raw)
To: brauner, djwong, hch, viro, jack, cem
Cc: linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, John Garry
In-Reply-To: <20250506090427.2549456-1-john.g.garry@oracle.com>
XFS will be able to support large atomic writes (atomic write > 1x block)
in future. This will be achieved by using different operating methods,
depending on the size of the write.
Specifically a new method of operation based in FS atomic extent remapping
will be supported in addition to the current HW offload-based method.
The FS method will generally be appreciably slower performing than the
HW-offload method. However the FS method will be typically able to
contribute to achieving a larger atomic write unit max limit.
XFS will support a hybrid mode, where HW offload method will be used when
possible, i.e. HW offload is used when the length of the write is
supported, and for other times FS-based atomic writes will be used.
As such, there is an atomic write length at which the user may experience
appreciably slower performance.
Advertise this limit in a new statx field, stx_atomic_write_unit_max_opt.
When zero, it means that there is no such performance boundary.
Masks STATX{_ATTR}_WRITE_ATOMIC can be used to get this new field. This is
ok for older kernels which don't support this new field, as they would
report 0 in this field (from zeroing in cp_statx()) already. Furthermore
those older kernels don't support large atomic writes - apart from block
fops, but there would be consistent performance there for atomic writes
in range [unit min, unit max].
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Acked-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
block/bdev.c | 3 ++-
fs/ext4/inode.c | 2 +-
fs/stat.c | 6 +++++-
fs/xfs/xfs_iops.c | 2 +-
include/linux/fs.h | 3 ++-
include/linux/stat.h | 1 +
include/uapi/linux/stat.h | 8 ++++++--
7 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/block/bdev.c b/block/bdev.c
index 520515e4e64e..9f321fb94bac 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -1336,7 +1336,8 @@ void bdev_statx(struct path *path, struct kstat *stat,
generic_fill_statx_atomic_writes(stat,
queue_atomic_write_unit_min_bytes(bd_queue),
- queue_atomic_write_unit_max_bytes(bd_queue));
+ queue_atomic_write_unit_max_bytes(bd_queue),
+ 0);
}
stat->blksize = bdev_io_min(bdev);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 94c7d2d828a6..cdf01e60fa6d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5692,7 +5692,7 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path,
awu_max = sbi->s_awu_max;
}
- generic_fill_statx_atomic_writes(stat, awu_min, awu_max);
+ generic_fill_statx_atomic_writes(stat, awu_min, awu_max, 0);
}
flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
diff --git a/fs/stat.c b/fs/stat.c
index f13308bfdc98..c41855f62d22 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -136,13 +136,15 @@ EXPORT_SYMBOL(generic_fill_statx_attr);
* @stat: Where to fill in the attribute flags
* @unit_min: Minimum supported atomic write length in bytes
* @unit_max: Maximum supported atomic write length in bytes
+ * @unit_max_opt: Optimised maximum supported atomic write length in bytes
*
* Fill in the STATX{_ATTR}_WRITE_ATOMIC flags in the kstat structure from
* atomic write unit_min and unit_max values.
*/
void generic_fill_statx_atomic_writes(struct kstat *stat,
unsigned int unit_min,
- unsigned int unit_max)
+ unsigned int unit_max,
+ unsigned int unit_max_opt)
{
/* Confirm that the request type is known */
stat->result_mask |= STATX_WRITE_ATOMIC;
@@ -153,6 +155,7 @@ void generic_fill_statx_atomic_writes(struct kstat *stat,
if (unit_min) {
stat->atomic_write_unit_min = unit_min;
stat->atomic_write_unit_max = unit_max;
+ stat->atomic_write_unit_max_opt = unit_max_opt;
/* Initially only allow 1x segment */
stat->atomic_write_segments_max = 1;
@@ -732,6 +735,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
tmp.stx_atomic_write_unit_min = stat->atomic_write_unit_min;
tmp.stx_atomic_write_unit_max = stat->atomic_write_unit_max;
tmp.stx_atomic_write_segments_max = stat->atomic_write_segments_max;
+ tmp.stx_atomic_write_unit_max_opt = stat->atomic_write_unit_max_opt;
return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
}
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 756bd3ca8e00..f0e5d83195df 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -610,7 +610,7 @@ xfs_report_atomic_write(
if (xfs_inode_can_atomicwrite(ip))
unit_min = unit_max = ip->i_mount->m_sb.sb_blocksize;
- generic_fill_statx_atomic_writes(stat, unit_min, unit_max);
+ generic_fill_statx_atomic_writes(stat, unit_min, unit_max, 0);
}
STATIC int
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 016b0fe1536e..7b19d8f99aff 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3475,7 +3475,8 @@ void generic_fillattr(struct mnt_idmap *, u32, struct inode *, struct kstat *);
void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
void generic_fill_statx_atomic_writes(struct kstat *stat,
unsigned int unit_min,
- unsigned int unit_max);
+ unsigned int unit_max,
+ unsigned int unit_max_opt);
extern int vfs_getattr_nosec(const struct path *, struct kstat *, u32, unsigned int);
extern int vfs_getattr(const struct path *, struct kstat *, u32, unsigned int);
void __inode_add_bytes(struct inode *inode, loff_t bytes);
diff --git a/include/linux/stat.h b/include/linux/stat.h
index be7496a6a0dd..e3d00e7bb26d 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -57,6 +57,7 @@ struct kstat {
u32 dio_read_offset_align;
u32 atomic_write_unit_min;
u32 atomic_write_unit_max;
+ u32 atomic_write_unit_max_opt;
u32 atomic_write_segments_max;
};
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index f78ee3670dd5..1686861aae20 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -182,8 +182,12 @@ struct statx {
/* File offset alignment for direct I/O reads */
__u32 stx_dio_read_offset_align;
- /* 0xb8 */
- __u64 __spare3[9]; /* Spare space for future expansion */
+ /* Optimised max atomic write unit in bytes */
+ __u32 stx_atomic_write_unit_max_opt;
+ __u32 __spare2[1];
+
+ /* 0xc0 */
+ __u64 __spare3[8]; /* Spare space for future expansion */
/* 0x100 */
};
--
2.31.1
^ permalink raw reply related
* [PATCH v12 00/17] large atomic writes for xfs
From: John Garry @ 2025-05-06 9:04 UTC (permalink / raw)
To: brauner, djwong, hch, viro, jack, cem
Cc: linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, John Garry
Currently atomic write support for xfs is limited to writing a single
block as we have no way to guarantee alignment and that the write covers
a single extent.
This series introduces a method to issue atomic writes via a
software-based method.
The software-based method is used as a fallback for when attempting to
issue an atomic write over misaligned or multiple extents.
For xfs, this support is based on reflink CoW support.
The basic idea of this CoW method is to alloc a range in the CoW fork,
write the data, and atomically update the mapping.
Initial mysql performance testing has shown this method to perform ok.
However, there we are only using 16K atomic writes (and 4K block size),
so typically - and thankfully - this software fallback method won't be
used often.
For other FSes which want large atomics writes and don't support CoW, I
think that they can follow the example in [0].
Catherine is currently working on further xfstests for this feature,
which we hope to share soon.
About 17/17, maybe it can be omitted as there is no strong demand to have
it included.
Based on bfecc4091e07 (xfs/next-rc, xfs/for-next) xfs: allow ro mounts
if rtdev or logdev are read-only
[0] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
Differences to v11:
- split "xfs: ignore ..." patch
- inline sync_blockdev() in xfs_alloc_buftarg() (Christoph)
- fix xfs_calc_rtgroup_awu_max() for 0 block count (Darrick)
- Add RB tag from Christoph (thanks!)
Differences to v10:
- add "xfs: only call xfs_setsize_buftarg once ..." by Darrick
- symbol renames in "xfs: ignore HW which cannot..." by Darrick
Differences to v9:
- rework "ignore HW which cannot .." patch by Darrick
- Ensure power-of-2 max always for unit min/max when no HW support
Darrick J. Wong (6):
xfs: only call xfs_setsize_buftarg once per buffer target
xfs: separate out setting buftarg atomic writes limits
xfs: add helpers to compute log item overhead
xfs: add helpers to compute transaction reservation for finishing
intent items
xfs: ignore HW which cannot atomic write a single block
xfs: allow sysadmins to specify a maximum atomic write limit at mount
time
John Garry (11):
fs: add atomic write unit max opt to statx
xfs: rename xfs_inode_can_atomicwrite() ->
xfs_inode_can_hw_atomic_write()
xfs: allow block allocator to take an alignment hint
xfs: refactor xfs_reflink_end_cow_extent()
xfs: refine atomic write size check in xfs_file_write_iter()
xfs: add xfs_atomic_write_cow_iomap_begin()
xfs: add large atomic writes checks in xfs_direct_write_iomap_begin()
xfs: commit CoW-based atomic writes atomically
xfs: add xfs_file_dio_write_atomic()
xfs: add xfs_calc_atomic_write_unit_max()
xfs: update atomic write limits
Documentation/admin-guide/xfs.rst | 11 +
block/bdev.c | 3 +-
fs/ext4/inode.c | 2 +-
fs/stat.c | 6 +-
fs/xfs/libxfs/xfs_bmap.c | 5 +
fs/xfs/libxfs/xfs_bmap.h | 6 +-
fs/xfs/libxfs/xfs_log_rlimit.c | 4 +
fs/xfs/libxfs/xfs_trans_resv.c | 343 +++++++++++++++++++++++++++---
fs/xfs/libxfs/xfs_trans_resv.h | 25 +++
fs/xfs/xfs_bmap_item.c | 10 +
fs/xfs/xfs_bmap_item.h | 3 +
fs/xfs/xfs_buf.c | 70 ++++--
fs/xfs/xfs_buf.h | 4 +-
fs/xfs/xfs_buf_item.c | 19 ++
fs/xfs/xfs_buf_item.h | 3 +
fs/xfs/xfs_extfree_item.c | 10 +
fs/xfs/xfs_extfree_item.h | 3 +
fs/xfs/xfs_file.c | 87 +++++++-
fs/xfs/xfs_inode.h | 14 +-
fs/xfs/xfs_iomap.c | 190 ++++++++++++++++-
fs/xfs/xfs_iomap.h | 1 +
fs/xfs/xfs_iops.c | 76 ++++++-
fs/xfs/xfs_iops.h | 3 +
fs/xfs/xfs_log_cil.c | 4 +-
fs/xfs/xfs_log_priv.h | 13 ++
fs/xfs/xfs_mount.c | 161 ++++++++++++++
fs/xfs/xfs_mount.h | 17 ++
fs/xfs/xfs_refcount_item.c | 10 +
fs/xfs/xfs_refcount_item.h | 3 +
fs/xfs/xfs_reflink.c | 146 ++++++++++---
fs/xfs/xfs_reflink.h | 6 +
fs/xfs/xfs_rmap_item.c | 10 +
fs/xfs/xfs_rmap_item.h | 3 +
fs/xfs/xfs_super.c | 80 ++++++-
fs/xfs/xfs_trace.h | 115 ++++++++++
include/linux/fs.h | 3 +-
include/linux/stat.h | 1 +
include/uapi/linux/stat.h | 8 +-
38 files changed, 1351 insertions(+), 127 deletions(-)
--
2.31.1
^ permalink raw reply
* Re: [PATCH v1 bpf-next 4/5] bpf: Add kfunc to scrub SCM_RIGHTS at security_unix_may_send().
From: Mickaël Salaün @ 2025-05-06 8:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kuniyuki Iwashima, Martin KaFai Lau, Daniel Borkmann,
John Fastabend, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Günther Noack,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Ondrej Mosnacek, Casey Schaufler, Christian Brauner,
Kuniyuki Iwashima, bpf, Network Development, LSM List, selinux,
Kees Cook, Jann Horn, linux-api
In-Reply-To: <CAADnVQK1t3ZqERODdHJM_HaZDMm+JH4OFvwTsLNqZG0=4SQQcA@mail.gmail.com>
On Mon, May 05, 2025 at 05:13:32PM -0700, Alexei Starovoitov wrote:
> On Mon, May 5, 2025 at 3:00 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > As Christian Brauner said [0], systemd calls cmsg_close_all() [1] after
> > each recvmsg() to close() unwanted file descriptors sent via SCM_RIGHTS.
> >
> > However, this cannot work around the issue that close() for unwanted file
> > descriptors could block longer because the last fput() could occur on
> > the receiver side once sendmsg() with SCM_RIGHTS succeeds.
> >
> > Also, even filtering by LSM at recvmsg() does not work for the same reason.
> >
> > Thus, we need a better way to filter SCM_RIGHTS on the sender side.
> >
> > Let's add a new kfunc to scrub all file descriptors from skb in
> > sendmsg().
> >
> > This allows the receiver to keep recv()ing the bare data and disallows
> > the sender to impose the potential slowness of the last fput().
> >
> > If necessary, we can add more granular filtering per file descriptor
> > after refactoring GC code and adding some fd-to-file helpers for BPF.
> >
> > Sample:
> >
> > SEC("lsm/unix_may_send")
> > int BPF_PROG(unix_scrub_scm_rights,
> > struct socket *sock, struct socket *other, struct sk_buff *skb)
> > {
> > struct unix_skb_parms *cb;
> >
> > if (skb && bpf_unix_scrub_fds(skb))
> > return -EPERM;
> >
> > return 0;
> > }
>
> Any other programmability do you need there?
>
> If not and above is all that is needed then what Jann proposed
> sounds like better path to me:
> "
> I think the thorough fix would probably be to introduce a socket
> option (controlled via setsockopt()) that already blocks the peer's
> sendmsg().
> "
>
> Easier to operate and upriv process can use such setsockopt() too.
Adding a flag with setsockopt() will enable any program to protect
themselves instead of requiring the capability to load an eBPF program.
For the systemd use case, I think a flag would be enough, and it would
benefit more than only/mainly systemd.
Another thing is that we should have a consistent user space error code
if passing file descriptors is denied, to avoid confusing senders, to
not silently ignore dropped file descriptors, and to let the sender know
that it can send again but without passed file descriptors or maybe with
a maximum number of file descriptors. The ENFILE errno (file table
overflow) looks like a good candidate.
I guess both approaches are valuable, but this series should add this
new flag as well, and specify the expected error handling.
If we want to be able to handle legacy software not using this new flag,
we can leverage seccomp unotify.
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: John Garry @ 2025-05-06 6:57 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Darrick J. Wong, brauner, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <20250506042242.GA26378@lst.de>
On 06/05/2025 05:22, Christoph Hellwig wrote:
>>> It seems simpler to just have the individual sync_blockdev() calls from
>>> xfs_alloc_buftarg(), rather than adding ERR_PTR() et al handling in both
>>> xfs_alloc_buftarg() and xfs_open_devices().
>> Which of the following is better:
> To me version 2 looks much better. I had initial reservations as
> ERR_PTR doesn't play well with userspace, but none of this code is
> in libxfs, so that should be fine.
cool, that is what we/I had gone with in prep'ing v12
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: Christoph Hellwig @ 2025-05-06 4:22 UTC (permalink / raw)
To: John Garry
Cc: Darrick J. Wong, Christoph Hellwig, brauner, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api
In-Reply-To: <200d855d-550d-4207-9118-6a0c10d14f8a@oracle.com>
On Mon, May 05, 2025 at 04:27:56PM +0100, John Garry wrote:
> On 05/05/2025 15:48, John Garry wrote:
>>>> @Darrick, please comment on whether happy with changes discussed.
>>> I put the sync_blockdev calls in a separate function so that the
>>> EIO/ENOSPC/whatever errors that come from the block device sync don't
>>> get morphed into ENOMEM by xfs_alloc_buftarg before being passed up. I
>>> suppose we could make that function return an ERR_PTR, but I was trying
>>> to avoid making even more changes at the last minute, again.
>>
>> It seems simpler to just have the individual sync_blockdev() calls from
>> xfs_alloc_buftarg(), rather than adding ERR_PTR() et al handling in both
>> xfs_alloc_buftarg() and xfs_open_devices().
>
> Which of the following is better:
To me version 2 looks much better. I had initial reservations as
ERR_PTR doesn't play well with userspace, but none of this code is
in libxfs, so that should be fine.
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: John Garry @ 2025-05-05 15:27 UTC (permalink / raw)
To: Darrick J. Wong, Christoph Hellwig
Cc: brauner, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <40def355-38db-4424-b9f0-b82bba62462b@oracle.com>
On 05/05/2025 15:48, John Garry wrote:
>>> @Darrick, please comment on whether happy with changes discussed.
>> I put the sync_blockdev calls in a separate function so that the
>> EIO/ENOSPC/whatever errors that come from the block device sync don't
>> get morphed into ENOMEM by xfs_alloc_buftarg before being passed up. I
>> suppose we could make that function return an ERR_PTR, but I was trying
>> to avoid making even more changes at the last minute, again.
>
> It seems simpler to just have the individual sync_blockdev() calls from
> xfs_alloc_buftarg(), rather than adding ERR_PTR() et al handling in both
> xfs_alloc_buftarg() and xfs_open_devices().
Which of the following is better:
------8<-----
int
@@ -1810,10 +1806,10 @@ xfs_alloc_buftarg(
* When allocating the buftargs we have not yet read the super block and
* thus don't know the file system sector size yet.
*/
- 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))
+ btp->bt_meta_sectorsize = bdev_logical_block_size(btp->bt_bdev);
+ btp->bt_meta_sectormask = btp->bt_meta_sectorsize - 1;
+
+ if (xfs_init_buftarg(btp, btp->bt_meta_sectorsize, mp->m_super->s_id))
goto error_free;
return btp;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5e456a6073ca..48d5b630fe46 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -481,21 +481,38 @@ xfs_open_devices(
/*
* Setup xfs_mount buffer target pointers
+ *
+ * Flush and invalidate all devices' pagecaches before reading any
+ * metadata because XFS doesn't use the bdev pagecache.
*/
- error = -ENOMEM;
mp->m_ddev_targp = xfs_alloc_buftarg(mp, sb->s_bdev_file);
- if (!mp->m_ddev_targp)
+ if (!mp->m_ddev_targp) {
+ error = -ENOMEM;
+ goto out_close_rtdev;
+ }
+ error = sync_blockdev(mp->m_ddev_targp->bt_bdev);
+ if (error)
goto out_close_rtdev;
if (rtdev_file) {
mp->m_rtdev_targp = xfs_alloc_buftarg(mp, rtdev_file);
- if (!mp->m_rtdev_targp)
+ if (!mp->m_rtdev_targp) {
+ error = -ENOMEM;
+ goto out_free_ddev_targ;
+ }
+ error = sync_blockdev(mp->m_rtdev_targp->bt_bdev);
+ if (error)
goto out_free_ddev_targ;
}
if (logdev_file && file_bdev(logdev_file) != ddev) {
mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
- if (!mp->m_logdev_targp)
+ if (!mp->m_logdev_targp) {
+ error = -ENOMEM;
+ goto out_free_rtdev_targ;
+ }
+ error = sync_blockdev(mp->m_logdev_targp->bt_bdev);
+ if (error)
goto out_free_rtdev_targ;
----->8------
int
@@ -1786,6 +1782,8 @@ xfs_alloc_buftarg(
{
struct xfs_buftarg *btp;
const struct dax_holder_operations *ops = NULL;
+ int error;
+
#if defined(CONFIG_FS_DAX) && defined(CONFIG_MEMORY_FAILURE)
ops = &xfs_dax_holder_operations;
@@ -1806,21 +1804,31 @@ xfs_alloc_buftarg(
btp->bt_bdev);
}
+ /*
+ * Flush and invalidate all devices' pagecaches before reading any
+ * metadata because XFS doesn't use the bdev pagecache.
+ */
+ error = sync_blockdev(mp->m_ddev_targp->bt_bdev);
+ if (error)
+ goto error_free;
+
/*
* When allocating the buftargs we have not yet read the super block and
* thus don't know the file system sector size yet.
*/
- 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))
+ btp->bt_meta_sectorsize = bdev_logical_block_size(btp->bt_bdev);
+ btp->bt_meta_sectormask = btp->bt_meta_sectorsize - 1;
+
+ if (xfs_init_buftarg(btp, btp->bt_meta_sectorsize, mp->m_super->s_id)) {
+ error = -ENOMEM;
goto error_free;
+ }
return btp;
error_free:
kfree(btp);
- return NULL;
+ return ERR_PTR(error);
}
static inline void
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5e456a6073ca..4daf0cc480af 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -482,21 +482,26 @@ xfs_open_devices(
/*
* Setup xfs_mount buffer target pointers
*/
- error = -ENOMEM;
mp->m_ddev_targp = xfs_alloc_buftarg(mp, sb->s_bdev_file);
- if (!mp->m_ddev_targp)
+ if (IS_ERR(mp->m_ddev_targp)) {
+ error = PTR_ERR(mp->m_ddev_targp);
goto out_close_rtdev;
+ }
if (rtdev_file) {
mp->m_rtdev_targp = xfs_alloc_buftarg(mp, rtdev_file);
- if (!mp->m_rtdev_targp)
+ if (IS_ERR(mp->m_rtdev_targp)) {
+ error = PTR_ERR(mp->m_rtdev_targp);
goto out_free_ddev_targ;
+ }
}
if (logdev_file && file_bdev(logdev_file) != ddev) {
mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
- if (!mp->m_logdev_targp)
+ if (IS_ERR(mp->m_logdev_targp)) {
+ error = PTR_ERR(mp->m_logdev_targp);
goto out_free_rtdev_targ;
+ }
----8<-----
^ permalink raw reply related
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: John Garry @ 2025-05-05 14:48 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Christoph Hellwig, brauner, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <20250505142234.GG1035866@frogsfrogsfrogs>
On 05/05/2025 15:22, Darrick J. Wong wrote:
>>> Yes. Or in fact just folding it into xfs_alloc_buftarg, which might
>>> be even simpler.
>> Yes, that was my next question..
>>
>>> While you're at it adding a command why we are doing
>>> the sync would also be really useful, and having it in just one place
>>> helps with that.
>> ok, there was such comment in xfs_preflush_devices().
>>
>> @Darrick, please comment on whether happy with changes discussed.
> I put the sync_blockdev calls in a separate function so that the
> EIO/ENOSPC/whatever errors that come from the block device sync don't
> get morphed into ENOMEM by xfs_alloc_buftarg before being passed up. I
> suppose we could make that function return an ERR_PTR, but I was trying
> to avoid making even more changes at the last minute, again.
It seems simpler to just have the individual sync_blockdev() calls from
xfs_alloc_buftarg(), rather than adding ERR_PTR() et al handling in both
xfs_alloc_buftarg() and xfs_open_devices().
^ permalink raw reply
* Re: [PATCH v11 14/16] xfs: add xfs_calc_atomic_write_unit_max()
From: Darrick J. Wong @ 2025-05-05 14:26 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <9d5d7037-6ed7-4c61-afec-8422d656de37@oracle.com>
On Mon, May 05, 2025 at 09:02:31AM +0100, John Garry wrote:
> On 05/05/2025 07:08, John Garry wrote:
> > On 05/05/2025 06:25, Darrick J. Wong wrote:
> > > Ok so I even attached the reply to the WRONG VERSION. Something in
> > > these changes cause xfs/289 to barf up this UBSAN warning, even on a
> > > realtime + rtgroups volume:
>
> Could this just be from another mount (of not a realtime + rtgroups xfs
> instance)?
Quite possibly.
> > >
> > > [ 1160.539004] ------------[ cut here ]------------
> > > [ 1160.540701] UBSAN: shift-out-of-bounds in /storage/home/djwong/
> > > cdev/work/linux-djw/include/linux/log2.h:67:13
> > > [ 1160.544597] shift exponent 4294967295 is too large for 64-bit
> > > type 'long unsigned int'
> > > [ 1160.547038] CPU: 3 UID: 0 PID: 288421 Comm: mount Not tainted
> > > 6.15.0-rc5-djwx #rc5 PREEMPT(lazy)
> > > 6f606c17703b80ffff7378e7041918eca24b3e68
> > > [ 1160.547045] Hardware name: QEMU Standard PC (i440FX + PIIX,
> > > 1996), BIOS 1.16.0-4.module+el8.8.0+21164+ed375313 04/01/2014
> > > [ 1160.547047] Call Trace:
> > > [ 1160.547049] <TASK>
> > > [ 1160.547051] dump_stack_lvl+0x4f/0x60
> > > [ 1160.547060] __ubsan_handle_shift_out_of_bounds+0x1bc/0x380
> > > [ 1160.547066] xfs_set_max_atomic_write_opt.cold+0x22d/0x252 [xfs
> > > 1f657532c3dee9b1d567597a31645929273d3283]
> > > [ 1160.547249] xfs_mountfs+0xa5c/0xb50 [xfs
> > > 1f657532c3dee9b1d567597a31645929273d3283]
> > > [ 1160.547434] xfs_fs_fill_super+0x7eb/0xb30 [xfs
> > > 1f657532c3dee9b1d567597a31645929273d3283]
> > > [ 1160.547616] ? xfs_open_devices+0x240/0x240 [xfs
> > > 1f657532c3dee9b1d567597a31645929273d3283]
> > > [ 1160.547797] get_tree_bdev_flags+0x132/0x1d0
> > > [ 1160.547801] vfs_get_tree+0x17/0xa0
> > > [ 1160.547803] path_mount+0x720/0xa80
> > > [ 1160.547807] __x64_sys_mount+0x10c/0x140
> > > [ 1160.547810] do_syscall_64+0x47/0x100
> > > [ 1160.547814] entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > > [ 1160.547817] RIP: 0033:0x7fde55d62e0a
> > > [ 1160.547820] Code: 48 8b 0d f9 7f 0c 00 f7 d8 64 89 01 48 83 c8 ff
> > > c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00
> > > 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c6 7f 0c 00 f7 d8 64
> > > 89 01 48
> > > [ 1160.547823] RSP: 002b:00007fff11920ce8 EFLAGS: 00000246 ORIG_RAX:
> > > 00000000000000a5
> > > [ 1160.547826] RAX: ffffffffffffffda RBX: 0000556a10cd1de0 RCX:
> > > 00007fde55d62e0a
> > > [ 1160.547828] RDX: 0000556a10cd2010 RSI: 0000556a10cd2090 RDI:
> > > 0000556a10ce2590
> > > [ 1160.547829] RBP: 0000000000000000 R08: 0000000000000000 R09:
> > > 00007fff11920d50
> > > [ 1160.547830] R10: 0000000000000000 R11: 0000000000000246 R12:
> > > 0000556a10ce2590
> > > [ 1160.547832] R13: 0000556a10cd2010 R14: 00007fde55eca264 R15:
> > > 0000556a10cd1ef8
> > > [ 1160.547834] </TASK>
> > > [ 1160.547835] ---[ end trace ]---
> > >
> > > John, can you please figure this one out, seeing as it's 10:30pm on
> > > Sunday night here?
>
>
> I could recreate this.
>
> >
>
> I think that we need this change:
>
> @@ -715,6 +716,9 @@ static inline xfs_extlen_t
> xfs_calc_rtgroup_awu_max(struct xfs_mount *mp)
> {
> struct xfs_groups *rgs = &mp->m_groups[XG_TYPE_RTG];
>
> + if (rgs->blocks == 0)
> + return 0;
> if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_bdev_awu_min > 0)
> return max_pow_of_two_factor(rgs->blocks);
> return rounddown_pow_of_two(rgs->blocks);
>
> My xfs/289 problem goes away with this change.
Ok good.
--D
>
> >
>
>
^ permalink raw reply
* Re: [PATCH v11 06/16] xfs: ignore HW which cannot atomic write a single block
From: Darrick J. Wong @ 2025-05-05 14:24 UTC (permalink / raw)
To: Christoph Hellwig
Cc: John Garry, brauner, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250505083050.GA31587@lst.de>
On Mon, May 05, 2025 at 10:30:50AM +0200, Christoph Hellwig wrote:
> On Mon, May 05, 2025 at 09:12:53AM +0100, John Garry wrote:
> > On 05/05/2025 06:45, John Garry wrote:
> >> On 05/05/2025 06:43, Christoph Hellwig wrote:
> >>> I think this subject line here is left from an earlier version and
> >>> doesn't quite seem to summarize what this patch is doing now?
> >
> > How about we just split this patch into 2 patches:
> > part 1 re-org with new helper xfs_configure_buftarg_atomic_writes()
> > part 2 ignore HW which cannot atomic write a single FS block
>
> Fine with me. Although just fixing up the subject sounds fine as well.
I don't care either way.
--D
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: Darrick J. Wong @ 2025-05-05 14:22 UTC (permalink / raw)
To: John Garry
Cc: Christoph Hellwig, brauner, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <bb8efa28-19e6-42f5-9a26-cdc0bc48926e@oracle.com>
On Mon, May 05, 2025 at 11:55:13AM +0100, John Garry wrote:
> On 05/05/2025 11:49, Christoph Hellwig wrote:
> > On Mon, May 05, 2025 at 11:04:55AM +0100, John Garry wrote:
> > > @@ -503,6 +509,9 @@ xfs_open_devices(
> > > mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
> > > if (!mp->m_logdev_targp)
> > > goto out_free_rtdev_targ;
> > > + error = sync_blockdev(mp->m_logdev_targp->bt_bdev);
> > > + if (error)
> > > + goto out_free_rtdev_targ;
> > > } else {
> > > mp->m_logdev_targp = mp->m_ddev_targp;
> > > /* Handle won't be used, drop it */
> > >
> > >
> > > Right?
> > Yes. Or in fact just folding it into xfs_alloc_buftarg, which might
> > be even simpler.
>
> Yes, that was my next question..
>
> > While you're at it adding a command why we are doing
> > the sync would also be really useful, and having it in just one place
> > helps with that.
>
> ok, there was such comment in xfs_preflush_devices().
>
> @Darrick, please comment on whether happy with changes discussed.
I put the sync_blockdev calls in a separate function so that the
EIO/ENOSPC/whatever errors that come from the block device sync don't
get morphed into ENOMEM by xfs_alloc_buftarg before being passed up. I
suppose we could make that function return an ERR_PTR, but I was trying
to avoid making even more changes at the last minute, again.
--D
> Thanks,
> John
>
>
^ permalink raw reply
* Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Christian Brauner @ 2025-05-05 13:35 UTC (permalink / raw)
To: Peter Zijlstra
Cc: John Hubbard, Lorenzo Stoakes, Shuah Khan, Christian Brauner,
Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, seanjc
In-Reply-To: <20250501124646.GC4356@noisy.programming.kicks-ass.net>
On Thu, May 01, 2025 at 02:46:46PM +0200, Peter Zijlstra wrote:
> On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
> > > On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
> > > > On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
> > > > > On 10/16/24 04:20, Lorenzo Stoakes wrote:
> > > ...
> > > > > > diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
> > > > > > index 88d6830ee004..1640b711889b 100644
> > > > > > --- a/tools/testing/selftests/pidfd/pidfd.h
> > > > > > +++ b/tools/testing/selftests/pidfd/pidfd.h
> > > > > > @@ -50,6 +50,14 @@
> > > > > > #define PIDFD_NONBLOCK O_NONBLOCK
> > > > > > #endif
> > > > > > +/* System header file may not have this available. */
> > > > > > +#ifndef PIDFD_SELF_THREAD
> > > > > > +#define PIDFD_SELF_THREAD -100
> > > > > > +#endif
> > > > > > +#ifndef PIDFD_SELF_THREAD_GROUP
> > > > > > +#define PIDFD_SELF_THREAD_GROUP -200
> > > > > > +#endif
> > > > > > +
> > > > >
> > > > > As mentioned in my response to v1 patch:
> > > > >
> > > > > kselftest has dependency on "make headers" and tests include
> > > > > headers from linux/ directory
> > > >
> > > > Right but that assumes you install the kernel headers on the build system,
> > > > which is quite a painful thing to have to do when you are quickly iterating
> > > > on a qemu setup.
> > > >
> > > > This is a use case I use all the time so not at all theoretical.
> > > >
> > >
> > > This is turning out to be a fairly typical reaction from kernel
> > > developers, when presented with the "you must first run make headers"
> > > requirement for kselftests.
> > >
> > > Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most
> > > colorful, so I'll helpfully cite it here. :)
> >
> > Let me re-try this.
> >
> > This is driving me insane. I've spend the past _TWO_ days trying to
> > build KVM selftests and I'm still failing.
> >
> > This is absolute atrocious crap and is costing me valuable time.
> >
> > Please fix this fucking selftests shit to just build. This is unusable
> > garbage.
>
> So after spending more time trying to remember how to debug Makefiles (I
> hate my life), I found that not only do I need this headers shit, the
> kvm selftests Makefile is actively broken if you use: make O=foo
>
> -INSTALL_HDR_PATH = $(top_srcdir)/usr
> +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr
>
>
> And then finally, I can do:
>
> make O=foo headers_install
> make O=foo -C tools/testing/selftests/kvm/
>
> So yeah, thank you very much for wasting my time *AGAIN*.
>
>
> Seriously, I want to be able to do:
>
> cd tools/testing/selftests/foo; make
>
> and have it just work. I would strongly suggest every subsystem to
> reclaim their selftests and make it so again.
>
> And on that, let me go merge the fixes I need to have x86 and futex
> build without this headers shit.
I'm completely lost as to what's happening here or whether the test here
is somehow at fault for something.
The pidfd.h head explicitly has no dependency on the pidfd uapi header
itself and I will NAK anything that makes it so. It's just a giant pain.
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: John Garry @ 2025-05-05 10:55 UTC (permalink / raw)
To: Christoph Hellwig
Cc: brauner, djwong, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250505104901.GA10128@lst.de>
On 05/05/2025 11:49, Christoph Hellwig wrote:
> On Mon, May 05, 2025 at 11:04:55AM +0100, John Garry wrote:
>> @@ -503,6 +509,9 @@ xfs_open_devices(
>> mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
>> if (!mp->m_logdev_targp)
>> goto out_free_rtdev_targ;
>> + error = sync_blockdev(mp->m_logdev_targp->bt_bdev);
>> + if (error)
>> + goto out_free_rtdev_targ;
>> } else {
>> mp->m_logdev_targp = mp->m_ddev_targp;
>> /* Handle won't be used, drop it */
>>
>>
>> Right?
> Yes. Or in fact just folding it into xfs_alloc_buftarg, which might
> be even simpler.
Yes, that was my next question..
> While you're at it adding a command why we are doing
> the sync would also be really useful, and having it in just one place
> helps with that.
ok, there was such comment in xfs_preflush_devices().
@Darrick, please comment on whether happy with changes discussed.
Thanks,
John
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: Christoph Hellwig @ 2025-05-05 10:49 UTC (permalink / raw)
To: John Garry
Cc: Christoph Hellwig, brauner, djwong, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api
In-Reply-To: <8ea91e81-9b96-458e-bd4e-64eada31e184@oracle.com>
On Mon, May 05, 2025 at 11:04:55AM +0100, John Garry wrote:
> @@ -503,6 +509,9 @@ xfs_open_devices(
> mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
> if (!mp->m_logdev_targp)
> goto out_free_rtdev_targ;
> + error = sync_blockdev(mp->m_logdev_targp->bt_bdev);
> + if (error)
> + goto out_free_rtdev_targ;
> } else {
> mp->m_logdev_targp = mp->m_ddev_targp;
> /* Handle won't be used, drop it */
>
>
> Right?
Yes. Or in fact just folding it into xfs_alloc_buftarg, which might
be even simpler. While you're at it adding a command why we are doing
the sync would also be really useful, and having it in just one place
helps with that.
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: John Garry @ 2025-05-05 10:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: brauner, djwong, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250505054031.GA20925@lst.de>
On 05/05/2025 06:40, Christoph Hellwig wrote:
>> +/*
>> + * Flush and invalidate all devices' pagecaches before reading any metadata
>> + * because XFS doesn't use the bdev pagecache.
>> + */
>> +STATIC int
>> +xfs_preflush_devices(
>> + struct xfs_mount *mp)
>> +{
>> + int error;
>> +
>> + error = xfs_buftarg_sync(mp->m_ddev_targp);
>> + if (error)
>> + return error;
>> +
>> + if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
>> + error = xfs_buftarg_sync(mp->m_ddev_targp);
>> + if (error)
>> + return error;
>> + }
> Why does this duplicate all the logic instead of being folded into
> xfs_open_devices?
So you mean an additive change like:
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 64fbd089ef55..9fa538938e07 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -488,6 +488,9 @@ xfs_open_devices(
mp->m_ddev_targp = xfs_alloc_buftarg(mp, sb->s_bdev_file);
if (!mp->m_ddev_targp)
goto out_close_rtdev;
+ error = sync_blockdev(mp->m_ddev_targp->bt_bdev);
+ if (error)
+ goto out_close_rtdev;
if (rtdev_file) {
@@ -495,6 +498,9 @@ xfs_open_devices(
mp->m_rtdev_targp = xfs_alloc_buftarg(mp, rtdev_file);
if (!mp->m_rtdev_targp)
goto out_free_ddev_targ;
+ error = sync_blockdev(mp->m_rtdev_targp->bt_bdev);
+ if (error)
+ goto out_free_ddev_targ;
}
if (logdev_file && file_bdev(logdev_file) != ddev) {
@@ -503,6 +509,9 @@ xfs_open_devices(
mp->m_logdev_targp = xfs_alloc_buftarg(mp, logdev_file);
if (!mp->m_logdev_targp)
goto out_free_rtdev_targ;
+ error = sync_blockdev(mp->m_logdev_targp->bt_bdev);
+ if (error)
+ goto out_free_rtdev_targ;
} else {
mp->m_logdev_targp = mp->m_ddev_targp;
/* Handle won't be used, drop it */
Right?
^ permalink raw reply related
* Re: [PATCH v11 06/16] xfs: ignore HW which cannot atomic write a single block
From: Christoph Hellwig @ 2025-05-05 8:30 UTC (permalink / raw)
To: John Garry
Cc: Christoph Hellwig, brauner, djwong, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api
In-Reply-To: <0b0d61e9-68e6-4eb0-a7bd-6e256e6d45f8@oracle.com>
On Mon, May 05, 2025 at 09:12:53AM +0100, John Garry wrote:
> On 05/05/2025 06:45, John Garry wrote:
>> On 05/05/2025 06:43, Christoph Hellwig wrote:
>>> I think this subject line here is left from an earlier version and
>>> doesn't quite seem to summarize what this patch is doing now?
>
> How about we just split this patch into 2 patches:
> part 1 re-org with new helper xfs_configure_buftarg_atomic_writes()
> part 2 ignore HW which cannot atomic write a single FS block
Fine with me. Although just fixing up the subject sounds fine as well.
^ permalink raw reply
* Re: [PATCH v11 06/16] xfs: ignore HW which cannot atomic write a single block
From: John Garry @ 2025-05-05 8:12 UTC (permalink / raw)
To: Christoph Hellwig
Cc: brauner, djwong, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <1d0e85d5-5e5c-4a8c-ae97-d90092c2c296@oracle.com>
On 05/05/2025 06:45, John Garry wrote:
> On 05/05/2025 06:43, Christoph Hellwig wrote:
>> I think this subject line here is left from an earlier version and
>> doesn't quite seem to summarize what this patch is doing now?
How about we just split this patch into 2 patches:
part 1 re-org with new helper xfs_configure_buftarg_atomic_writes()
part 2 ignore HW which cannot atomic write a single FS block
^ permalink raw reply
* Re: [PATCH v11 14/16] xfs: add xfs_calc_atomic_write_unit_max()
From: John Garry @ 2025-05-05 8:02 UTC (permalink / raw)
To: Darrick J. Wong
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <2a5688e8-88ef-4224-b757-af5adfca1be1@oracle.com>
On 05/05/2025 07:08, John Garry wrote:
> On 05/05/2025 06:25, Darrick J. Wong wrote:
>> Ok so I even attached the reply to the WRONG VERSION. Something in
>> these changes cause xfs/289 to barf up this UBSAN warning, even on a
>> realtime + rtgroups volume:
Could this just be from another mount (of not a realtime + rtgroups xfs
instance)?
>>
>> [ 1160.539004] ------------[ cut here ]------------
>> [ 1160.540701] UBSAN: shift-out-of-bounds in /storage/home/djwong/
>> cdev/work/linux-djw/include/linux/log2.h:67:13
>> [ 1160.544597] shift exponent 4294967295 is too large for 64-bit type
>> 'long unsigned int'
>> [ 1160.547038] CPU: 3 UID: 0 PID: 288421 Comm: mount Not tainted
>> 6.15.0-rc5-djwx #rc5 PREEMPT(lazy)
>> 6f606c17703b80ffff7378e7041918eca24b3e68
>> [ 1160.547045] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS 1.16.0-4.module+el8.8.0+21164+ed375313 04/01/2014
>> [ 1160.547047] Call Trace:
>> [ 1160.547049] <TASK>
>> [ 1160.547051] dump_stack_lvl+0x4f/0x60
>> [ 1160.547060] __ubsan_handle_shift_out_of_bounds+0x1bc/0x380
>> [ 1160.547066] xfs_set_max_atomic_write_opt.cold+0x22d/0x252 [xfs
>> 1f657532c3dee9b1d567597a31645929273d3283]
>> [ 1160.547249] xfs_mountfs+0xa5c/0xb50 [xfs
>> 1f657532c3dee9b1d567597a31645929273d3283]
>> [ 1160.547434] xfs_fs_fill_super+0x7eb/0xb30 [xfs
>> 1f657532c3dee9b1d567597a31645929273d3283]
>> [ 1160.547616] ? xfs_open_devices+0x240/0x240 [xfs
>> 1f657532c3dee9b1d567597a31645929273d3283]
>> [ 1160.547797] get_tree_bdev_flags+0x132/0x1d0
>> [ 1160.547801] vfs_get_tree+0x17/0xa0
>> [ 1160.547803] path_mount+0x720/0xa80
>> [ 1160.547807] __x64_sys_mount+0x10c/0x140
>> [ 1160.547810] do_syscall_64+0x47/0x100
>> [ 1160.547814] entry_SYSCALL_64_after_hwframe+0x4b/0x53
>> [ 1160.547817] RIP: 0033:0x7fde55d62e0a
>> [ 1160.547820] Code: 48 8b 0d f9 7f 0c 00 f7 d8 64 89 01 48 83 c8 ff
>> c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00
>> 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c6 7f 0c 00 f7 d8 64 89
>> 01 48
>> [ 1160.547823] RSP: 002b:00007fff11920ce8 EFLAGS: 00000246 ORIG_RAX:
>> 00000000000000a5
>> [ 1160.547826] RAX: ffffffffffffffda RBX: 0000556a10cd1de0 RCX:
>> 00007fde55d62e0a
>> [ 1160.547828] RDX: 0000556a10cd2010 RSI: 0000556a10cd2090 RDI:
>> 0000556a10ce2590
>> [ 1160.547829] RBP: 0000000000000000 R08: 0000000000000000 R09:
>> 00007fff11920d50
>> [ 1160.547830] R10: 0000000000000000 R11: 0000000000000246 R12:
>> 0000556a10ce2590
>> [ 1160.547832] R13: 0000556a10cd2010 R14: 00007fde55eca264 R15:
>> 0000556a10cd1ef8
>> [ 1160.547834] </TASK>
>> [ 1160.547835] ---[ end trace ]---
>>
>> John, can you please figure this one out, seeing as it's 10:30pm on
>> Sunday night here?
I could recreate this.
>
I think that we need this change:
@@ -715,6 +716,9 @@ static inline xfs_extlen_t
xfs_calc_rtgroup_awu_max(struct xfs_mount *mp)
{
struct xfs_groups *rgs = &mp->m_groups[XG_TYPE_RTG];
+ if (rgs->blocks == 0)
+ return 0;
if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_bdev_awu_min > 0)
return max_pow_of_two_factor(rgs->blocks);
return rounddown_pow_of_two(rgs->blocks);
My xfs/289 problem goes away with this change.
>
^ permalink raw reply
* Re: [PATCH v11 14/16] xfs: add xfs_calc_atomic_write_unit_max()
From: John Garry @ 2025-05-05 6:08 UTC (permalink / raw)
To: Darrick J. Wong
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250505052534.GX25675@frogsfrogsfrogs>
On 05/05/2025 06:25, Darrick J. Wong wrote:
> Ok so I even attached the reply to the WRONG VERSION. Something in
> these changes cause xfs/289 to barf up this UBSAN warning, even on a
> realtime + rtgroups volume:
>
> [ 1160.539004] ------------[ cut here ]------------
> [ 1160.540701] UBSAN: shift-out-of-bounds in /storage/home/djwong/cdev/work/linux-djw/include/linux/log2.h:67:13
> [ 1160.544597] shift exponent 4294967295 is too large for 64-bit type 'long unsigned int'
> [ 1160.547038] CPU: 3 UID: 0 PID: 288421 Comm: mount Not tainted 6.15.0-rc5-djwx #rc5 PREEMPT(lazy) 6f606c17703b80ffff7378e7041918eca24b3e68
> [ 1160.547045] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-4.module+el8.8.0+21164+ed375313 04/01/2014
> [ 1160.547047] Call Trace:
> [ 1160.547049] <TASK>
> [ 1160.547051] dump_stack_lvl+0x4f/0x60
> [ 1160.547060] __ubsan_handle_shift_out_of_bounds+0x1bc/0x380
> [ 1160.547066] xfs_set_max_atomic_write_opt.cold+0x22d/0x252 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
> [ 1160.547249] xfs_mountfs+0xa5c/0xb50 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
> [ 1160.547434] xfs_fs_fill_super+0x7eb/0xb30 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
> [ 1160.547616] ? xfs_open_devices+0x240/0x240 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
> [ 1160.547797] get_tree_bdev_flags+0x132/0x1d0
> [ 1160.547801] vfs_get_tree+0x17/0xa0
> [ 1160.547803] path_mount+0x720/0xa80
> [ 1160.547807] __x64_sys_mount+0x10c/0x140
> [ 1160.547810] do_syscall_64+0x47/0x100
> [ 1160.547814] entry_SYSCALL_64_after_hwframe+0x4b/0x53
> [ 1160.547817] RIP: 0033:0x7fde55d62e0a
> [ 1160.547820] Code: 48 8b 0d f9 7f 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c6 7f 0c 00 f7 d8 64 89 01 48
> [ 1160.547823] RSP: 002b:00007fff11920ce8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> [ 1160.547826] RAX: ffffffffffffffda RBX: 0000556a10cd1de0 RCX: 00007fde55d62e0a
> [ 1160.547828] RDX: 0000556a10cd2010 RSI: 0000556a10cd2090 RDI: 0000556a10ce2590
> [ 1160.547829] RBP: 0000000000000000 R08: 0000000000000000 R09: 00007fff11920d50
> [ 1160.547830] R10: 0000000000000000 R11: 0000000000000246 R12: 0000556a10ce2590
> [ 1160.547832] R13: 0000556a10cd2010 R14: 00007fde55eca264 R15: 0000556a10cd1ef8
> [ 1160.547834] </TASK>
> [ 1160.547835] ---[ end trace ]---
>
> John, can you please figure this one out, seeing as it's 10:30pm on
> Sunday night here?
I'll check it
thanks for your effort
I have known some ubsan red herrings
- I hope that this is not one of them ..
^ permalink raw reply
* Re: [PATCH v11 06/16] xfs: ignore HW which cannot atomic write a single block
From: John Garry @ 2025-05-05 5:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: brauner, djwong, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250505054310.GB20925@lst.de>
On 05/05/2025 06:43, Christoph Hellwig wrote:
> I think this subject line here is left from an earlier version and
> doesn't quite seem to summarize what this patch is doing now?
>
>> +extern int xfs_configure_buftarg(struct xfs_buftarg *, unsigned int);
>
> Please drop the extern and spell out the parameter name while you're at
> it.
I can fix all this, if Darrick does not beat me to it.
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
cheers
^ permalink raw reply
* Re: [PATCH v11 06/16] xfs: ignore HW which cannot atomic write a single block
From: Christoph Hellwig @ 2025-05-05 5:43 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250504085923.1895402-7-john.g.garry@oracle.com>
I think this subject line here is left from an earlier version and
doesn't quite seem to summarize what this patch is doing now?
> +extern int xfs_configure_buftarg(struct xfs_buftarg *, unsigned int);
Please drop the extern and spell out the parameter name while you're at
it.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v11 02/16] xfs: only call xfs_setsize_buftarg once per buffer target
From: Christoph Hellwig @ 2025-05-05 5:40 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250504085923.1895402-3-john.g.garry@oracle.com>
On Sun, May 04, 2025 at 08:59:09AM +0000, John Garry wrote:
> +static inline int xfs_buftarg_sync(struct xfs_buftarg *btp)
> +{
> + return sync_blockdev(btp->bt_bdev);
> +}
What is the point in having this wrapper?
> +/*
> + * Flush and invalidate all devices' pagecaches before reading any metadata
> + * because XFS doesn't use the bdev pagecache.
> + */
> +STATIC int
> +xfs_preflush_devices(
> + struct xfs_mount *mp)
> +{
> + int error;
> +
> + error = xfs_buftarg_sync(mp->m_ddev_targp);
> + if (error)
> + return error;
> +
> + if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
> + error = xfs_buftarg_sync(mp->m_ddev_targp);
> + if (error)
> + return error;
> + }
Why does this duplicate all the logic instead of being folded into
xfs_open_devices?
^ permalink raw reply
* Re: [PATCH v11 14/16] xfs: add xfs_calc_atomic_write_unit_max()
From: Darrick J. Wong @ 2025-05-05 5:25 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250504085923.1895402-15-john.g.garry@oracle.com>
On Sun, May 04, 2025 at 08:59:21AM +0000, John Garry wrote:
> Now that CoW-based atomic writes are supported, update the max size of an
> atomic write for the data device.
>
> The limit of a CoW-based atomic write will be the limit of the number of
> logitems which can fit into a single transaction.
>
> In addition, the max atomic write size needs to be aligned to the agsize.
> Limit the size of atomic writes to the greatest power-of-two factor of the
> agsize so that allocations for an atomic write will always be aligned
> compatibly with the alignment requirements of the storage.
>
> Function xfs_atomic_write_logitems() is added to find the limit the number
> of log items which can fit in a single transaction.
>
> Amend the max atomic write computation to create a new transaction
> reservation type, and compute the maximum size of an atomic write
> completion (in fsblocks) based on this new transaction reservation.
> Initially, tr_atomic_write is a clone of tr_itruncate, which provides a
> reasonable level of parallelism. In the next patch, we'll add a mount
> option so that sysadmins can configure their own limits.
>
> [djwong: use a new reservation type for atomic write ioends, refactor
> group limit calculations]
>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> [jpg: rounddown power-of-2 always]
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> fs/xfs/libxfs/xfs_trans_resv.c | 94 ++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_trans_resv.h | 2 +
> fs/xfs/xfs_mount.c | 81 +++++++++++++++++++++++++++++
> fs/xfs/xfs_mount.h | 6 +++
> fs/xfs/xfs_reflink.c | 16 ++++++
> fs/xfs/xfs_reflink.h | 2 +
> fs/xfs/xfs_trace.h | 60 ++++++++++++++++++++++
> 7 files changed, 261 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index a841432abf83..e73c09fbd24c 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -22,6 +22,12 @@
> #include "xfs_rtbitmap.h"
> #include "xfs_attr_item.h"
> #include "xfs_log.h"
> +#include "xfs_defer.h"
> +#include "xfs_bmap_item.h"
> +#include "xfs_extfree_item.h"
> +#include "xfs_rmap_item.h"
> +#include "xfs_refcount_item.h"
> +#include "xfs_trace.h"
>
> #define _ALLOC true
> #define _FREE false
> @@ -1394,3 +1400,91 @@ xfs_trans_resv_calc(
> */
> xfs_calc_default_atomic_ioend_reservation(mp, resp);
> }
> +
> +/*
> + * Return the per-extent and fixed transaction reservation sizes needed to
> + * complete an atomic write.
> + */
> +STATIC unsigned int
> +xfs_calc_atomic_write_ioend_geometry(
> + struct xfs_mount *mp,
> + unsigned int *step_size)
> +{
> + const unsigned int efi = xfs_efi_log_space(1);
> + const unsigned int efd = xfs_efd_log_space(1);
> + const unsigned int rui = xfs_rui_log_space(1);
> + const unsigned int rud = xfs_rud_log_space();
> + const unsigned int cui = xfs_cui_log_space(1);
> + const unsigned int cud = xfs_cud_log_space();
> + const unsigned int bui = xfs_bui_log_space(1);
> + const unsigned int bud = xfs_bud_log_space();
> +
> + /*
> + * Maximum overhead to complete an atomic write ioend in software:
> + * remove data fork extent + remove cow fork extent + map extent into
> + * data fork.
> + *
> + * tx0: Creates a BUI and a CUI and that's all it needs.
> + *
> + * tx1: Roll to finish the BUI. Need space for the BUD, an RUI, and
> + * enough space to relog the CUI (== CUI + CUD).
> + *
> + * tx2: Roll again to finish the RUI. Need space for the RUD and space
> + * to relog the CUI.
> + *
> + * tx3: Roll again, need space for the CUD and possibly a new EFI.
> + *
> + * tx4: Roll again, need space for an EFD.
> + *
> + * If the extent referenced by the pair of BUI/CUI items is not the one
> + * being currently processed, then we need to reserve space to relog
> + * both items.
> + */
> + const unsigned int tx0 = bui + cui;
> + const unsigned int tx1 = bud + rui + cui + cud;
> + const unsigned int tx2 = rud + cui + cud;
> + const unsigned int tx3 = cud + efi;
> + const unsigned int tx4 = efd;
> + const unsigned int relog = bui + bud + cui + cud;
> +
> + const unsigned int per_intent = max(max3(tx0, tx1, tx2),
> + max3(tx3, tx4, relog));
> +
> + /* Overhead to finish one step of each intent item type */
> + const unsigned int f1 = xfs_calc_finish_efi_reservation(mp, 1);
> + const unsigned int f2 = xfs_calc_finish_rui_reservation(mp, 1);
> + const unsigned int f3 = xfs_calc_finish_cui_reservation(mp, 1);
> + const unsigned int f4 = xfs_calc_finish_bui_reservation(mp, 1);
> +
> + /* We only finish one item per transaction in a chain */
> + *step_size = max(f4, max3(f1, f2, f3));
> +
> + return per_intent;
> +}
> +
> +/*
> + * Compute the maximum size (in fsblocks) of atomic writes that we can complete
> + * given the existing log reservations.
> + */
> +xfs_extlen_t
> +xfs_calc_max_atomic_write_fsblocks(
> + struct xfs_mount *mp)
> +{
> + const struct xfs_trans_res *resv = &M_RES(mp)->tr_atomic_ioend;
> + unsigned int per_intent = 0;
> + unsigned int step_size = 0;
> + unsigned int ret = 0;
> +
> + if (resv->tr_logres > 0) {
> + per_intent = xfs_calc_atomic_write_ioend_geometry(mp,
> + &step_size);
> +
> + if (resv->tr_logres >= step_size)
> + ret = (resv->tr_logres - step_size) / per_intent;
> + }
> +
> + trace_xfs_calc_max_atomic_write_fsblocks(mp, per_intent, step_size,
> + resv->tr_logres, ret);
> +
> + return ret;
> +}
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> index 670045d417a6..a6d303b83688 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.h
> +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> @@ -121,4 +121,6 @@ unsigned int xfs_calc_itruncate_reservation_minlogsize(struct xfs_mount *mp);
> unsigned int xfs_calc_write_reservation_minlogsize(struct xfs_mount *mp);
> unsigned int xfs_calc_qm_dqalloc_reservation_minlogsize(struct xfs_mount *mp);
>
> +xfs_extlen_t xfs_calc_max_atomic_write_fsblocks(struct xfs_mount *mp);
> +
> #endif /* __XFS_TRANS_RESV_H__ */
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 00b53f479ece..9c40914afabd 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -666,6 +666,80 @@ xfs_agbtree_compute_maxlevels(
> mp->m_agbtree_maxlevels = max(levels, mp->m_refc_maxlevels);
> }
>
> +/* Maximum atomic write IO size that the kernel allows. */
> +static inline xfs_extlen_t xfs_calc_atomic_write_max(struct xfs_mount *mp)
> +{
> + return rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
> +}
> +
> +static inline unsigned int max_pow_of_two_factor(const unsigned int nr)
> +{
> + return 1 << (ffs(nr) - 1);
> +}
> +
> +/*
> + * If the data device advertises atomic write support, limit the size of data
> + * device atomic writes to the greatest power-of-two factor of the AG size so
> + * that every atomic write unit aligns with the start of every AG. This is
> + * required so that the per-AG allocations for an atomic write will always be
> + * aligned compatibly with the alignment requirements of the storage.
> + *
> + * If the data device doesn't advertise atomic writes, then there are no
> + * alignment restrictions and the largest out-of-place write we can do
> + * ourselves is the number of blocks that user files can allocate from any AG.
> + */
> +static inline xfs_extlen_t xfs_calc_perag_awu_max(struct xfs_mount *mp)
> +{
> + if (mp->m_ddev_targp->bt_bdev_awu_min > 0)
> + return max_pow_of_two_factor(mp->m_sb.sb_agblocks);
> + return rounddown_pow_of_two(mp->m_ag_max_usable);
> +}
> +
> +/*
> + * Reflink on the realtime device requires rtgroups, and atomic writes require
> + * reflink.
> + *
> + * If the realtime device advertises atomic write support, limit the size of
> + * data device atomic writes to the greatest power-of-two factor of the rtgroup
> + * size so that every atomic write unit aligns with the start of every rtgroup.
> + * This is required so that the per-rtgroup allocations for an atomic write
> + * will always be aligned compatibly with the alignment requirements of the
> + * storage.
> + *
> + * If the rt device doesn't advertise atomic writes, then there are no
> + * alignment restrictions and the largest out-of-place write we can do
> + * ourselves is the number of blocks that user files can allocate from any
> + * rtgroup.
> + */
> +static inline xfs_extlen_t xfs_calc_rtgroup_awu_max(struct xfs_mount *mp)
> +{
> + struct xfs_groups *rgs = &mp->m_groups[XG_TYPE_RTG];
> +
> + if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_bdev_awu_min > 0)
> + return max_pow_of_two_factor(rgs->blocks);
> + return rounddown_pow_of_two(rgs->blocks);
Ok so I even attached the reply to the WRONG VERSION. Something in
these changes cause xfs/289 to barf up this UBSAN warning, even on a
realtime + rtgroups volume:
[ 1160.539004] ------------[ cut here ]------------
[ 1160.540701] UBSAN: shift-out-of-bounds in /storage/home/djwong/cdev/work/linux-djw/include/linux/log2.h:67:13
[ 1160.544597] shift exponent 4294967295 is too large for 64-bit type 'long unsigned int'
[ 1160.547038] CPU: 3 UID: 0 PID: 288421 Comm: mount Not tainted 6.15.0-rc5-djwx #rc5 PREEMPT(lazy) 6f606c17703b80ffff7378e7041918eca24b3e68
[ 1160.547045] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-4.module+el8.8.0+21164+ed375313 04/01/2014
[ 1160.547047] Call Trace:
[ 1160.547049] <TASK>
[ 1160.547051] dump_stack_lvl+0x4f/0x60
[ 1160.547060] __ubsan_handle_shift_out_of_bounds+0x1bc/0x380
[ 1160.547066] xfs_set_max_atomic_write_opt.cold+0x22d/0x252 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
[ 1160.547249] xfs_mountfs+0xa5c/0xb50 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
[ 1160.547434] xfs_fs_fill_super+0x7eb/0xb30 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
[ 1160.547616] ? xfs_open_devices+0x240/0x240 [xfs 1f657532c3dee9b1d567597a31645929273d3283]
[ 1160.547797] get_tree_bdev_flags+0x132/0x1d0
[ 1160.547801] vfs_get_tree+0x17/0xa0
[ 1160.547803] path_mount+0x720/0xa80
[ 1160.547807] __x64_sys_mount+0x10c/0x140
[ 1160.547810] do_syscall_64+0x47/0x100
[ 1160.547814] entry_SYSCALL_64_after_hwframe+0x4b/0x53
[ 1160.547817] RIP: 0033:0x7fde55d62e0a
[ 1160.547820] Code: 48 8b 0d f9 7f 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c6 7f 0c 00 f7 d8 64 89 01 48
[ 1160.547823] RSP: 002b:00007fff11920ce8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
[ 1160.547826] RAX: ffffffffffffffda RBX: 0000556a10cd1de0 RCX: 00007fde55d62e0a
[ 1160.547828] RDX: 0000556a10cd2010 RSI: 0000556a10cd2090 RDI: 0000556a10ce2590
[ 1160.547829] RBP: 0000000000000000 R08: 0000000000000000 R09: 00007fff11920d50
[ 1160.547830] R10: 0000000000000000 R11: 0000000000000246 R12: 0000556a10ce2590
[ 1160.547832] R13: 0000556a10cd2010 R14: 00007fde55eca264 R15: 0000556a10cd1ef8
[ 1160.547834] </TASK>
[ 1160.547835] ---[ end trace ]---
John, can you please figure this one out, seeing as it's 10:30pm on
Sunday night here?
--D
> +}
> +
> +/* Compute the maximum atomic write unit size for each section. */
> +static inline void
> +xfs_calc_atomic_write_unit_max(
> + struct xfs_mount *mp)
> +{
> + struct xfs_groups *ags = &mp->m_groups[XG_TYPE_AG];
> + struct xfs_groups *rgs = &mp->m_groups[XG_TYPE_RTG];
> +
> + const xfs_extlen_t max_write = xfs_calc_atomic_write_max(mp);
> + const xfs_extlen_t max_ioend = xfs_reflink_max_atomic_cow(mp);
> + const xfs_extlen_t max_agsize = xfs_calc_perag_awu_max(mp);
> + const xfs_extlen_t max_rgsize = xfs_calc_rtgroup_awu_max(mp);
> +
> + ags->awu_max = min3(max_write, max_ioend, max_agsize);
> + rgs->awu_max = min3(max_write, max_ioend, max_rgsize);
> +
> + trace_xfs_calc_atomic_write_unit_max(mp, max_write, max_ioend,
> + max_agsize, max_rgsize);
> +}
> +
> /* Compute maximum possible height for realtime btree types for this fs. */
> static inline void
> xfs_rtbtree_compute_maxlevels(
> @@ -1082,6 +1156,13 @@ xfs_mountfs(
> xfs_zone_gc_start(mp);
> }
>
> + /*
> + * Pre-calculate atomic write unit max. This involves computations
> + * derived from transaction reservations, so we must do this after the
> + * log is fully initialized.
> + */
> + xfs_calc_atomic_write_unit_max(mp);
> +
> return 0;
>
> out_agresv:
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index e67bc3e91f98..e2abf31438e0 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -119,6 +119,12 @@ struct xfs_groups {
> * SMR hard drives.
> */
> xfs_fsblock_t start_fsb;
> +
> + /*
> + * Maximum length of an atomic write for files stored in this
> + * collection of allocation groups, in fsblocks.
> + */
> + xfs_extlen_t awu_max;
> };
>
> struct xfs_freecounter {
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 218dee76768b..ad3bcb76d805 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1040,6 +1040,22 @@ xfs_reflink_end_atomic_cow(
> return error;
> }
>
> +/* Compute the largest atomic write that we can complete through software. */
> +xfs_extlen_t
> +xfs_reflink_max_atomic_cow(
> + struct xfs_mount *mp)
> +{
> + /* We cannot do any atomic writes without out of place writes. */
> + if (!xfs_can_sw_atomic_write(mp))
> + return 0;
> +
> + /*
> + * Atomic write limits must always be a power-of-2, according to
> + * generic_atomic_write_valid.
> + */
> + return rounddown_pow_of_two(xfs_calc_max_atomic_write_fsblocks(mp));
> +}
> +
> /*
> * Free all CoW staging blocks that are still referenced by the ondisk refcount
> * metadata. The ondisk metadata does not track which inode created the
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 412e9b6f2082..36cda724da89 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -68,4 +68,6 @@ extern int xfs_reflink_update_dest(struct xfs_inode *dest, xfs_off_t newlen,
>
> bool xfs_reflink_supports_rextsize(struct xfs_mount *mp, unsigned int rextsize);
>
> +xfs_extlen_t xfs_reflink_max_atomic_cow(struct xfs_mount *mp);
> +
> #endif /* __XFS_REFLINK_H */
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 9554578c6da4..d5ae00f8e04c 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -170,6 +170,66 @@ DEFINE_ATTR_LIST_EVENT(xfs_attr_list_notfound);
> DEFINE_ATTR_LIST_EVENT(xfs_attr_leaf_list);
> DEFINE_ATTR_LIST_EVENT(xfs_attr_node_list);
>
> +TRACE_EVENT(xfs_calc_atomic_write_unit_max,
> + TP_PROTO(struct xfs_mount *mp, unsigned int max_write,
> + unsigned int max_ioend, unsigned int max_agsize,
> + unsigned int max_rgsize),
> + TP_ARGS(mp, max_write, max_ioend, max_agsize, max_rgsize),
> + TP_STRUCT__entry(
> + __field(dev_t, dev)
> + __field(unsigned int, max_write)
> + __field(unsigned int, max_ioend)
> + __field(unsigned int, max_agsize)
> + __field(unsigned int, max_rgsize)
> + __field(unsigned int, data_awu_max)
> + __field(unsigned int, rt_awu_max)
> + ),
> + TP_fast_assign(
> + __entry->dev = mp->m_super->s_dev;
> + __entry->max_write = max_write;
> + __entry->max_ioend = max_ioend;
> + __entry->max_agsize = max_agsize;
> + __entry->max_rgsize = max_rgsize;
> + __entry->data_awu_max = mp->m_groups[XG_TYPE_AG].awu_max;
> + __entry->rt_awu_max = mp->m_groups[XG_TYPE_RTG].awu_max;
> + ),
> + TP_printk("dev %d:%d max_write %u max_ioend %u max_agsize %u max_rgsize %u data_awu_max %u rt_awu_max %u",
> + MAJOR(__entry->dev), MINOR(__entry->dev),
> + __entry->max_write,
> + __entry->max_ioend,
> + __entry->max_agsize,
> + __entry->max_rgsize,
> + __entry->data_awu_max,
> + __entry->rt_awu_max)
> +);
> +
> +TRACE_EVENT(xfs_calc_max_atomic_write_fsblocks,
> + TP_PROTO(struct xfs_mount *mp, unsigned int per_intent,
> + unsigned int step_size, unsigned int logres,
> + unsigned int blockcount),
> + TP_ARGS(mp, per_intent, step_size, logres, blockcount),
> + TP_STRUCT__entry(
> + __field(dev_t, dev)
> + __field(unsigned int, per_intent)
> + __field(unsigned int, step_size)
> + __field(unsigned int, logres)
> + __field(unsigned int, blockcount)
> + ),
> + TP_fast_assign(
> + __entry->dev = mp->m_super->s_dev;
> + __entry->per_intent = per_intent;
> + __entry->step_size = step_size;
> + __entry->logres = logres;
> + __entry->blockcount = blockcount;
> + ),
> + TP_printk("dev %d:%d per_intent %u step_size %u logres %u blockcount %u",
> + MAJOR(__entry->dev), MINOR(__entry->dev),
> + __entry->per_intent,
> + __entry->step_size,
> + __entry->logres,
> + __entry->blockcount)
> +);
> +
> TRACE_EVENT(xlog_intent_recovery_failed,
> TP_PROTO(struct xfs_mount *mp, const struct xfs_defer_op_type *ops,
> int error),
> --
> 2.31.1
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox