misc cleanups v2

misc cleanups v2

[Christoph Hellwig]

Hi all,

this series has a bunch of cleanups, mostly around the mount code and
triggered by various recent changes in the area.

Changes since v1:
 - keep the bdev_validate_blocksize call
 - add a new xfs_group_type_buftarg helper
 - make the buftarg awu names even shorter
 - avoid an unused variable warning for non-DEBUG builds

Diffstat:
 xfs_buf.c            |   31 +++++-----------
 xfs_buf.h            |   23 +-----------
 xfs_buf_mem.c        |    2 -
 xfs_discard.c        |   29 +++------------
 xfs_file.c           |    2 -
 xfs_inode.h          |    2 -
 xfs_iomap.c          |    2 -
 xfs_iops.c           |    2 -
 xfs_mount.c          |   97 ++++++++++++++++++++-------------------------------
 xfs_mount.h          |   17 ++++++++
 xfs_notify_failure.c |    3 -
 xfs_trace.h          |   31 +++++++---------
 12 files changed, 95 insertions(+), 146 deletions(-)

[PATCH 1/7] xfs: clean up the initial read logic in xfs_readsb

[Christoph Hellwig]

The initial sb read is always for a device logical block size
buffer.  The device logical block size is provided in the
bt_logical_sectorsize in struct buftarg, so use that instead of the
confusingly named xfs_getsize_buftarg buffer that reads it from the bdev.

Update the comments surrounding the code to better describe what is going
on.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.h   |  1 -
 fs/xfs/xfs_mount.c | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 15fc56948346..73a9686110e8 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -375,7 +375,6 @@ extern void xfs_buftarg_wait(struct xfs_buftarg *);
 extern void xfs_buftarg_drain(struct xfs_buftarg *);
 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)
 
 int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 29276fe60df9..047100b080aa 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -171,19 +171,16 @@ xfs_readsb(
 	ASSERT(mp->m_ddev_targp != NULL);
 
 	/*
-	 * For the initial read, we must guess at the sector
-	 * size based on the block device.  It's enough to
-	 * get the sb_sectsize out of the superblock and
-	 * then reread with the proper length.
-	 * We don't verify it yet, because it may not be complete.
+	 * In the first pass, use the device sector size to just read enough
+	 * of the superblock to extract the XFS sector size.
+	 *
+	 * The device sector size must be smaller than or equal to the XFS
+	 * sector size and thus we can always read the superblock.  Once we know
+	 * the XFS sector size, re-read it and run the buffer verifier.
 	 */
-	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
+	sector_size = mp->m_ddev_targp->bt_logical_sectorsize;
 	buf_ops = NULL;
 
-	/*
-	 * Allocate a (locked) buffer to hold the superblock. This will be kept
-	 * around at all times to optimize access to the superblock.
-	 */
 reread:
 	error = xfs_buf_read_uncached(mp->m_ddev_targp, XFS_SB_DADDR,
 				      BTOBB(sector_size), &bp, buf_ops);
@@ -247,6 +244,10 @@ xfs_readsb(
 	/* no need to be quiet anymore, so reset the buf ops */
 	bp->b_ops = &xfs_sb_buf_ops;
 
+	/*
+	 * Keep a pointer of the sb buffer around instead of caching it in the
+	 * buffer cache because we access it frequently.
+	 */
 	mp->m_sb_bp = bp;
 	xfs_buf_unlock(bp);
 	return 0;
-- 
2.47.2

Re: [PATCH 1/7] xfs: clean up the initial read logic in xfs_readsb

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 12:40:35PM +0200, Christoph Hellwig wrote:
> The initial sb read is always for a device logical block size
> buffer.  The device logical block size is provided in the
> bt_logical_sectorsize in struct buftarg, so use that instead of the
> confusingly named xfs_getsize_buftarg buffer that reads it from the bdev.
> 
> Update the comments surrounding the code to better describe what is going
> on.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.h   |  1 -
>  fs/xfs/xfs_mount.c | 21 +++++++++++----------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 15fc56948346..73a9686110e8 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -375,7 +375,6 @@ extern void xfs_buftarg_wait(struct xfs_buftarg *);
>  extern void xfs_buftarg_drain(struct xfs_buftarg *);
>  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)
>  
>  int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 29276fe60df9..047100b080aa 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -171,19 +171,16 @@ xfs_readsb(
>  	ASSERT(mp->m_ddev_targp != NULL);
>  
>  	/*
> -	 * For the initial read, we must guess at the sector
> -	 * size based on the block device.  It's enough to
> -	 * get the sb_sectsize out of the superblock and
> -	 * then reread with the proper length.
> -	 * We don't verify it yet, because it may not be complete.
> +	 * In the first pass, use the device sector size to just read enough
> +	 * of the superblock to extract the XFS sector size.
> +	 *
> +	 * The device sector size must be smaller than or equal to the XFS
> +	 * sector size and thus we can always read the superblock.  Once we know
> +	 * the XFS sector size, re-read it and run the buffer verifier.
>  	 */
> -	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
> +	sector_size = mp->m_ddev_targp->bt_logical_sectorsize;
>  	buf_ops = NULL;
>  
> -	/*
> -	 * Allocate a (locked) buffer to hold the superblock. This will be kept
> -	 * around at all times to optimize access to the superblock.
> -	 */
>  reread:
>  	error = xfs_buf_read_uncached(mp->m_ddev_targp, XFS_SB_DADDR,
>  				      BTOBB(sector_size), &bp, buf_ops);
> @@ -247,6 +244,10 @@ xfs_readsb(
>  	/* no need to be quiet anymore, so reset the buf ops */
>  	bp->b_ops = &xfs_sb_buf_ops;
>  
> +	/*
> +	 * Keep a pointer of the sb buffer around instead of caching it in the
> +	 * buffer cache because we access it frequently.
> +	 */
>  	mp->m_sb_bp = bp;
>  	xfs_buf_unlock(bp);
>  	return 0;
> -- 
> 2.47.2
> 
> 

[PATCH 2/7] xfs: remove the call to sync_blockdev in xfs_configure_buftarg

[Christoph Hellwig]

This extra call is no needed as xfs_alloc_buftarg already calls
sync_blockdev.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ba5bd6031ece..7a05310da895 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1744,8 +1744,7 @@ xfs_configure_buftarg(
 	 */
 	if (bdev_can_atomic_write(btp->bt_bdev))
 		xfs_configure_buftarg_atomic_writes(btp);
-
-	return sync_blockdev(btp->bt_bdev);
+	return 0;
 }
 
 int
-- 
2.47.2

Re: [PATCH 2/7] xfs: remove the call to sync_blockdev in xfs_configure_buftarg

[John Garry]

On 01/07/2025 11:40, Christoph Hellwig wrote:
> This extra call is no needed as xfs_alloc_buftarg already calls

not needed

> sync_blockdev.
> 
> Signed-off-by: Christoph Hellwig<hch@lst.de>

seems fine, so FWIW:

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH 2/7] xfs: remove the call to sync_blockdev in xfs_configure_buftarg

[John Garry]

On 01/07/2025 11:40, Christoph Hellwig wrote:
> This extra call is no needed as xfs_alloc_buftarg already calls
> sync_blockdev.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/xfs/xfs_buf.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ba5bd6031ece..7a05310da895 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1744,8 +1744,7 @@ xfs_configure_buftarg(
>   	 */
>   	if (bdev_can_atomic_write(btp->bt_bdev))
>   		xfs_configure_buftarg_atomic_writes(btp);
> -
> -	return sync_blockdev(btp->bt_bdev);
> +	return 0;
>   }
>   
>   int

BTW, the comment would need to be removed as well:

	/*
	 * Flush the block device pagecache so our bios see anything dirtied
	 * before mount.
	 */
	if (bdev_can_atomic_write(btp->bt_bdev))
		xfs_configure_buftarg_atomic_writes(btp);

	return sync_blockdev(btp->bt_bdev);
}

This all seems to have come from 6e7d71b3a0, which is a strange 
resolution to me.

Re: [PATCH 2/7] xfs: remove the call to sync_blockdev in xfs_configure_buftarg

[Christoph Hellwig]

On Tue, Jul 01, 2025 at 12:03:53PM +0100, John Garry wrote:
> BTW, the comment would need to be removed as well:
>
> 	/*
> 	 * Flush the block device pagecache so our bios see anything dirtied
> 	 * before mount.
> 	 */
> 	if (bdev_can_atomic_write(btp->bt_bdev))
> 		xfs_configure_buftarg_atomic_writes(btp);
>
> 	return sync_blockdev(btp->bt_bdev);
> }
>
> This all seems to have come from 6e7d71b3a0, which is a strange resolution 
> to me.

