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

[Dave Chinner <david@fromorbit.com>]

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