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

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jun 17, 2025 at 12:51:59PM +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 9d2ab567cf81..294dd9d61dbb 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -376,7 +376,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
> 
>