Yes, including having the comment in the wrong place.

[PATCH 3/7] xfs: add a xfs_group_type_buftarg helper

[Christoph Hellwig]

Generalize the xfs_group_type helper in the discard code to return a buftarg
and move it to xfs_mount.h, and use the result in xfs_dax_notify_dev_failure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_discard.c        | 29 +++++++----------------------
 fs/xfs/xfs_mount.h          | 17 +++++++++++++++++
 fs/xfs/xfs_notify_failure.c |  3 +--
 3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 94d0873bcd62..603d51365645 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -103,24 +103,6 @@ xfs_discard_endio(
 	bio_put(bio);
 }
 
-static inline struct block_device *
-xfs_group_bdev(
-	const struct xfs_group	*xg)
-{
-	struct xfs_mount	*mp = xg->xg_mount;
-
-	switch (xg->xg_type) {
-	case XG_TYPE_AG:
-		return mp->m_ddev_targp->bt_bdev;
-	case XG_TYPE_RTG:
-		return mp->m_rtdev_targp->bt_bdev;
-	default:
-		ASSERT(0);
-		break;
-	}
-	return NULL;
-}
-
 /*
  * Walk the discard list and issue discards on all the busy extents in the
  * list. We plug and chain the bios so that we only need a single completion
@@ -138,11 +120,14 @@ xfs_discard_extents(
 
 	blk_start_plug(&plug);
 	list_for_each_entry(busyp, &extents->extent_list, list) {
-		trace_xfs_discard_extent(busyp->group, busyp->bno,
-				busyp->length);
+		struct xfs_group	*xg = busyp->group;
+		struct xfs_buftarg	*btp =
+			xfs_group_type_buftarg(xg->xg_mount, xg->xg_type);
+
+		trace_xfs_discard_extent(xg, busyp->bno, busyp->length);
 
-		error = __blkdev_issue_discard(xfs_group_bdev(busyp->group),
-				xfs_gbno_to_daddr(busyp->group, busyp->bno),
+		error = __blkdev_issue_discard(btp->bt_bdev,
+				xfs_gbno_to_daddr(xg, busyp->bno),
 				XFS_FSB_TO_BB(mp, busyp->length),
 				GFP_KERNEL, &bio);
 		if (error && error != -EOPNOTSUPP) {
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index d85084f9f317..97de44c32272 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -802,4 +802,21 @@ static inline void xfs_mod_sb_delalloc(struct xfs_mount *mp, int64_t delta)
 int xfs_set_max_atomic_write_opt(struct xfs_mount *mp,
 		unsigned long long new_max_bytes);
 
+static inline struct xfs_buftarg *
+xfs_group_type_buftarg(
+	struct xfs_mount	*mp,
+	enum xfs_group_type	type)
+{
+	switch (type) {
+	case XG_TYPE_AG:
+		return mp->m_ddev_targp;
+	case XG_TYPE_RTG:
+		return mp->m_rtdev_targp;
+	default:
+		ASSERT(0);
+		break;
+	}
+	return NULL;
+}
+
 #endif	/* __XFS_MOUNT_H__ */
diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index 3545dc1d953c..42e9c72b85c0 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -253,8 +253,7 @@ xfs_dax_notify_dev_failure(
 		return -EOPNOTSUPP;
 	}
 
-	error = xfs_dax_translate_range(type == XG_TYPE_RTG ?
-			mp->m_rtdev_targp : mp->m_ddev_targp,
+	error = xfs_dax_translate_range(xfs_group_type_buftarg(mp, type),
 			offset, len, &daddr, &bblen);
 	if (error)
 		return error;
-- 
2.47.2

Re: [PATCH 3/7] xfs: add a xfs_group_type_buftarg helper

[John Garry]

On 01/07/2025 11:40, Christoph Hellwig wrote:
> Generalize the xfs_group_type helper in the discard code to return a buftarg
> and move it to xfs_mount.h, and use the result in xfs_dax_notify_dev_failure.
> 
> Signed-off-by: Christoph Hellwig<hch@lst.de>

looks ok, so FWIW:

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH 3/7] xfs: add a xfs_group_type_buftarg helper

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 12:40:37PM +0200, Christoph Hellwig wrote:
> Generalize the xfs_group_type helper in the discard code to return a buftarg
> and move it to xfs_mount.h, and use the result in xfs_dax_notify_dev_failure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_discard.c        | 29 +++++++----------------------
>  fs/xfs/xfs_mount.h          | 17 +++++++++++++++++
>  fs/xfs/xfs_notify_failure.c |  3 +--
>  3 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 94d0873bcd62..603d51365645 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -103,24 +103,6 @@ xfs_discard_endio(
>  	bio_put(bio);
>  }
>  
> -static inline struct block_device *
> -xfs_group_bdev(
> -	const struct xfs_group	*xg)
> -{
> -	struct xfs_mount	*mp = xg->xg_mount;
> -
> -	switch (xg->xg_type) {
> -	case XG_TYPE_AG:
> -		return mp->m_ddev_targp->bt_bdev;
> -	case XG_TYPE_RTG:
> -		return mp->m_rtdev_targp->bt_bdev;
> -	default:
> -		ASSERT(0);
> -		break;
> -	}
> -	return NULL;
> -}
> -
>  /*
>   * Walk the discard list and issue discards on all the busy extents in the
>   * list. We plug and chain the bios so that we only need a single completion
> @@ -138,11 +120,14 @@ xfs_discard_extents(
>  
>  	blk_start_plug(&plug);
>  	list_for_each_entry(busyp, &extents->extent_list, list) {
> -		trace_xfs_discard_extent(busyp->group, busyp->bno,
> -				busyp->length);
> +		struct xfs_group	*xg = busyp->group;
> +		struct xfs_buftarg	*btp =
> +			xfs_group_type_buftarg(xg->xg_mount, xg->xg_type);
> +
> +		trace_xfs_discard_extent(xg, busyp->bno, busyp->length);
>  
> -		error = __blkdev_issue_discard(xfs_group_bdev(busyp->group),
> -				xfs_gbno_to_daddr(busyp->group, busyp->bno),
> +		error = __blkdev_issue_discard(btp->bt_bdev,
> +				xfs_gbno_to_daddr(xg, busyp->bno),
>  				XFS_FSB_TO_BB(mp, busyp->length),
>  				GFP_KERNEL, &bio);
>  		if (error && error != -EOPNOTSUPP) {
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index d85084f9f317..97de44c32272 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -802,4 +802,21 @@ static inline void xfs_mod_sb_delalloc(struct xfs_mount *mp, int64_t delta)
>  int xfs_set_max_atomic_write_opt(struct xfs_mount *mp,
>  		unsigned long long new_max_bytes);
>  
> +static inline struct xfs_buftarg *
> +xfs_group_type_buftarg(
> +	struct xfs_mount	*mp,
> +	enum xfs_group_type	type)
> +{
> +	switch (type) {
> +	case XG_TYPE_AG:
> +		return mp->m_ddev_targp;
> +	case XG_TYPE_RTG:
> +		return mp->m_rtdev_targp;
> +	default:
> +		ASSERT(0);
> +		break;
> +	}
> +	return NULL;
> +}
> +
>  #endif	/* __XFS_MOUNT_H__ */
> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> index 3545dc1d953c..42e9c72b85c0 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -253,8 +253,7 @@ xfs_dax_notify_dev_failure(
>  		return -EOPNOTSUPP;
>  	}
>  
> -	error = xfs_dax_translate_range(type == XG_TYPE_RTG ?
> -			mp->m_rtdev_targp : mp->m_ddev_targp,
> +	error = xfs_dax_translate_range(xfs_group_type_buftarg(mp, type),
>  			offset, len, &daddr, &bblen);
>  	if (error)
>  		return error;
> -- 
> 2.47.2
> 
> 

[PATCH 4/7] xfs: refactor xfs_calc_atomic_write_unit_max

[Christoph Hellwig]

This function and the helpers used by it duplicate the same logic for AGs
and RTGs.  Use the xfs_group_type enum to unify both variants.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_mount.c | 76 +++++++++++++++++-----------------------------
 fs/xfs/xfs_trace.h | 31 +++++++++----------
 2 files changed, 42 insertions(+), 65 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 047100b080aa..99fbb22bad4c 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -679,68 +679,46 @@ static inline unsigned int max_pow_of_two_factor(const unsigned int nr)
 }
 
 /*
- * 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
+ * If the underlying device advertises atomic write support, limit the size of
+ * atomic writes to the greatest power-of-two factor of the group size so
+ * that every atomic write unit aligns with the start of every group.  This is
+ * required so that the 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.
+ * If the 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 group.
  */
