small fixes for 6.13

small fixes for 6.13

[Christoph Hellwig]

Hi all,

two little fixes and a refactoring to prepare one of them for the
new rtgroup code.

Diffstat:
 libxfs/xfs_bmap.c |    6 +----
 xfs_rtalloc.c     |   62 +++++++++++++++++++++++++++---------------------------
 2 files changed, 33 insertions(+), 35 deletions(-)

[PATCH 1/3] xfs: factor out a xfs_rt_check_size helper

[Christoph Hellwig]

Add a helper to check that the last block of a RT device is readable
to share the code between mount and growfs.  This also adds the mount
time overflow check to growfs and improves the error messages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_rtalloc.c | 62 ++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 0cb534d71119..90f4fdd47087 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1225,6 +1225,34 @@ xfs_grow_last_rtg(
 			mp->m_sb.sb_rgextents;
 }
 
+/*
+ * Read in the last block of the RT device to make sure it is accessible.
+ */
+static int
+xfs_rt_check_size(
+	struct xfs_mount	*mp,
+	xfs_rfsblock_t		last_block)
+{
+	xfs_daddr_t		daddr = XFS_FSB_TO_BB(mp, last_block);
+	struct xfs_buf		*bp;
+	int			error;
+
+	if (XFS_BB_TO_FSB(mp, daddr) != last_block) {
+		xfs_warn(mp, "RT device size overflow: %llu != %llu",
+			XFS_BB_TO_FSB(mp, daddr), last_block);
+		return -EFBIG;
+	}
+
+	error = xfs_buf_read_uncached(mp->m_rtdev_targp, daddr,
+			XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
+	if (error)
+		xfs_warn(mp, "cannot read last RT device block (%lld)",
+				last_block);
+	else
+		xfs_buf_relse(bp);
+	return error;
+}
+
 /*
  * Grow the realtime area of the filesystem.
  */
@@ -1236,7 +1264,6 @@ xfs_growfs_rt(
 	xfs_rgnumber_t		old_rgcount = mp->m_sb.sb_rgcount;
 	xfs_rgnumber_t		new_rgcount = 1;
 	xfs_rgnumber_t		rgno;
-	struct xfs_buf		*bp;
 	xfs_agblock_t		old_rextsize = mp->m_sb.sb_rextsize;
 	int			error;
 
@@ -1273,15 +1300,10 @@ xfs_growfs_rt(
 	error = xfs_sb_validate_fsb_count(&mp->m_sb, in->newblocks);
 	if (error)
 		goto out_unlock;
-	/*
-	 * Read in the last block of the device, make sure it exists.
-	 */
-	error = xfs_buf_read_uncached(mp->m_rtdev_targp,
-				XFS_FSB_TO_BB(mp, in->newblocks - 1),
-				XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
+
+	error = xfs_rt_check_size(mp, in->newblocks - 1);
 	if (error)
 		goto out_unlock;
-	xfs_buf_relse(bp);
 
 	/*
 	 * Calculate new parameters.  These are the final values to be reached.
@@ -1408,10 +1430,6 @@ int				/* error */
 xfs_rtmount_init(
 	struct xfs_mount	*mp)	/* file system mount structure */
 {
-	struct xfs_buf		*bp;	/* buffer for last block of subvolume */
-	xfs_daddr_t		d;	/* address of last block of subvolume */
-	int			error;
-
 	if (mp->m_sb.sb_rblocks == 0)
 		return 0;
 	if (mp->m_rtdev_targp == NULL) {
@@ -1422,25 +1440,7 @@ xfs_rtmount_init(
 
 	mp->m_rsumblocks = xfs_rtsummary_blockcount(mp, &mp->m_rsumlevels);
 
-	/*
-	 * Check that the realtime section is an ok size.
-	 */
-	d = (xfs_daddr_t)XFS_FSB_TO_BB(mp, mp->m_sb.sb_rblocks);
-	if (XFS_BB_TO_FSB(mp, d) != mp->m_sb.sb_rblocks) {
-		xfs_warn(mp, "realtime mount -- %llu != %llu",
-			(unsigned long long) XFS_BB_TO_FSB(mp, d),
-			(unsigned long long) mp->m_sb.sb_rblocks);
-		return -EFBIG;
-	}
-	error = xfs_buf_read_uncached(mp->m_rtdev_targp,
-					d - XFS_FSB_TO_BB(mp, 1),
-					XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
-	if (error) {
-		xfs_warn(mp, "realtime device size check failed");
-		return error;
-	}
-	xfs_buf_relse(bp);
-	return 0;
+	return xfs_rt_check_size(mp, mp->m_sb.sb_rblocks - 1);
 }
 
 static int
-- 
2.45.2

Re: [PATCH 1/3] xfs: factor out a xfs_rt_check_size helper

[Darrick J. Wong]

On Tue, Nov 19, 2024 at 04:49:47PM +0100, Christoph Hellwig wrote:
> Add a helper to check that the last block of a RT device is readable
> to share the code between mount and growfs.  This also adds the mount
> time overflow check to growfs and improves the error messages.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_rtalloc.c | 62 ++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 0cb534d71119..90f4fdd47087 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1225,6 +1225,34 @@ xfs_grow_last_rtg(
>  			mp->m_sb.sb_rgextents;
>  }
>  
> +/*
> + * Read in the last block of the RT device to make sure it is accessible.
> + */
> +static int
> +xfs_rt_check_size(
> +	struct xfs_mount	*mp,
> +	xfs_rfsblock_t		last_block)
> +{
> +	xfs_daddr_t		daddr = XFS_FSB_TO_BB(mp, last_block);
> +	struct xfs_buf		*bp;
> +	int			error;
> +
> +	if (XFS_BB_TO_FSB(mp, daddr) != last_block) {
> +		xfs_warn(mp, "RT device size overflow: %llu != %llu",
> +			XFS_BB_TO_FSB(mp, daddr), last_block);
> +		return -EFBIG;
> +	}
> +
> +	error = xfs_buf_read_uncached(mp->m_rtdev_targp, daddr,
> +			XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
> +	if (error)
> +		xfs_warn(mp, "cannot read last RT device block (%lld)",
> +				last_block);

"cannot read last RT device sector"?  Something to hint that the units
are 512b blocks, not fsblocks?  This is certainly better than the old
message. :)

Also, should there be a similar function to handle the last datadev
read that mount and growfs perform?

--D

> +	else
> +		xfs_buf_relse(bp);
> +	return error;
> +}
> +
>  /*
>   * Grow the realtime area of the filesystem.
>   */
> @@ -1236,7 +1264,6 @@ xfs_growfs_rt(
>  	xfs_rgnumber_t		old_rgcount = mp->m_sb.sb_rgcount;
>  	xfs_rgnumber_t		new_rgcount = 1;
>  	xfs_rgnumber_t		rgno;
> -	struct xfs_buf		*bp;
>  	xfs_agblock_t		old_rextsize = mp->m_sb.sb_rextsize;
>  	int			error;
>  
> @@ -1273,15 +1300,10 @@ xfs_growfs_rt(
>  	error = xfs_sb_validate_fsb_count(&mp->m_sb, in->newblocks);
>  	if (error)
>  		goto out_unlock;
> -	/*
> -	 * Read in the last block of the device, make sure it exists.
> -	 */
> -	error = xfs_buf_read_uncached(mp->m_rtdev_targp,
> -				XFS_FSB_TO_BB(mp, in->newblocks - 1),
> -				XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
> +
> +	error = xfs_rt_check_size(mp, in->newblocks - 1);
>  	if (error)
>  		goto out_unlock;
> -	xfs_buf_relse(bp);
>  
>  	/*
>  	 * Calculate new parameters.  These are the final values to be reached.
> @@ -1408,10 +1430,6 @@ int				/* error */
>  xfs_rtmount_init(
>  	struct xfs_mount	*mp)	/* file system mount structure */
>  {
> -	struct xfs_buf		*bp;	/* buffer for last block of subvolume */
> -	xfs_daddr_t		d;	/* address of last block of subvolume */
> -	int			error;
> -
>  	if (mp->m_sb.sb_rblocks == 0)
>  		return 0;
>  	if (mp->m_rtdev_targp == NULL) {
> @@ -1422,25 +1440,7 @@ xfs_rtmount_init(
>  
>  	mp->m_rsumblocks = xfs_rtsummary_blockcount(mp, &mp->m_rsumlevels);
>  
> -	/*
> -	 * Check that the realtime section is an ok size.
> -	 */
> -	d = (xfs_daddr_t)XFS_FSB_TO_BB(mp, mp->m_sb.sb_rblocks);
> -	if (XFS_BB_TO_FSB(mp, d) != mp->m_sb.sb_rblocks) {
> -		xfs_warn(mp, "realtime mount -- %llu != %llu",
> -			(unsigned long long) XFS_BB_TO_FSB(mp, d),
> -			(unsigned long long) mp->m_sb.sb_rblocks);
> -		return -EFBIG;
> -	}
> -	error = xfs_buf_read_uncached(mp->m_rtdev_targp,
> -					d - XFS_FSB_TO_BB(mp, 1),
> -					XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
> -	if (error) {
> -		xfs_warn(mp, "realtime device size check failed");
> -		return error;
> -	}
> -	xfs_buf_relse(bp);
> -	return 0;
> +	return xfs_rt_check_size(mp, mp->m_sb.sb_rblocks - 1);
>  }
>  
>  static int
> -- 
> 2.45.2
> 
> 

Re: [PATCH 1/3] xfs: factor out a xfs_rt_check_size helper

[Christoph Hellwig]

On Tue, Nov 19, 2024 at 08:15:43AM -0800, Darrick J. Wong wrote:
> > +	error = xfs_buf_read_uncached(mp->m_rtdev_targp, daddr,
> > +			XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
> > +	if (error)
> > +		xfs_warn(mp, "cannot read last RT device block (%lld)",
> > +				last_block);
> 
> "cannot read last RT device sector"?  Something to hint that the units
> are 512b blocks, not fsblocks?  This is certainly better than the old
> message. :)

Sure.

> Also, should there be a similar function to handle the last datadev
> read that mount and growfs perform?

I'll take a look.

[PATCH 2/3] xfs: use the proper conversion helpers in xfs_rt_check_size

[Christoph Hellwig]

Use the proper helpers to deal with sparse rtbno encoding.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 90f4fdd47087..a991b750df81 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1233,13 +1233,13 @@ xfs_rt_check_size(
 	struct xfs_mount	*mp,
 	xfs_rfsblock_t		last_block)
 {
-	xfs_daddr_t		daddr = XFS_FSB_TO_BB(mp, last_block);
+	xfs_daddr_t		daddr = xfs_rtb_to_daddr(mp, last_block);
 	struct xfs_buf		*bp;
 	int			error;
 
-	if (XFS_BB_TO_FSB(mp, daddr) != last_block) {
+	if (xfs_daddr_to_rtb(mp, daddr) != last_block) {
 		xfs_warn(mp, "RT device size overflow: %llu != %llu",
-			XFS_BB_TO_FSB(mp, daddr), last_block);
+			xfs_daddr_to_rtb(mp, daddr), last_block);
 		return -EFBIG;
 	}
 
-- 
2.45.2

Re: [PATCH 2/3] xfs: use the proper conversion helpers in xfs_rt_check_size

[Darrick J. Wong]

On Tue, Nov 19, 2024 at 04:49:48PM +0100, Christoph Hellwig wrote:
> Use the proper helpers to deal with sparse rtbno encoding.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_rtalloc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 90f4fdd47087..a991b750df81 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1233,13 +1233,13 @@ xfs_rt_check_size(
>  	struct xfs_mount	*mp,
>  	xfs_rfsblock_t		last_block)
>  {
> -	xfs_daddr_t		daddr = XFS_FSB_TO_BB(mp, last_block);
> +	xfs_daddr_t		daddr = xfs_rtb_to_daddr(mp, last_block);
>  	struct xfs_buf		*bp;
>  	int			error;
>  
> -	if (XFS_BB_TO_FSB(mp, daddr) != last_block) {
> +	if (xfs_daddr_to_rtb(mp, daddr) != last_block) {

Er... this converts the daddr to a segmented xfs_rtblock_t type, but
last_block is a non segmented xfs_rfsblock_t type.  You can't compare
the two directly.  I think the code was correct without this patch.

--D

>  		xfs_warn(mp, "RT device size overflow: %llu != %llu",
> -			XFS_BB_TO_FSB(mp, daddr), last_block);
> +			xfs_daddr_to_rtb(mp, daddr), last_block);
>  		return -EFBIG;
>  	}
>  
> -- 
> 2.45.2
> 
> 

Re: [PATCH 2/3] xfs: use the proper conversion helpers in xfs_rt_check_size

[Christoph Hellwig]

On Tue, Nov 19, 2024 at 08:21:42AM -0800, Darrick J. Wong wrote:
> > -	xfs_daddr_t		daddr = XFS_FSB_TO_BB(mp, last_block);
> > +	xfs_daddr_t		daddr = xfs_rtb_to_daddr(mp, last_block);
> >  	struct xfs_buf		*bp;
> >  	int			error;
> >  
> > -	if (XFS_BB_TO_FSB(mp, daddr) != last_block) {
> > +	if (xfs_daddr_to_rtb(mp, daddr) != last_block) {
> 
> Er... this converts the daddr to a segmented xfs_rtblock_t type, but
> last_block is a non segmented xfs_rfsblock_t type.  You can't compare
> the two directly.  I think the code was correct without this patch.

Hmm.  Yeah, it just breaks the other things I'm about to overload
xfs_rtb_to_daddr/xfs_daddr_to_rtb with..  Time for even more
helpers probably..

[PATCH 3/3] xfs: don't call xfs_bmap_same_rtgroup in xfs_bmap_add_extent_hole_delay

[Christoph Hellwig]

xfs_bmap_add_extent_hole_delay works entirely on delalloc extents, for
which xfs_bmap_same_rtgroup doesn't make sense.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9052839305e2..5255f93bae31 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -2620,8 +2620,7 @@ xfs_bmap_add_extent_hole_delay(
 	 */
 	if ((state & BMAP_LEFT_VALID) && (state & BMAP_LEFT_DELAY) &&
 	    left.br_startoff + left.br_blockcount == new->br_startoff &&
-	    left.br_blockcount + new->br_blockcount <= XFS_MAX_BMBT_EXTLEN &&
-	    xfs_bmap_same_rtgroup(ip, whichfork, &left, new))
+	    left.br_blockcount + new->br_blockcount <= XFS_MAX_BMBT_EXTLEN)
 		state |= BMAP_LEFT_CONTIG;
 
 	if ((state & BMAP_RIGHT_VALID) && (state & BMAP_RIGHT_DELAY) &&
@@ -2629,8 +2628,7 @@ xfs_bmap_add_extent_hole_delay(
 	    new->br_blockcount + right.br_blockcount <= XFS_MAX_BMBT_EXTLEN &&
 	    (!(state & BMAP_LEFT_CONTIG) ||
 	     (left.br_blockcount + new->br_blockcount +
-	      right.br_blockcount <= XFS_MAX_BMBT_EXTLEN)) &&
-	    xfs_bmap_same_rtgroup(ip, whichfork, new, &right))
+	      right.br_blockcount <= XFS_MAX_BMBT_EXTLEN)))
 		state |= BMAP_RIGHT_CONTIG;
 
 	/*
-- 
2.45.2

Re: [PATCH 3/3] xfs: don't call xfs_bmap_same_rtgroup in xfs_bmap_add_extent_hole_delay

[Darrick J. Wong]

On Tue, Nov 19, 2024 at 04:49:49PM +0100, Christoph Hellwig wrote:
> xfs_bmap_add_extent_hole_delay works entirely on delalloc extents, for
> which xfs_bmap_same_rtgroup doesn't make sense.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 9052839305e2..5255f93bae31 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -2620,8 +2620,7 @@ xfs_bmap_add_extent_hole_delay(
>  	 */
>  	if ((state & BMAP_LEFT_VALID) && (state & BMAP_LEFT_DELAY) &&
>  	    left.br_startoff + left.br_blockcount == new->br_startoff &&
> -	    left.br_blockcount + new->br_blockcount <= XFS_MAX_BMBT_EXTLEN &&
> -	    xfs_bmap_same_rtgroup(ip, whichfork, &left, new))
> +	    left.br_blockcount + new->br_blockcount <= XFS_MAX_BMBT_EXTLEN)
>  		state |= BMAP_LEFT_CONTIG;
>  
>  	if ((state & BMAP_RIGHT_VALID) && (state & BMAP_RIGHT_DELAY) &&
> @@ -2629,8 +2628,7 @@ xfs_bmap_add_extent_hole_delay(
>  	    new->br_blockcount + right.br_blockcount <= XFS_MAX_BMBT_EXTLEN &&
>  	    (!(state & BMAP_LEFT_CONTIG) ||
>  	     (left.br_blockcount + new->br_blockcount +
> -	      right.br_blockcount <= XFS_MAX_BMBT_EXTLEN)) &&
> -	    xfs_bmap_same_rtgroup(ip, whichfork, new, &right))
> +	      right.br_blockcount <= XFS_MAX_BMBT_EXTLEN)))
>  		state |= BMAP_RIGHT_CONTIG;
>  
>  	/*
> -- 
> 2.45.2
> 
>