-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)
+static xfs_extlen_t
+xfs_calc_group_awu_max(
+	struct xfs_mount	*mp,
+	enum xfs_group_type	type)
 {
-	struct xfs_groups	*rgs = &mp->m_groups[XG_TYPE_RTG];
+	struct xfs_groups	*g = &mp->m_groups[type];
+	struct xfs_buftarg	*btp = xfs_group_type_buftarg(mp, type);
 
-	if (rgs->blocks == 0)
+	if (g->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);
+	if (btp && btp->bt_bdev_awu_min > 0)
+		return max_pow_of_two_factor(g->blocks);
+	return rounddown_pow_of_two(g->blocks);
 }
 
 /* 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_mount	*mp,
+	enum xfs_group_type	type)
 {
-	struct xfs_groups	*ags = &mp->m_groups[XG_TYPE_AG];
-	struct xfs_groups	*rgs = &mp->m_groups[XG_TYPE_RTG];
+	struct xfs_groups	*g = &mp->m_groups[type];
 
 	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);
+	const xfs_extlen_t	max_gsize = xfs_calc_group_awu_max(mp, type);
 
-	trace_xfs_calc_atomic_write_unit_max(mp, max_write, max_ioend,
-			max_agsize, max_rgsize);
+	g->awu_max = min3(max_write, max_ioend, max_gsize);
+	trace_xfs_calc_atomic_write_unit_max(mp, type, max_write, max_ioend,
+			max_gsize, g->awu_max);
 }
 
 /*
@@ -758,7 +736,8 @@ xfs_set_max_atomic_write_opt(
 		max(mp->m_groups[XG_TYPE_AG].blocks,
 		    mp->m_groups[XG_TYPE_RTG].blocks);
 	const xfs_extlen_t	max_group_write =
-		max(xfs_calc_perag_awu_max(mp), xfs_calc_rtgroup_awu_max(mp));
+		max(xfs_calc_group_awu_max(mp, XG_TYPE_AG),
+		    xfs_calc_group_awu_max(mp, XG_TYPE_RTG));
 	int			error;
 
 	if (new_max_bytes == 0)
@@ -814,7 +793,8 @@ xfs_set_max_atomic_write_opt(
 		return error;
 	}
 
-	xfs_calc_atomic_write_unit_max(mp);
+	xfs_calc_atomic_write_unit_max(mp, XG_TYPE_AG);
+	xfs_calc_atomic_write_unit_max(mp, XG_TYPE_RTG);
 	mp->m_awu_max_bytes = new_max_bytes;
 	return 0;
 }
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index aae0d0ef84e0..6addebd764b0 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -171,36 +171,33 @@ 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_PROTO(struct xfs_mount *mp, enum xfs_group_type type,
+		 unsigned int max_write, unsigned int max_ioend,
+		 unsigned int max_gsize, unsigned int awu_max),
+	TP_ARGS(mp, type, max_write, max_ioend, max_gsize, awu_max),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
+		__field(enum xfs_group_type, type)
 		__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)
+		__field(unsigned int, max_gsize)
+		__field(unsigned int, awu_max)
 	),
 	TP_fast_assign(
 		__entry->dev = mp->m_super->s_dev;
+		__entry->type = type;
 		__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;
+		__entry->max_gsize = max_gsize;
+		__entry->awu_max = 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",
+	TP_printk("dev %d:%d %s max_write %u max_ioend %u max_gsize %u awu_max %u",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __print_symbolic(__entry->type, XG_TYPE_STRINGS),
 		  __entry->max_write,
 		  __entry->max_ioend,
-		  __entry->max_agsize,
-		  __entry->max_rgsize,
-		  __entry->data_awu_max,
-		  __entry->rt_awu_max)
+		  __entry->max_gsize,
+		  __entry->awu_max)
 );
 
 TRACE_EVENT(xfs_calc_max_atomic_write_fsblocks,
-- 
2.47.2

Re: [PATCH 4/7] xfs: refactor xfs_calc_atomic_write_unit_max

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 12:40:38PM +0200, Christoph Hellwig wrote:
> This function and the helpers used by it duplicate the same logic for AGs
> and RTGs.  Use the xfs_group_type enum to unify both variants.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: John Garry <john.g.garry@oracle.com>

LGTM,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_mount.c | 76 +++++++++++++++++-----------------------------
>  fs/xfs/xfs_trace.h | 31 +++++++++----------
>  2 files changed, 42 insertions(+), 65 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 047100b080aa..99fbb22bad4c 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -679,68 +679,46 @@ static inline unsigned int max_pow_of_two_factor(const unsigned int nr)
>  }
>  
>  /*
> - * 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
> + * If the underlying device advertises atomic write support, limit the size of
> + * atomic writes to the greatest power-of-two factor of the group size so
> + * that every atomic write unit aligns with the start of every group.  This is
> + * required so that the 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.
> + * If the 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 group.
>   */
> -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)
> +static xfs_extlen_t
> +xfs_calc_group_awu_max(
> +	struct xfs_mount	*mp,
> +	enum xfs_group_type	type)
>  {
> -	struct xfs_groups	*rgs = &mp->m_groups[XG_TYPE_RTG];
> +	struct xfs_groups	*g = &mp->m_groups[type];
> +	struct xfs_buftarg	*btp = xfs_group_type_buftarg(mp, type);
>  
> -	if (rgs->blocks == 0)
> +	if (g->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);
> +	if (btp && btp->bt_bdev_awu_min > 0)
> +		return max_pow_of_two_factor(g->blocks);
> +	return rounddown_pow_of_two(g->blocks);
>  }
>  
>  /* 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_mount	*mp,
> +	enum xfs_group_type	type)
>  {
> -	struct xfs_groups	*ags = &mp->m_groups[XG_TYPE_AG];
> -	struct xfs_groups	*rgs = &mp->m_groups[XG_TYPE_RTG];
> +	struct xfs_groups	*g = &mp->m_groups[type];
>  
>  	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);
> +	const xfs_extlen_t	max_gsize = xfs_calc_group_awu_max(mp, type);
>  
> -	trace_xfs_calc_atomic_write_unit_max(mp, max_write, max_ioend,
> -			max_agsize, max_rgsize);
> +	g->awu_max = min3(max_write, max_ioend, max_gsize);
> +	trace_xfs_calc_atomic_write_unit_max(mp, type, max_write, max_ioend,
> +			max_gsize, g->awu_max);
>  }
>  
>  /*
> @@ -758,7 +736,8 @@ xfs_set_max_atomic_write_opt(
>  		max(mp->m_groups[XG_TYPE_AG].blocks,
>  		    mp->m_groups[XG_TYPE_RTG].blocks);
>  	const xfs_extlen_t	max_group_write =
> -		max(xfs_calc_perag_awu_max(mp), xfs_calc_rtgroup_awu_max(mp));
> +		max(xfs_calc_group_awu_max(mp, XG_TYPE_AG),
> +		    xfs_calc_group_awu_max(mp, XG_TYPE_RTG));
>  	int			error;
>  
>  	if (new_max_bytes == 0)
> @@ -814,7 +793,8 @@ xfs_set_max_atomic_write_opt(
>  		return error;
>  	}
>  
> -	xfs_calc_atomic_write_unit_max(mp);
> +	xfs_calc_atomic_write_unit_max(mp, XG_TYPE_AG);
> +	xfs_calc_atomic_write_unit_max(mp, XG_TYPE_RTG);
>  	mp->m_awu_max_bytes = new_max_bytes;
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index aae0d0ef84e0..6addebd764b0 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -171,36 +171,33 @@ 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_PROTO(struct xfs_mount *mp, enum xfs_group_type type,
> +		 unsigned int max_write, unsigned int max_ioend,
> +		 unsigned int max_gsize, unsigned int awu_max),
> +	TP_ARGS(mp, type, max_write, max_ioend, max_gsize, awu_max),
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
> +		__field(enum xfs_group_type, type)
>  		__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)
> +		__field(unsigned int, max_gsize)
> +		__field(unsigned int, awu_max)
>  	),
>  	TP_fast_assign(
>  		__entry->dev = mp->m_super->s_dev;
> +		__entry->type = type;
>  		__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;
> +		__entry->max_gsize = max_gsize;
> +		__entry->awu_max = 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",
> +	TP_printk("dev %d:%d %s max_write %u max_ioend %u max_gsize %u awu_max %u",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
> +		  __print_symbolic(__entry->type, XG_TYPE_STRINGS),
>  		  __entry->max_write,
>  		  __entry->max_ioend,
> -		  __entry->max_agsize,
> -		  __entry->max_rgsize,
> -		  __entry->data_awu_max,
> -		  __entry->rt_awu_max)
> +		  __entry->max_gsize,
> +		  __entry->awu_max)
>  );
>  
>  TRACE_EVENT(xfs_calc_max_atomic_write_fsblocks,
> -- 
> 2.47.2
> 
> 

[PATCH 5/7] xfs: rename the bt_bdev_* buftarg fields

[Christoph Hellwig]

The extra bdev_ is weird, so drop it.  Also improve the comment to make
it clear these are the hardware limits.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c   | 4 ++--
 fs/xfs/xfs_buf.h   | 6 +++---
 fs/xfs/xfs_file.c  | 2 +-
 fs/xfs/xfs_inode.h | 2 +-
 fs/xfs/xfs_iomap.c | 2 +-
 fs/xfs/xfs_iops.c  | 2 +-
 fs/xfs/xfs_mount.c | 2 +-
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 7a05310da895..661f6c70e9d0 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1712,8 +1712,8 @@ xfs_configure_buftarg_atomic_writes(
 		max_bytes = 0;
 	}
 
-	btp->bt_bdev_awu_min = min_bytes;
-	btp->bt_bdev_awu_max = max_bytes;
+	btp->bt_awu_min = min_bytes;
+	btp->bt_awu_max = max_bytes;
 }
 
 /* Configure a buffer target that abstracts a block device. */
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 73a9686110e8..7987a6d64874 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -112,9 +112,9 @@ struct xfs_buftarg {
 	struct percpu_counter	bt_readahead_count;
 	struct ratelimit_state	bt_ioerror_rl;
 
-	/* Atomic write unit values, bytes */
-	unsigned int		bt_bdev_awu_min;
-	unsigned int		bt_bdev_awu_max;
+	/* Hardware atomic write unit values, bytes */
+	unsigned int		bt_awu_min;
+	unsigned int		bt_awu_max;
 
 	/* built-in cache, if we're not using the perag one */
 	struct xfs_buf_cache	bt_cache[];
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 48254a72071b..377fc9077781 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -752,7 +752,7 @@ xfs_file_dio_write_atomic(
 	 * HW offload should be faster, so try that first if it is already
 	 * known that the write length is not too large.
 	 */
-	if (ocount > xfs_inode_buftarg(ip)->bt_bdev_awu_max)
+	if (ocount > xfs_inode_buftarg(ip)->bt_awu_max)
 		dops = &xfs_atomic_write_cow_iomap_ops;
 	else
 		dops = &xfs_direct_write_iomap_ops;
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index d7e2b902ef5c..07fbdcc4cbf5 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -358,7 +358,7 @@ static inline bool xfs_inode_has_bigrtalloc(const struct xfs_inode *ip)
 
 static inline bool xfs_inode_can_hw_atomic_write(const struct xfs_inode *ip)
 {
-	return xfs_inode_buftarg(ip)->bt_bdev_awu_max > 0;
+	return xfs_inode_buftarg(ip)->bt_awu_max > 0;
 }
 
 /*
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index ff05e6b1b0bb..ec30b78bf5c4 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -827,7 +827,7 @@ xfs_bmap_hw_atomic_write_possible(
 	/*
 	 * The ->iomap_begin caller should ensure this, but check anyway.
 	 */
-	return len <= xfs_inode_buftarg(ip)->bt_bdev_awu_max;
+	return len <= xfs_inode_buftarg(ip)->bt_awu_max;
 }
 
 static int
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 8cddbb7c149b..01e597290eb5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -665,7 +665,7 @@ xfs_get_atomic_write_max_opt(
 	 * less than our out of place write limit, but we don't want to exceed
 	 * the awu_max.
 	 */
-	return min(awu_max, xfs_inode_buftarg(ip)->bt_bdev_awu_max);
+	return min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
 }
 
 static void
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 99fbb22bad4c..0b690bc119d7 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -699,7 +699,7 @@ xfs_calc_group_awu_max(
 
 	if (g->blocks == 0)
 		return 0;
-	if (btp && btp->bt_bdev_awu_min > 0)
+	if (btp && btp->bt_awu_min > 0)
 		return max_pow_of_two_factor(g->blocks);
 	return rounddown_pow_of_two(g->blocks);
 }
-- 
2.47.2

Re: [PATCH 5/7] xfs: rename the bt_bdev_* buftarg fields

[John Garry]

On 01/07/2025 11:40, Christoph Hellwig wrote:
> The extra bdev_ is weird, so drop it.  Also improve the comment to make
> it clear these are the hardware limits.
> 
> Signed-off-by: Christoph Hellwig<hch@lst.de>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH 5/7] xfs: rename the bt_bdev_* buftarg fields

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 12:40:39PM +0200, Christoph Hellwig wrote:
> The extra bdev_ is weird, so drop it.  Also improve the comment to make
> it clear these are the hardware limits.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks fine, thanks for the clarification in the xfs_buftarg definition

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c   | 4 ++--
>  fs/xfs/xfs_buf.h   | 6 +++---
>  fs/xfs/xfs_file.c  | 2 +-
>  fs/xfs/xfs_inode.h | 2 +-
>  fs/xfs/xfs_iomap.c | 2 +-
>  fs/xfs/xfs_iops.c  | 2 +-
>  fs/xfs/xfs_mount.c | 2 +-
>  7 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 7a05310da895..661f6c70e9d0 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1712,8 +1712,8 @@ xfs_configure_buftarg_atomic_writes(
>  		max_bytes = 0;
>  	}
>  
> -	btp->bt_bdev_awu_min = min_bytes;
> -	btp->bt_bdev_awu_max = max_bytes;
> +	btp->bt_awu_min = min_bytes;
> +	btp->bt_awu_max = max_bytes;
>  }
>  
>  /* Configure a buffer target that abstracts a block device. */
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 73a9686110e8..7987a6d64874 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -112,9 +112,9 @@ struct xfs_buftarg {
>  	struct percpu_counter	bt_readahead_count;
>  	struct ratelimit_state	bt_ioerror_rl;
>  
> -	/* Atomic write unit values, bytes */
> -	unsigned int		bt_bdev_awu_min;
> -	unsigned int		bt_bdev_awu_max;
> +	/* Hardware atomic write unit values, bytes */
> +	unsigned int		bt_awu_min;
> +	unsigned int		bt_awu_max;
>  
>  	/* built-in cache, if we're not using the perag one */
>  	struct xfs_buf_cache	bt_cache[];
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 48254a72071b..377fc9077781 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -752,7 +752,7 @@ xfs_file_dio_write_atomic(
>  	 * HW offload should be faster, so try that first if it is already
>  	 * known that the write length is not too large.
>  	 */
> -	if (ocount > xfs_inode_buftarg(ip)->bt_bdev_awu_max)
> +	if (ocount > xfs_inode_buftarg(ip)->bt_awu_max)
>  		dops = &xfs_atomic_write_cow_iomap_ops;
>  	else
>  		dops = &xfs_direct_write_iomap_ops;
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index d7e2b902ef5c..07fbdcc4cbf5 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -358,7 +358,7 @@ static inline bool xfs_inode_has_bigrtalloc(const struct xfs_inode *ip)
>  
>  static inline bool xfs_inode_can_hw_atomic_write(const struct xfs_inode *ip)
>  {
> -	return xfs_inode_buftarg(ip)->bt_bdev_awu_max > 0;
> +	return xfs_inode_buftarg(ip)->bt_awu_max > 0;
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index ff05e6b1b0bb..ec30b78bf5c4 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -827,7 +827,7 @@ xfs_bmap_hw_atomic_write_possible(
>  	/*
>  	 * The ->iomap_begin caller should ensure this, but check anyway.
>  	 */
> -	return len <= xfs_inode_buftarg(ip)->bt_bdev_awu_max;
> +	return len <= xfs_inode_buftarg(ip)->bt_awu_max;
>  }
>  
>  static int
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 8cddbb7c149b..01e597290eb5 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -665,7 +665,7 @@ xfs_get_atomic_write_max_opt(
>  	 * less than our out of place write limit, but we don't want to exceed
>  	 * the awu_max.
>  	 */
> -	return min(awu_max, xfs_inode_buftarg(ip)->bt_bdev_awu_max);
> +	return min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
>  }
>  
>  static void
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 99fbb22bad4c..0b690bc119d7 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -699,7 +699,7 @@ xfs_calc_group_awu_max(
>  
>  	if (g->blocks == 0)
>  		return 0;
> -	if (btp && btp->bt_bdev_awu_min > 0)
> +	if (btp && btp->bt_awu_min > 0)
>  		return max_pow_of_two_factor(g->blocks);
>  	return rounddown_pow_of_two(g->blocks);
>  }
> -- 
> 2.47.2
> 
> 

[PATCH 6/7] xfs: remove the bt_bdev_file buftarg field

[Christoph Hellwig]

And use bt_file for both bdev and shmem backed buftargs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 4 ++--
 fs/xfs/xfs_buf.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 661f6c70e9d0..b73da43f489c 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1683,7 +1683,7 @@ xfs_free_buftarg(
 	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
 	/* the main block device is closed by kill_block_super */
 	if (btp->bt_bdev != btp->bt_mount->m_super->s_bdev)
-		bdev_fput(btp->bt_bdev_file);
+		bdev_fput(btp->bt_file);
 	kfree(btp);
 }
 
@@ -1802,7 +1802,7 @@ xfs_alloc_buftarg(
 	btp = kzalloc(sizeof(*btp), GFP_KERNEL | __GFP_NOFAIL);
 
 	btp->bt_mount = mp;
-	btp->bt_bdev_file = bdev_file;
+	btp->bt_file = bdev_file;
 	btp->bt_bdev = file_bdev(bdev_file);
 	btp->bt_dev = btp->bt_bdev->bd_dev;
 	btp->bt_daxdev = fs_dax_get_by_bdev(btp->bt_bdev, &btp->bt_dax_part_off,
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 7987a6d64874..b269e115d9ac 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -94,7 +94,6 @@ void xfs_buf_cache_destroy(struct xfs_buf_cache *bch);
  */
 struct xfs_buftarg {
 	dev_t			bt_dev;
-	struct file		*bt_bdev_file;
 	struct block_device	*bt_bdev;
 	struct dax_device	*bt_daxdev;
 	struct file		*bt_file;
-- 
2.47.2

Re: [PATCH 6/7] xfs: remove the bt_bdev_file buftarg field

[John Garry]

On 01/07/2025 11:40, Christoph Hellwig wrote:
> And use bt_file for both bdev and shmem backed buftargs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

looks ok, so FWIW:

Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
>   fs/xfs/xfs_buf.c | 4 ++--
>   fs/xfs/xfs_buf.h | 1 -
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 661f6c70e9d0..b73da43f489c 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1683,7 +1683,7 @@ xfs_free_buftarg(
>   	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
>   	/* the main block device is closed by kill_block_super */
>   	if (btp->bt_bdev != btp->bt_mount->m_super->s_bdev)
> -		bdev_fput(btp->bt_bdev_file);
> +		bdev_fput(btp->bt_file);
>   	kfree(btp);
>   }
>   
> @@ -1802,7 +1802,7 @@ xfs_alloc_buftarg(
>   	btp = kzalloc(sizeof(*btp), GFP_KERNEL | __GFP_NOFAIL);
>   
>   	btp->bt_mount = mp;
> -	btp->bt_bdev_file = bdev_file;
> +	btp->bt_file = bdev_file;
>   	btp->bt_bdev = file_bdev(bdev_file);
>   	btp->bt_dev = btp->bt_bdev->bd_dev;
>   	btp->bt_daxdev = fs_dax_get_by_bdev(btp->bt_bdev, &btp->bt_dax_part_off,
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 7987a6d64874..b269e115d9ac 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -94,7 +94,6 @@ void xfs_buf_cache_destroy(struct xfs_buf_cache *bch);
>    */
>   struct xfs_buftarg {
>   	dev_t			bt_dev;
> -	struct file		*bt_bdev_file;
>   	struct block_device	*bt_bdev;
>   	struct dax_device	*bt_daxdev;
>   	struct file		*bt_file;

Re: [PATCH 6/7] xfs: remove the bt_bdev_file buftarg field

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 12:40:40PM +0200, Christoph Hellwig wrote:
> And use bt_file for both bdev and shmem backed buftargs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Woot, that's a nice savings!
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 4 ++--
>  fs/xfs/xfs_buf.h | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 661f6c70e9d0..b73da43f489c 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1683,7 +1683,7 @@ xfs_free_buftarg(
>  	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
>  	/* the main block device is closed by kill_block_super */
>  	if (btp->bt_bdev != btp->bt_mount->m_super->s_bdev)
> -		bdev_fput(btp->bt_bdev_file);
> +		bdev_fput(btp->bt_file);
>  	kfree(btp);
>  }
>  
> @@ -1802,7 +1802,7 @@ xfs_alloc_buftarg(
>  	btp = kzalloc(sizeof(*btp), GFP_KERNEL | __GFP_NOFAIL);
>  
>  	btp->bt_mount = mp;
> -	btp->bt_bdev_file = bdev_file;
> +	btp->bt_file = bdev_file;
>  	btp->bt_bdev = file_bdev(bdev_file);
>  	btp->bt_dev = btp->bt_bdev->bd_dev;
>  	btp->bt_daxdev = fs_dax_get_by_bdev(btp->bt_bdev, &btp->bt_dax_part_off,
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 7987a6d64874..b269e115d9ac 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -94,7 +94,6 @@ void xfs_buf_cache_destroy(struct xfs_buf_cache *bch);
>   */
>  struct xfs_buftarg {
>  	dev_t			bt_dev;
> -	struct file		*bt_bdev_file;
>  	struct block_device	*bt_bdev;
>  	struct dax_device	*bt_daxdev;
>  	struct file		*bt_file;
> -- 
> 2.47.2
> 
> 

[PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

The file system only has a single file system sector size.  Read that
from the in-core super block to avoid confusion about the two different
"sector sizes" stored in the buftarg.  Note that this loosens the
alignment asserts for memory backed buftargs that set the page size here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c     | 20 +++++---------------
 fs/xfs/xfs_buf.h     | 15 ---------------
 fs/xfs/xfs_buf_mem.c |  2 --
 3 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index b73da43f489c..0f20d9514d0d 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -387,17 +387,18 @@ xfs_buf_map_verify(
 	struct xfs_buftarg	*btp,
 	struct xfs_buf_map	*map)
 {
+	struct xfs_mount	*mp = btp->bt_mount;
 	xfs_daddr_t		eofs;
 
 	/* Check for IOs smaller than the sector size / not sector aligned */
-	ASSERT(!(BBTOB(map->bm_len) < btp->bt_meta_sectorsize));
-	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)btp->bt_meta_sectormask));
+	ASSERT(!(BBTOB(map->bm_len) < mp->m_sb.sb_sectsize));
+	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)(mp->m_sb.sb_sectsize - 1)));
 
 	/*
 	 * Corrupted block numbers can get through to here, unfortunately, so we
 	 * have to check that the buffer falls within the filesystem bounds.
 	 */
-	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
+	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
 	if (map->bm_bn < 0 || map->bm_bn >= eofs) {
 		xfs_alert(btp->bt_mount,
 			  "%s: daddr 0x%llx out of range, EOFS 0x%llx",
@@ -1726,10 +1727,6 @@ xfs_configure_buftarg(
 
 	ASSERT(btp->bt_bdev != NULL);
 
-	/* Set up metadata sector size info */
-	btp->bt_meta_sectorsize = sectorsize;
-	btp->bt_meta_sectormask = sectorsize - 1;
-
 	error = bdev_validate_blocksize(btp->bt_bdev, sectorsize);
 	if (error) {
 		xfs_warn(btp->bt_mount,
@@ -1816,14 +1813,7 @@ xfs_alloc_buftarg(
 	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.
-	 */
-	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,
+	error = xfs_init_buftarg(btp, bdev_logical_block_size(btp->bt_bdev),
 				mp->m_super->s_id);
 	if (error)
 		goto error_free;
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index b269e115d9ac..8edfd9ed799e 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -79,19 +79,6 @@ struct xfs_buf_cache {
 int xfs_buf_cache_init(struct xfs_buf_cache *bch);
 void xfs_buf_cache_destroy(struct xfs_buf_cache *bch);
 
-/*
- * The xfs_buftarg contains 2 notions of "sector size" -
- *
- * 1) The metadata sector size, which is the minimum unit and
- *    alignment of IO which will be performed by metadata operations.
- * 2) The device logical sector size
- *
- * The first is specified at mkfs time, and is stored on-disk in the
- * superblock's sb_sectsize.
- *
- * The latter is derived from the underlying device, and controls direct IO
- * alignment constraints.
- */
 struct xfs_buftarg {
 	dev_t			bt_dev;
 	struct block_device	*bt_bdev;
@@ -99,8 +86,6 @@ struct xfs_buftarg {
 	struct file		*bt_file;
 	u64			bt_dax_part_off;
 	struct xfs_mount	*bt_mount;
-	unsigned int		bt_meta_sectorsize;
-	size_t			bt_meta_sectormask;
 	size_t			bt_logical_sectorsize;
 	size_t			bt_logical_sectormask;
 
diff --git a/fs/xfs/xfs_buf_mem.c b/fs/xfs/xfs_buf_mem.c
index dcbfa274e06d..46f527750d34 100644
--- a/fs/xfs/xfs_buf_mem.c
+++ b/fs/xfs/xfs_buf_mem.c
@@ -90,8 +90,6 @@ xmbuf_alloc(
 	btp->bt_dev = (dev_t)-1U;
 	btp->bt_bdev = NULL; /* in-memory buftargs have no bdev */
 	btp->bt_file = file;
-	btp->bt_meta_sectorsize = XMBUF_BLOCKSIZE;
-	btp->bt_meta_sectormask = XMBUF_BLOCKSIZE - 1;
 
 	error = xfs_init_buftarg(btp, XMBUF_BLOCKSIZE, descr);
 	if (error)
-- 
2.47.2

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Darrick J. Wong]

On Tue, Jul 01, 2025 at 12:40:41PM +0200, Christoph Hellwig wrote:
> The file system only has a single file system sector size.  Read that
> from the in-core super block to avoid confusion about the two different
> "sector sizes" stored in the buftarg.

Yeah, that whole bt_meta_sectorsize stuff was confusing.

>                                        Note that this loosens the
> alignment asserts for memory backed buftargs that set the page size here.

Doesn't matter at all since shmem files are byte addressable anyway. :)

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c     | 20 +++++---------------
>  fs/xfs/xfs_buf.h     | 15 ---------------
>  fs/xfs/xfs_buf_mem.c |  2 --
>  3 files changed, 5 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index b73da43f489c..0f20d9514d0d 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -387,17 +387,18 @@ xfs_buf_map_verify(
>  	struct xfs_buftarg	*btp,
>  	struct xfs_buf_map	*map)
>  {
> +	struct xfs_mount	*mp = btp->bt_mount;
>  	xfs_daddr_t		eofs;
>  
>  	/* Check for IOs smaller than the sector size / not sector aligned */
> -	ASSERT(!(BBTOB(map->bm_len) < btp->bt_meta_sectorsize));
> -	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)btp->bt_meta_sectormask));
> +	ASSERT(!(BBTOB(map->bm_len) < mp->m_sb.sb_sectsize));

Urgh, should the polarity of this be reversed?

	ASSERT(BBTOB(map->bm_len) >= mp->m_sb.sb_sectsize);

Though I don't really care enough to block the patch so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)(mp->m_sb.sb_sectsize - 1)));
>  
>  	/*
>  	 * Corrupted block numbers can get through to here, unfortunately, so we
>  	 * have to check that the buffer falls within the filesystem bounds.
>  	 */
> -	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
> +	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
>  	if (map->bm_bn < 0 || map->bm_bn >= eofs) {
>  		xfs_alert(btp->bt_mount,
>  			  "%s: daddr 0x%llx out of range, EOFS 0x%llx",
> @@ -1726,10 +1727,6 @@ xfs_configure_buftarg(
>  
>  	ASSERT(btp->bt_bdev != NULL);
>  
> -	/* Set up metadata sector size info */
> -	btp->bt_meta_sectorsize = sectorsize;
> -	btp->bt_meta_sectormask = sectorsize - 1;
> -
>  	error = bdev_validate_blocksize(btp->bt_bdev, sectorsize);
>  	if (error) {
>  		xfs_warn(btp->bt_mount,
> @@ -1816,14 +1813,7 @@ xfs_alloc_buftarg(
>  	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.
> -	 */
> -	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,
> +	error = xfs_init_buftarg(btp, bdev_logical_block_size(btp->bt_bdev),
>  				mp->m_super->s_id);
>  	if (error)
>  		goto error_free;
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index b269e115d9ac..8edfd9ed799e 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -79,19 +79,6 @@ struct xfs_buf_cache {
>  int xfs_buf_cache_init(struct xfs_buf_cache *bch);
>  void xfs_buf_cache_destroy(struct xfs_buf_cache *bch);
>  
> -/*
> - * The xfs_buftarg contains 2 notions of "sector size" -
> - *
> - * 1) The metadata sector size, which is the minimum unit and
> - *    alignment of IO which will be performed by metadata operations.
> - * 2) The device logical sector size
> - *
> - * The first is specified at mkfs time, and is stored on-disk in the
> - * superblock's sb_sectsize.
> - *
> - * The latter is derived from the underlying device, and controls direct IO
> - * alignment constraints.
> - */
>  struct xfs_buftarg {
>  	dev_t			bt_dev;
>  	struct block_device	*bt_bdev;
> @@ -99,8 +86,6 @@ struct xfs_buftarg {
>  	struct file		*bt_file;
>  	u64			bt_dax_part_off;
>  	struct xfs_mount	*bt_mount;
> -	unsigned int		bt_meta_sectorsize;
> -	size_t			bt_meta_sectormask;
>  	size_t			bt_logical_sectorsize;
>  	size_t			bt_logical_sectormask;
>  
> diff --git a/fs/xfs/xfs_buf_mem.c b/fs/xfs/xfs_buf_mem.c
> index dcbfa274e06d..46f527750d34 100644
> --- a/fs/xfs/xfs_buf_mem.c
> +++ b/fs/xfs/xfs_buf_mem.c
> @@ -90,8 +90,6 @@ xmbuf_alloc(
>  	btp->bt_dev = (dev_t)-1U;
>  	btp->bt_bdev = NULL; /* in-memory buftargs have no bdev */
>  	btp->bt_file = file;
> -	btp->bt_meta_sectorsize = XMBUF_BLOCKSIZE;
> -	btp->bt_meta_sectormask = XMBUF_BLOCKSIZE - 1;
>  
>  	error = xfs_init_buftarg(btp, XMBUF_BLOCKSIZE, descr);
>  	if (error)
> -- 
> 2.47.2
> 
> 

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Dave Chinner]

On Tue, Jul 01, 2025 at 12:40:41PM +0200, Christoph Hellwig wrote:
> The file system only has a single file system sector size.  Read that

This is still an incorrect assertion.

We still have the exact same notion of two different sector sizes
for the data device - one for metadata alignment (from the sb) and
a different one for data (direct IO) alignment (from the bdev).

Removing the abstraction and the comment that explains this does not
make this fundamental data vs metadata sector size difference
within the data device buftarg and betweeen different buftargs
go away.

All you are doing is changing the way this differentiation is
implemented - it's making the data device metadata sector size an
implicit property of *all* buftargs, rather than an explicit
property defined at buftarg instantiation.

IOWs, you are removing an explicit abstraction that exists for
correctness and replacing it with a fixed value that isn't correct
for all buftargs in the filesystem.

> Read that
> from the in-core super block to avoid confusion about the two different
> "sector sizes" stored in the buftarg.  Note that this loosens the
> alignment asserts for memory backed buftargs that set the page size here.

I think you got that wrong, because ....


> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c     | 20 +++++---------------
>  fs/xfs/xfs_buf.h     | 15 ---------------
>  fs/xfs/xfs_buf_mem.c |  2 --
>  3 files changed, 5 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index b73da43f489c..0f20d9514d0d 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -387,17 +387,18 @@ xfs_buf_map_verify(
>  	struct xfs_buftarg	*btp,
>  	struct xfs_buf_map	*map)
>  {
> +	struct xfs_mount	*mp = btp->bt_mount;
>  	xfs_daddr_t		eofs;
>  
>  	/* Check for IOs smaller than the sector size / not sector aligned */
> -	ASSERT(!(BBTOB(map->bm_len) < btp->bt_meta_sectorsize));
> -	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)btp->bt_meta_sectormask));
> +	ASSERT(!(BBTOB(map->bm_len) < mp->m_sb.sb_sectsize));
> +	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)(mp->m_sb.sb_sectsize - 1)));

.... these asserts will fire for single block XMBUF buffers because
PAGE_SIZE < mp->m_sb.sb_sectsize when the metadata sector size is
larger than PAGE_SIZE....

Fundamentally, using mp->m_sb.sb_sectsize for all buftargs is simply
wrong. Buftargs have different sector sizes, and we should continue
to encapsulate this in the buftarg.

Also, I can see no obvious reason for getting rid of this
abstraction and none has been given. It doesn't make the code any
cleaner, and it introduces an incorrect implicit assumption in the
buffer cache code (i.e. that every buftarg has the same sector
size). Hence I don't think we actually should be "cleaning up" this
code like this...

Now, if there's some other functional reason for doing this change
that hasn't been stated, let's talk about the change in that
context. Can you explain why this is necessary?

But as a straight cleanup it makes no sense...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

On Wed, Jul 02, 2025 at 08:55:06AM +1000, Dave Chinner wrote:
> .... these asserts will fire for single block XMBUF buffers because
> PAGE_SIZE < mp->m_sb.sb_sectsize when the metadata sector size is
> larger than PAGE_SIZE....

Oh right, we now support metadata sector sizes > PAGE_SIZE.

> Also, I can see no obvious reason for getting rid of this
> abstraction and none has been given. It doesn't make the code any
> cleaner, and it introduces an incorrect implicit assumption in the
> buffer cache code (i.e. that every buftarg has the same sector
> size). Hence I don't think we actually should be "cleaning up" this
> code like this...
> 
> Now, if there's some other functional reason for doing this change
> that hasn't been stated, let's talk about the change in that
> context. Can you explain why this is necessary?

Mostly because they way they were used is horribly confusing.
The worst part is fixed up in patch 1, but I wanted to go one step
futher. I guess I'll just drop this patch and we'll have to live
with the two sector sizes.

misc cleanups

[Christoph Hellwig]

Hi all,

this series has a bunch of cleanups, mostly around the mount code and
triggered by various recent changes in the area.

Diffstat:
 xfs_buf.c     |   42 +++++-------------------
 xfs_buf.h     |   23 +------------
 xfs_buf_mem.c |    2 -
 xfs_file.c    |    2 -
 xfs_inode.h   |    2 -
 xfs_iomap.c   |    2 -
 xfs_iops.c    |    2 -
 xfs_mount.c   |   98 +++++++++++++++++++++++-----------------------------------
 xfs_super.c   |   12 +------
 xfs_trace.h   |   31 ++++++++----------
 10 files changed, 74 insertions(+), 142 deletions(-)

[PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

The file system only has a single file system sector size.  Read that
from the in-core super block to avoid confusion about the two different
"sector sizes" stored in the buftarg.  Note that this loosens the
alignment asserts for memory backed buftargs that set the page size here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c     | 21 +++++----------------
 fs/xfs/xfs_buf.h     | 17 +----------------
 fs/xfs/xfs_buf_mem.c |  2 --
 fs/xfs/xfs_super.c   | 12 +++---------
 4 files changed, 9 insertions(+), 43 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index c8f0f8fe433a..0acac5302c54 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -387,11 +387,12 @@ xfs_buf_map_verify(
 	struct xfs_buftarg	*btp,
 	struct xfs_buf_map	*map)
 {
+	unsigned int		sectsize = btp->bt_mount->m_sb.sb_sectsize;
 	xfs_daddr_t		eofs;
 
 	/* Check for IOs smaller than the sector size / not sector aligned */
-	ASSERT(!(BBTOB(map->bm_len) < btp->bt_meta_sectorsize));
-	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)btp->bt_meta_sectormask));
+	ASSERT(!(BBTOB(map->bm_len) < sectsize));
+	ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)(sectsize - 1)));
 
 	/*
 	 * Corrupted block numbers can get through to here, unfortunately, so we
@@ -1719,15 +1720,10 @@ xfs_configure_buftarg_atomic_writes(
 /* Configure a buffer target that abstracts a block device. */
 int
 xfs_configure_buftarg(
-	struct xfs_buftarg	*btp,
-	unsigned int		sectorsize)
+	struct xfs_buftarg	*btp)
 {
 	ASSERT(btp->bt_bdev != NULL);
 
-	/* Set up metadata sector size info */
-	btp->bt_meta_sectorsize = sectorsize;
-	btp->bt_meta_sectormask = sectorsize - 1;
-
 	/*
 	 * Flush the block device pagecache so our bios see anything dirtied
 	 * before mount.
@@ -1806,14 +1802,7 @@ xfs_alloc_buftarg(
 	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.
-	 */
-	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,
+	error = xfs_init_buftarg(btp, bdev_logical_block_size(btp->bt_bdev),
 				mp->m_super->s_id);
 	if (error)
 		goto error_free;
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index adc97351f12a..ec17baed2cbb 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -79,19 +79,6 @@ struct xfs_buf_cache {
 int xfs_buf_cache_init(struct xfs_buf_cache *bch);
 void xfs_buf_cache_destroy(struct xfs_buf_cache *bch);
 
-/*
- * The xfs_buftarg contains 2 notions of "sector size" -
- *
- * 1) The metadata sector size, which is the minimum unit and
- *    alignment of IO which will be performed by metadata operations.
- * 2) The device logical sector size
- *
- * The first is specified at mkfs time, and is stored on-disk in the
- * superblock's sb_sectsize.
- *
- * The latter is derived from the underlying device, and controls direct IO
- * alignment constraints.
- */
 struct xfs_buftarg {
 	dev_t			bt_dev;
 	struct block_device	*bt_bdev;
@@ -99,8 +86,6 @@ struct xfs_buftarg {
 	struct file		*bt_file;
 	u64			bt_dax_part_off;
 	struct xfs_mount	*bt_mount;
-	unsigned int		bt_meta_sectorsize;
-	size_t			bt_meta_sectormask;
 	size_t			bt_logical_sectorsize;
 	size_t			bt_logical_sectormask;
 
@@ -373,7 +358,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 *);
-int xfs_configure_buftarg(struct xfs_buftarg *btp, unsigned int sectorsize);
+int xfs_configure_buftarg(struct xfs_buftarg *btp);
 
 #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
 
diff --git a/fs/xfs/xfs_buf_mem.c b/fs/xfs/xfs_buf_mem.c
index dcbfa274e06d..46f527750d34 100644
--- a/fs/xfs/xfs_buf_mem.c
+++ b/fs/xfs/xfs_buf_mem.c
@@ -90,8 +90,6 @@ xmbuf_alloc(
 	btp->bt_dev = (dev_t)-1U;
 	btp->bt_bdev = NULL; /* in-memory buftargs have no bdev */
 	btp->bt_file = file;
-	btp->bt_meta_sectorsize = XMBUF_BLOCKSIZE;
-	btp->bt_meta_sectormask = XMBUF_BLOCKSIZE - 1;
 
 	error = xfs_init_buftarg(btp, XMBUF_BLOCKSIZE, descr);
 	if (error)
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bb0a82635a77..9067a6977627 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -541,17 +541,12 @@ xfs_setup_devices(
 {
 	int			error;
 
-	error = xfs_configure_buftarg(mp->m_ddev_targp, mp->m_sb.sb_sectsize);
+	error = xfs_configure_buftarg(mp->m_ddev_targp);
 	if (error)
 		return error;
 
 	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
-		unsigned int	log_sector_size = BBSIZE;
-
-		if (xfs_has_sector(mp))
-			log_sector_size = mp->m_sb.sb_logsectsize;
-		error = xfs_configure_buftarg(mp->m_logdev_targp,
-					    log_sector_size);
+		error = xfs_configure_buftarg(mp->m_logdev_targp);
 		if (error)
 			return error;
 	}
@@ -564,8 +559,7 @@ xfs_setup_devices(
 		}
 		mp->m_rtdev_targp = mp->m_ddev_targp;
 	} else if (mp->m_rtname) {
-		error = xfs_configure_buftarg(mp->m_rtdev_targp,
-					    mp->m_sb.sb_sectsize);
+		error = xfs_configure_buftarg(mp->m_rtdev_targp);
 		if (error)
 			return error;
 	}
-- 
2.47.2

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[John Garry]

On 17/06/2025 11:52, Christoph Hellwig wrote:
>   xfs_configure_buftarg(
> -	struct xfs_buftarg	*btp,
> -	unsigned int		sectorsize)
> +	struct xfs_buftarg	*btp)
>   {

This is now just really a wrapper for calling 
xfs_configure_buftarg_atomic_write() - is that ok?

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[John Garry]

On 17/06/2025 13:15, John Garry wrote:
> On 17/06/2025 11:52, Christoph Hellwig wrote:
>>   xfs_configure_buftarg(
>> -    struct xfs_buftarg    *btp,
>> -    unsigned int        sectorsize)
>> +    struct xfs_buftarg    *btp)
>>   {
> 
> This is now just really a wrapper for calling 
> xfs_configure_buftarg_atomic_write() - is that ok?

And the bdev_can_atomic_write() call [not shown] is superfluous already, 
as we have that same check in bdev_atomic_write_unit_{min,max}_bytes()

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

On Tue, Jun 17, 2025 at 01:21:38PM +0100, John Garry wrote:
> On 17/06/2025 13:15, John Garry wrote:
>> On 17/06/2025 11:52, Christoph Hellwig wrote:
>>>   xfs_configure_buftarg(
>>> -    struct xfs_buftarg    *btp,
>>> -    unsigned int        sectorsize)
>>> +    struct xfs_buftarg    *btp)
>>>   {
>>
>> This is now just really a wrapper for calling 
>> xfs_configure_buftarg_atomic_write() - is that ok?
>
> And the bdev_can_atomic_write() call [not shown] is superfluous already, as 
> we have that same check in bdev_atomic_write_unit_{min,max}_bytes()

I'll look into it, that should simplify things further.

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Dave Chinner]

On Tue, Jun 17, 2025 at 12:52:05PM +0200, Christoph Hellwig wrote:
> The file system only has a single file system sector size.

The external log device can have a different sector size to
the rest of the filesystem. This series looks like it removes the
ability to validate that the log device sector size in teh
superblock is valid for the backing device....

At minimum, the series description needs to be more than "this is
just a code cleanup" because it really changes the way block devices
are opened, configured and validated against the on-disk information
in the superblock....

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

On Wed, Jun 18, 2025 at 09:51:10AM +1000, Dave Chinner wrote:
> On Tue, Jun 17, 2025 at 12:52:05PM +0200, Christoph Hellwig wrote:
> > The file system only has a single file system sector size.
> 
> The external log device can have a different sector size to
> the rest of the filesystem. This series looks like it removes the
> ability to validate that the log device sector size in teh
> superblock is valid for the backing device....

I don't follow.  Do you mean it remove the future possibility to do this?
Even then it would be better to do this directly based off the superblock
and not use a field in the buftarg currently only used for cached buffers
(which aren't used on anything but the main device).

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Dave Chinner]

On Wed, Jun 18, 2025 at 07:15:09AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 18, 2025 at 09:51:10AM +1000, Dave Chinner wrote:
> > On Tue, Jun 17, 2025 at 12:52:05PM +0200, Christoph Hellwig wrote:
> > > The file system only has a single file system sector size.
> > 
> > The external log device can have a different sector size to
> > the rest of the filesystem. This series looks like it removes the
> > ability to validate that the log device sector size in teh
> > superblock is valid for the backing device....
> 
> I don't follow.  Do you mean it remove the future possibility to do this?

No, I mean that this:

# mkfs.xfs -l sectsize=512,logdev=/dev/nvme1n1 -d sectsize=4k ....  /dev/nvme0n1

is an valid filesystem configuration and has been for a long, long
time. i.e. the logdev does not have to have the same physical sector
size support as the data device.

If the above filesystem was moved to new devices where the external
log device also had a minimum LBA sector size of 4kB, that
filesystem must not mount. The 512 byte sector size for the journal
means journal IO is aligned and rounded to 512 byte boundaries, not
4kB boundaries like the new underlying device requires. IOWs, that
fs config is unusable on that new device config.

This sort of sector size mismatch is currently caught by the
xfs_setup_devices() -> xfs_configure_buftarg() ->
bdev_validate_blocksize() path. That validation path is what you are
removing in this series, so you are introducing regressions in device
sector size validation during mount....

> Even then it would be better to do this directly based off the superblock
> and not use a field in the buftarg currently only used for cached buffers
> (which aren't used on anything but the main device).

IDGI. The sector size passed to xfs_configure_buftarg() by
xfs_setup_devices() for the bdev LBA size check comes directly from
the superblock. You're advocating that we do exactly what the code
you are removing already does....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

On Thu, Jun 19, 2025 at 12:42:39PM +1000, Dave Chinner wrote:
> > > The external log device can have a different sector size to
> > > the rest of the filesystem. This series looks like it removes the
> > > ability to validate that the log device sector size in teh
> > > superblock is valid for the backing device....
> > 
> > I don't follow.  Do you mean it remove the future possibility to do this?
> 
> No, I mean that this:
> 
> # mkfs.xfs -l sectsize=512,logdev=/dev/nvme1n1 -d sectsize=4k ....  /dev/nvme0n1
> 
> is an valid filesystem configuration and has been for a long, long
> time. i.e. the logdev does not have to have the same physical sector
> size support as the data device.

Sure, and I've never disagreed.  But you'd not explained how that is
relevant for this patch.  The bt_meta_sectorsize is only used for
asserting the alignment of cached buffers, and we place no buffers
(cached or uncached) on the log device ever.

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Dave Chinner]

On Tue, Jun 24, 2025 at 04:11:17PM +0200, Christoph Hellwig wrote:
> On Thu, Jun 19, 2025 at 12:42:39PM +1000, Dave Chinner wrote:
> > > > The external log device can have a different sector size to
> > > > the rest of the filesystem. This series looks like it removes the
> > > > ability to validate that the log device sector size in teh
> > > > superblock is valid for the backing device....
> > > 
> > > I don't follow.  Do you mean it remove the future possibility to do this?
> > 
> > No, I mean that this:
> > 
> > # mkfs.xfs -l sectsize=512,logdev=/dev/nvme1n1 -d sectsize=4k ....  /dev/nvme0n1
> > 
> > is an valid filesystem configuration and has been for a long, long
> > time. i.e. the logdev does not have to have the same physical sector
> > size support as the data device.
> 
> Sure, and I've never disagreed.  But you'd not explained how that is
> relevant for this patch.  The bt_meta_sectorsize is only used for
> asserting the alignment of cached buffers, and we place no buffers
> (cached or uncached) on the log device ever.

Again: I am not talking about the bt_meta_sectorsize removal being a
problem!

I will repeat it once more: this patchset removes the check that
guarantees the the underlying block device has a sector size that is
valid for the sector size the filesystem devices are configured to
use. That is not acceptible - a 512 byte sector filesystem device
must not be able to mount on a hard 4kB sector device because the
moment we do a 512 byte aligned IO to the log device, the bdev will
give an EIO error and we'll shut down the filesystem.

We check for sector size mismatches at device open time so we don't
get into this nasty situation. You are removing those sector size
checks in this patch set. This is an obvious functional regression,
and that is the part of this change that I'm NACKing. 

Christoph, I explained this in the email you are replying to, and
you have simply ignored it all. Please at least read the entire
email before you respond this time!

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[Christoph Hellwig]

On Wed, Jun 25, 2025 at 09:55:28AM +1000, Dave Chinner wrote:
> Again: I am not talking about the bt_meta_sectorsize removal being a
> problem!

Why do you comment on the patch doing that then?

> I will repeat it once more: this patchset removes the check that
> guarantees the the underlying block device has a sector size that is
> valid for the sector size the filesystem devices are configured to
> use. That is not acceptible - a 512 byte sector filesystem device
> must not be able to mount on a hard 4kB sector device because the
> moment we do a 512 byte aligned IO to the log device, the bdev will
> give an EIO error and we'll shut down the filesystem.

Yes, and I've already agree to not drop that check after you initially
pointed that out.

So I'm really confused on what you are trying to comment on for this
patch.

Re: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg

[kernel test robot]

Hi Christoph,

kernel test robot noticed the following build warnings:

[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on linus/master v6.16-rc2 next-20250617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christoph-Hellwig/xfs-remove-the-call-to-sync_blockdev-in-xfs_configure_buftarg/20250617-195226
base:   https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link:    https://lore.kernel.org/r/20250617105238.3393499-8-hch%40lst.de
patch subject: [PATCH 7/7] xfs: remove the bt_meta_sectorsize field in struct buftarg
config: s390-randconfig-r072-20250618 (https://download.01.org/0day-ci/archive/20250618/202506181720.cz0T8fSK-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250618/202506181720.cz0T8fSK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506181720.cz0T8fSK-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/xfs/xfs_buf.c: In function 'xfs_buf_map_verify':
>> fs/xfs/xfs_buf.c:390:16: warning: unused variable 'sectsize' [-Wunused-variable]
     unsigned int  sectsize = btp->bt_mount->m_sb.sb_sectsize;
                   ^~~~~~~~


vim +/sectsize +390 fs/xfs/xfs_buf.c

   384	
   385	static int
   386	xfs_buf_map_verify(
   387		struct xfs_buftarg	*btp,
   388		struct xfs_buf_map	*map)
   389	{
 > 390		unsigned int		sectsize = btp->bt_mount->m_sb.sb_sectsize;
   391		xfs_daddr_t		eofs;
   392	
   393		/* Check for IOs smaller than the sector size / not sector aligned */
   394		ASSERT(!(BBTOB(map->bm_len) < sectsize));
   395		ASSERT(!(BBTOB(map->bm_bn) & (xfs_off_t)(sectsize - 1)));
   396	
   397		/*
   398		 * Corrupted block numbers can get through to here, unfortunately, so we
   399		 * have to check that the buffer falls within the filesystem bounds.
   400		 */
   401		eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
   402		if (map->bm_bn < 0 || map->bm_bn >= eofs) {
   403			xfs_alert(btp->bt_mount,
   404				  "%s: daddr 0x%llx out of range, EOFS 0x%llx",
   405				  __func__, map->bm_bn, eofs);
   406			WARN_ON(1);
   407			return -EFSCORRUPTED;
   408		}
   409		return 0;
   410	}
   411	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki