RT allocator tidy ups

RT allocator tidy ups

[Christoph Hellwig]

Hi all,

this series has a bunch of tidy ups preparing for my research into
per-RTG RT bitmaps.

The first is a fix for a one-off bug that I could only reproduce with
my hacky patches, but which seems real.  The second- and third-last
patches change the policy for retrying allocations that didn't succeed
and could use very careful review.  The rest is fairly mechanic
cleanups.

Diffstat:
 libxfs/xfs_bmap.c     |   55 ++--
 libxfs/xfs_bmap.h     |    2 
 libxfs/xfs_format.h   |   14 -
 libxfs/xfs_rtbitmap.c |  106 +++-----
 libxfs/xfs_rtbitmap.h |    4 
 libxfs/xfs_types.h    |    1 
 scrub/rtsummary.c     |    2 
 xfs_bmap_util.c       |  141 ----------
 xfs_bmap_util.h       |    2 
 xfs_quota.h           |    5 
 xfs_rtalloc.c         |  647 +++++++++++++++++++++++++-------------------------
 xfs_rtalloc.h         |   37 --
 12 files changed, 416 insertions(+), 600 deletions(-)

[PATCH 01/19] xfs: consider minlen sized extents in xfs_rtallocate_extent_block

[Christoph Hellwig]

minlen is the lower bound on the extent length that the caller can
accept, and maxlen is at this point the maximal available length.
This means a minlen extent is perfectly fine to use, so do it.  This
matches the equivalent logic in xfs_rtallocate_extent_exact that also
accepts a minlen sized extent.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 8feb58c6241ce4..fe98a96a26484f 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -309,7 +309,7 @@ xfs_rtallocate_extent_block(
 	/*
 	 * Searched the whole thing & didn't find a maxlen free extent.
 	 */
-	if (minlen < maxlen && besti != -1) {
+	if (minlen <= maxlen && besti != -1) {
 		xfs_rtxlen_t	p;	/* amount to trim length by */
 
 		/*
-- 
2.39.2

Re: [PATCH 01/19] xfs: consider minlen sized extents in xfs_rtallocate_extent_block

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:20AM +0100, Christoph Hellwig wrote:
> minlen is the lower bound on the extent length that the caller can
> accept, and maxlen is at this point the maximal available length.
> This means a minlen extent is perfectly fine to use, so do it.  This
> matches the equivalent logic in xfs_rtallocate_extent_exact that also
> accepts a minlen sized extent.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_rtalloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 8feb58c6241ce4..fe98a96a26484f 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -309,7 +309,7 @@ xfs_rtallocate_extent_block(
>  	/*
>  	 * Searched the whole thing & didn't find a maxlen free extent.
>  	 */
> -	if (minlen < maxlen && besti != -1) {
> +	if (minlen <= maxlen && besti != -1) {
>  		xfs_rtxlen_t	p;	/* amount to trim length by */
>  
>  		/*
> -- 
> 2.39.2
> 
> 

[PATCH 02/19] xfs: turn the xfs_trans_mod_dquot_byino stub into an inline function

[Christoph Hellwig]

Without this upcoming change can cause an unused variable warning,
when adding a local variable for the fields field passed to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_quota.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index dcc785fdd34532..e0d56489f3b287 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -127,7 +127,10 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
 }
 #define xfs_trans_dup_dqinfo(tp, tp2)
 #define xfs_trans_free_dqinfo(tp)
-#define xfs_trans_mod_dquot_byino(tp, ip, fields, delta) do { } while (0)
+static inline void xfs_trans_mod_dquot_byino(struct xfs_trans *tp,
+		struct xfs_inode *ip, uint field, int64_t delta)
+{
+}
 #define xfs_trans_apply_dquot_deltas(tp)
 #define xfs_trans_unreserve_and_mod_dquots(tp)
 static inline int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp,
-- 
2.39.2

Re: [PATCH 02/19] xfs: turn the xfs_trans_mod_dquot_byino stub into an inline function

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:21AM +0100, Christoph Hellwig wrote:
> Without this upcoming change can cause an unused variable warning,
> when adding a local variable for the fields field passed to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_quota.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index dcc785fdd34532..e0d56489f3b287 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -127,7 +127,10 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
>  }
>  #define xfs_trans_dup_dqinfo(tp, tp2)
>  #define xfs_trans_free_dqinfo(tp)
> -#define xfs_trans_mod_dquot_byino(tp, ip, fields, delta) do { } while (0)
> +static inline void xfs_trans_mod_dquot_byino(struct xfs_trans *tp,
> +		struct xfs_inode *ip, uint field, int64_t delta)
> +{
> +}
>  #define xfs_trans_apply_dquot_deltas(tp)
>  #define xfs_trans_unreserve_and_mod_dquots(tp)
>  static inline int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp,
> -- 
> 2.39.2
> 
> 

[PATCH 03/19] xfs: remove the xfs_alloc_arg argument to xfs_bmap_btalloc_accounting

[Christoph Hellwig]

xfs_bmap_btalloc_accounting only uses the len field from args, but that
has just been propagated to ap->length field by the caller.

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index ca6614f4eac50a..afdfb3455d9ebe 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3265,8 +3265,7 @@ xfs_bmap_btalloc_select_lengths(
 /* Update all inode and quota accounting for the allocation we just did. */
 static void
 xfs_bmap_btalloc_accounting(
-	struct xfs_bmalloca	*ap,
-	struct xfs_alloc_arg	*args)
+	struct xfs_bmalloca	*ap)
 {
 	if (ap->flags & XFS_BMAPI_COWFORK) {
 		/*
@@ -3279,7 +3278,7 @@ xfs_bmap_btalloc_accounting(
 		 * yet.
 		 */
 		if (ap->wasdel) {
-			xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)args->len);
+			xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)ap->length);
 			return;
 		}
 
@@ -3291,22 +3290,22 @@ xfs_bmap_btalloc_accounting(
 		 * This essentially transfers the transaction quota reservation
 		 * to that of a delalloc extent.
 		 */
-		ap->ip->i_delayed_blks += args->len;
+		ap->ip->i_delayed_blks += ap->length;
 		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, XFS_TRANS_DQ_RES_BLKS,
-				-(long)args->len);
+				-(long)ap->length);
 		return;
 	}
 
 	/* data/attr fork only */
-	ap->ip->i_nblocks += args->len;
+	ap->ip->i_nblocks += ap->length;
 	xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
 	if (ap->wasdel) {
-		ap->ip->i_delayed_blks -= args->len;
-		xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)args->len);
+		ap->ip->i_delayed_blks -= ap->length;
+		xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)ap->length);
 	}
 	xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
 		ap->wasdel ? XFS_TRANS_DQ_DELBCOUNT : XFS_TRANS_DQ_BCOUNT,
-		args->len);
+		ap->length);
 }
 
 static int
@@ -3380,7 +3379,7 @@ xfs_bmap_process_allocated_extent(
 		ap->offset = orig_offset;
 	else if (ap->offset + ap->length < orig_offset + orig_length)
 		ap->offset = orig_offset + orig_length - ap->length;
-	xfs_bmap_btalloc_accounting(ap, args);
+	xfs_bmap_btalloc_accounting(ap);
 }
 
 #ifdef DEBUG
-- 
2.39.2

Re: [PATCH 03/19] xfs: remove the xfs_alloc_arg argument to xfs_bmap_btalloc_accounting

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:22AM +0100, Christoph Hellwig wrote:
> xfs_bmap_btalloc_accounting only uses the len field from args, but that
> has just been propagated to ap->length field by the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yep, nice streamlining...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index ca6614f4eac50a..afdfb3455d9ebe 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3265,8 +3265,7 @@ xfs_bmap_btalloc_select_lengths(
>  /* Update all inode and quota accounting for the allocation we just did. */
>  static void
>  xfs_bmap_btalloc_accounting(
> -	struct xfs_bmalloca	*ap,
> -	struct xfs_alloc_arg	*args)
> +	struct xfs_bmalloca	*ap)
>  {
>  	if (ap->flags & XFS_BMAPI_COWFORK) {
>  		/*
> @@ -3279,7 +3278,7 @@ xfs_bmap_btalloc_accounting(
>  		 * yet.
>  		 */
>  		if (ap->wasdel) {
> -			xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)args->len);
> +			xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)ap->length);
>  			return;
>  		}
>  
> @@ -3291,22 +3290,22 @@ xfs_bmap_btalloc_accounting(
>  		 * This essentially transfers the transaction quota reservation
>  		 * to that of a delalloc extent.
>  		 */
> -		ap->ip->i_delayed_blks += args->len;
> +		ap->ip->i_delayed_blks += ap->length;
>  		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, XFS_TRANS_DQ_RES_BLKS,
> -				-(long)args->len);
> +				-(long)ap->length);
>  		return;
>  	}
>  
>  	/* data/attr fork only */
> -	ap->ip->i_nblocks += args->len;
> +	ap->ip->i_nblocks += ap->length;
>  	xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
>  	if (ap->wasdel) {
> -		ap->ip->i_delayed_blks -= args->len;
> -		xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)args->len);
> +		ap->ip->i_delayed_blks -= ap->length;
> +		xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)ap->length);
>  	}
>  	xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
>  		ap->wasdel ? XFS_TRANS_DQ_DELBCOUNT : XFS_TRANS_DQ_BCOUNT,
> -		args->len);
> +		ap->length);
>  }
>  
>  static int
> @@ -3380,7 +3379,7 @@ xfs_bmap_process_allocated_extent(
>  		ap->offset = orig_offset;
>  	else if (ap->offset + ap->length < orig_offset + orig_length)
>  		ap->offset = orig_offset + orig_length - ap->length;
> -	xfs_bmap_btalloc_accounting(ap, args);
> +	xfs_bmap_btalloc_accounting(ap);
>  }
>  
>  #ifdef DEBUG
> -- 
> 2.39.2
> 
> 

[PATCH 04/19] xfs: also use xfs_bmap_btalloc_accounting for RT allocations

[Christoph Hellwig]

Make xfs_bmap_btalloc_accounting more generic by handling the RT quota
reservations and then also use it from xfs_bmap_rtalloc instead of
open coding the accounting logic there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 21 ++++++++++++++-------
 fs/xfs/libxfs/xfs_bmap.h |  2 ++
 fs/xfs/xfs_bmap_util.c   | 12 +-----------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index afdfb3455d9ebe..6722205949ad4c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3263,10 +3263,14 @@ xfs_bmap_btalloc_select_lengths(
 }
 
 /* Update all inode and quota accounting for the allocation we just did. */
-static void
-xfs_bmap_btalloc_accounting(
+void
+xfs_bmap_alloc_account(
 	struct xfs_bmalloca	*ap)
 {
+	bool			isrt = XFS_IS_REALTIME_INODE(ap->ip) &&
+					(ap->flags & XFS_BMAPI_ATTRFORK);
+	uint			fld;
+
 	if (ap->flags & XFS_BMAPI_COWFORK) {
 		/*
 		 * COW fork blocks are in-core only and thus are treated as
@@ -3291,7 +3295,8 @@ xfs_bmap_btalloc_accounting(
 		 * to that of a delalloc extent.
 		 */
 		ap->ip->i_delayed_blks += ap->length;
-		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, XFS_TRANS_DQ_RES_BLKS,
+		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, isrt ?
+				XFS_TRANS_DQ_RES_RTBLKS : XFS_TRANS_DQ_RES_BLKS,
 				-(long)ap->length);
 		return;
 	}
@@ -3302,10 +3307,12 @@ xfs_bmap_btalloc_accounting(
 	if (ap->wasdel) {
 		ap->ip->i_delayed_blks -= ap->length;
 		xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)ap->length);
+		fld = isrt ? XFS_TRANS_DQ_DELRTBCOUNT : XFS_TRANS_DQ_DELBCOUNT;
+	} else {
+		fld = isrt ? XFS_TRANS_DQ_RTBCOUNT : XFS_TRANS_DQ_BCOUNT;
 	}
-	xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
-		ap->wasdel ? XFS_TRANS_DQ_DELBCOUNT : XFS_TRANS_DQ_BCOUNT,
-		ap->length);
+
+	xfs_trans_mod_dquot_byino(ap->tp, ap->ip, fld, ap->length);
 }
 
 static int
@@ -3379,7 +3386,7 @@ xfs_bmap_process_allocated_extent(
 		ap->offset = orig_offset;
 	else if (ap->offset + ap->length < orig_offset + orig_length)
 		ap->offset = orig_offset + orig_length - ap->length;
-	xfs_bmap_btalloc_accounting(ap);
+	xfs_bmap_alloc_account(ap);
 }
 
 #ifdef DEBUG
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index e33470e39728d5..cb86f3d15abe9f 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -116,6 +116,8 @@ static inline int xfs_bmapi_whichfork(uint32_t bmapi_flags)
 	return XFS_DATA_FORK;
 }
 
+void xfs_bmap_alloc_account(struct xfs_bmalloca *ap);
+
 /*
  * Special values for xfs_bmbt_irec_t br_startblock field.
  */
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 731260a5af6db4..d6432a7ef2857d 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -168,17 +168,7 @@ xfs_bmap_rtalloc(
 	if (rtx != NULLRTEXTNO) {
 		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
 		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
-		ap->ip->i_nblocks += ap->length;
-		xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
-		if (ap->wasdel)
-			ap->ip->i_delayed_blks -= ap->length;
-		/*
-		 * Adjust the disk quota also. This was reserved
-		 * earlier.
-		 */
-		xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
-			ap->wasdel ? XFS_TRANS_DQ_DELRTBCOUNT :
-					XFS_TRANS_DQ_RTBCOUNT, ap->length);
+		xfs_bmap_alloc_account(ap);
 		return 0;
 	}
 
-- 
2.39.2

Re: [PATCH 04/19] xfs: also use xfs_bmap_btalloc_accounting for RT allocations

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:23AM +0100, Christoph Hellwig wrote:
> Make xfs_bmap_btalloc_accounting more generic by handling the RT quota
> reservations and then also use it from xfs_bmap_rtalloc instead of
> open coding the accounting logic there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Better than the copypasta that I came up with...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 21 ++++++++++++++-------
>  fs/xfs/libxfs/xfs_bmap.h |  2 ++
>  fs/xfs/xfs_bmap_util.c   | 12 +-----------
>  3 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index afdfb3455d9ebe..6722205949ad4c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3263,10 +3263,14 @@ xfs_bmap_btalloc_select_lengths(
>  }
>  
>  /* Update all inode and quota accounting for the allocation we just did. */
> -static void
> -xfs_bmap_btalloc_accounting(
> +void
> +xfs_bmap_alloc_account(
>  	struct xfs_bmalloca	*ap)
>  {
> +	bool			isrt = XFS_IS_REALTIME_INODE(ap->ip) &&
> +					(ap->flags & XFS_BMAPI_ATTRFORK);
> +	uint			fld;
> +
>  	if (ap->flags & XFS_BMAPI_COWFORK) {
>  		/*
>  		 * COW fork blocks are in-core only and thus are treated as
> @@ -3291,7 +3295,8 @@ xfs_bmap_btalloc_accounting(
>  		 * to that of a delalloc extent.
>  		 */
>  		ap->ip->i_delayed_blks += ap->length;
> -		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, XFS_TRANS_DQ_RES_BLKS,
> +		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, isrt ?
> +				XFS_TRANS_DQ_RES_RTBLKS : XFS_TRANS_DQ_RES_BLKS,
>  				-(long)ap->length);
>  		return;
>  	}
> @@ -3302,10 +3307,12 @@ xfs_bmap_btalloc_accounting(
>  	if (ap->wasdel) {
>  		ap->ip->i_delayed_blks -= ap->length;
>  		xfs_mod_delalloc(ap->ip->i_mount, -(int64_t)ap->length);
> +		fld = isrt ? XFS_TRANS_DQ_DELRTBCOUNT : XFS_TRANS_DQ_DELBCOUNT;
> +	} else {
> +		fld = isrt ? XFS_TRANS_DQ_RTBCOUNT : XFS_TRANS_DQ_BCOUNT;
>  	}
> -	xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
> -		ap->wasdel ? XFS_TRANS_DQ_DELBCOUNT : XFS_TRANS_DQ_BCOUNT,
> -		ap->length);
> +
> +	xfs_trans_mod_dquot_byino(ap->tp, ap->ip, fld, ap->length);
>  }
>  
>  static int
> @@ -3379,7 +3386,7 @@ xfs_bmap_process_allocated_extent(
>  		ap->offset = orig_offset;
>  	else if (ap->offset + ap->length < orig_offset + orig_length)
>  		ap->offset = orig_offset + orig_length - ap->length;
> -	xfs_bmap_btalloc_accounting(ap);
> +	xfs_bmap_alloc_account(ap);
>  }
>  
>  #ifdef DEBUG
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index e33470e39728d5..cb86f3d15abe9f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -116,6 +116,8 @@ static inline int xfs_bmapi_whichfork(uint32_t bmapi_flags)
>  	return XFS_DATA_FORK;
>  }
>  
> +void xfs_bmap_alloc_account(struct xfs_bmalloca *ap);
> +
>  /*
>   * Special values for xfs_bmbt_irec_t br_startblock field.
>   */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 731260a5af6db4..d6432a7ef2857d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -168,17 +168,7 @@ xfs_bmap_rtalloc(
>  	if (rtx != NULLRTEXTNO) {
>  		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
>  		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
> -		ap->ip->i_nblocks += ap->length;
> -		xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
> -		if (ap->wasdel)
> -			ap->ip->i_delayed_blks -= ap->length;
> -		/*
> -		 * Adjust the disk quota also. This was reserved
> -		 * earlier.
> -		 */
> -		xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
> -			ap->wasdel ? XFS_TRANS_DQ_DELRTBCOUNT :
> -					XFS_TRANS_DQ_RTBCOUNT, ap->length);
> +		xfs_bmap_alloc_account(ap);
>  		return 0;
>  	}
>  
> -- 
> 2.39.2
> 
> 

[PATCH 05/19] xfs: move xfs_bmap_rtalloc to xfs_rtalloc.c

[Christoph Hellwig]

xfs_bmap_rtalloc is currently in xfs_bmap_util.c, which is a somewhat
odd spot for it, given that is only called from xfs_bmap.c and calls
into xfs_rtalloc.c to do the actual work.  Move xfs_bmap_rtalloc to
xfs_rtalloc.c and mark xfs_rtpick_extent xfs_rtallocate_extent and
xfs_rtallocate_extent static now that they aren't called from outside
of xfs_rtalloc.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c | 131 ---------------------------------------
 fs/xfs/xfs_rtalloc.c   | 135 ++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_rtalloc.h   |  37 -----------
 3 files changed, 133 insertions(+), 170 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index d6432a7ef2857d..c2531c28905c09 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -69,137 +69,6 @@ xfs_zero_extent(
 		GFP_NOFS, 0);
 }
 
-#ifdef CONFIG_XFS_RT
-int
-xfs_bmap_rtalloc(
-	struct xfs_bmalloca	*ap)
-{
-	struct xfs_mount	*mp = ap->ip->i_mount;
-	xfs_fileoff_t		orig_offset = ap->offset;
-	xfs_rtxnum_t		rtx;
-	xfs_rtxlen_t		prod = 0;  /* product factor for allocators */
-	xfs_extlen_t		mod = 0;   /* product factor for allocators */
-	xfs_rtxlen_t		ralen = 0; /* realtime allocation length */
-	xfs_extlen_t		align;     /* minimum allocation alignment */
-	xfs_extlen_t		orig_length = ap->length;
-	xfs_extlen_t		minlen = mp->m_sb.sb_rextsize;
-	xfs_rtxlen_t		raminlen;
-	bool			rtlocked = false;
-	bool			ignore_locality = false;
-	int			error;
-
-	align = xfs_get_extsz_hint(ap->ip);
-retry:
-	prod = xfs_extlen_to_rtxlen(mp, align);
-	error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
-					align, 1, ap->eof, 0,
-					ap->conv, &ap->offset, &ap->length);
-	if (error)
-		return error;
-	ASSERT(ap->length);
-	ASSERT(xfs_extlen_to_rtxmod(mp, ap->length) == 0);
-
-	/*
-	 * If we shifted the file offset downward to satisfy an extent size
-	 * hint, increase minlen by that amount so that the allocator won't
-	 * give us an allocation that's too short to cover at least one of the
-	 * blocks that the caller asked for.
-	 */
-	if (ap->offset != orig_offset)
-		minlen += orig_offset - ap->offset;
-
-	/*
-	 * If the offset & length are not perfectly aligned
-	 * then kill prod, it will just get us in trouble.
-	 */
-	div_u64_rem(ap->offset, align, &mod);
-	if (mod || ap->length % align)
-		prod = 1;
-	/*
-	 * Set ralen to be the actual requested length in rtextents.
-	 *
-	 * If the old value was close enough to XFS_BMBT_MAX_EXTLEN that
-	 * we rounded up to it, cut it back so it's valid again.
-	 * Note that if it's a really large request (bigger than
-	 * XFS_BMBT_MAX_EXTLEN), we don't hear about that number, and can't
-	 * adjust the starting point to match it.
-	 */
-	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
-
-	/*
-	 * Lock out modifications to both the RT bitmap and summary inodes
-	 */
-	if (!rtlocked) {
-		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
-		xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
-		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
-		xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
-		rtlocked = true;
-	}
-
-	/*
-	 * If it's an allocation to an empty file at offset 0,
-	 * pick an extent that will space things out in the rt area.
-	 */
-	if (ap->eof && ap->offset == 0) {
-		error = xfs_rtpick_extent(mp, ap->tp, ralen, &rtx);
-		if (error)
-			return error;
-		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
-	} else {
-		ap->blkno = 0;
-	}
-
-	xfs_bmap_adjacent(ap);
-
-	/*
-	 * Realtime allocation, done through xfs_rtallocate_extent.
-	 */
-	if (ignore_locality)
-		rtx = 0;
-	else
-		rtx = xfs_rtb_to_rtx(mp, ap->blkno);
-	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
-	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
-			ap->wasdel, prod, &rtx);
-	if (error)
-		return error;
-
-	if (rtx != NULLRTEXTNO) {
-		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
-		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
-		xfs_bmap_alloc_account(ap);
-		return 0;
-	}
-
-	if (align > mp->m_sb.sb_rextsize) {
-		/*
-		 * We previously enlarged the request length to try to satisfy
-		 * an extent size hint.  The allocator didn't return anything,
-		 * so reset the parameters to the original values and try again
-		 * without alignment criteria.
-		 */
-		ap->offset = orig_offset;
-		ap->length = orig_length;
-		minlen = align = mp->m_sb.sb_rextsize;
-		goto retry;
-	}
-
-	if (!ignore_locality && ap->blkno != 0) {
-		/*
-		 * If we can't allocate near a specific rt extent, try again
-		 * without locality criteria.
-		 */
-		ignore_locality = true;
-		goto retry;
-	}
-
-	ap->blkno = NULLFSBLOCK;
-	ap->length = 0;
-	return 0;
-}
-#endif /* CONFIG_XFS_RT */
-
 /*
  * Extent tree block counting routines.
  */
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index fe98a96a26484f..74edea8579818d 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -14,12 +14,14 @@
 #include "xfs_inode.h"
 #include "xfs_bmap.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_bmap_util.h"
 #include "xfs_trans.h"
 #include "xfs_trans_space.h"
 #include "xfs_icache.h"
 #include "xfs_rtalloc.h"
 #include "xfs_sb.h"
 #include "xfs_rtbitmap.h"
+#include "xfs_quota.h"
 
 /*
  * Read and return the summary information for a given extent size,
@@ -1166,7 +1168,7 @@ xfs_growfs_rt(
  * parameters.  The length units are all in realtime extents, as is the
  * result block number.
  */
-int
+static int
 xfs_rtallocate_extent(
 	struct xfs_trans	*tp,
 	xfs_rtxnum_t		start,	/* starting rtext number to allocate */
@@ -1414,7 +1416,7 @@ xfs_rtunmount_inodes(
  * of rtextents and the fraction.
  * The fraction sequence is 0, 1/2, 1/4, 3/4, 1/8, ..., 7/8, 1/16, ...
  */
-int						/* error */
+static int
 xfs_rtpick_extent(
 	xfs_mount_t		*mp,		/* file system mount point */
 	xfs_trans_t		*tp,		/* transaction pointer */
@@ -1453,3 +1455,132 @@ xfs_rtpick_extent(
 	*pick = b;
 	return 0;
 }
+
+int
+xfs_bmap_rtalloc(
+	struct xfs_bmalloca	*ap)
+{
+	struct xfs_mount	*mp = ap->ip->i_mount;
+	xfs_fileoff_t		orig_offset = ap->offset;
+	xfs_rtxnum_t		rtx;
+	xfs_rtxlen_t		prod = 0;  /* product factor for allocators */
+	xfs_extlen_t		mod = 0;   /* product factor for allocators */
+	xfs_rtxlen_t		ralen = 0; /* realtime allocation length */
+	xfs_extlen_t		align;     /* minimum allocation alignment */
+	xfs_extlen_t		orig_length = ap->length;
+	xfs_extlen_t		minlen = mp->m_sb.sb_rextsize;
+	xfs_rtxlen_t		raminlen;
+	bool			rtlocked = false;
+	bool			ignore_locality = false;
+	int			error;
+
+	align = xfs_get_extsz_hint(ap->ip);
+retry:
+	prod = xfs_extlen_to_rtxlen(mp, align);
+	error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
+					align, 1, ap->eof, 0,
+					ap->conv, &ap->offset, &ap->length);
+	if (error)
+		return error;
+	ASSERT(ap->length);
+	ASSERT(xfs_extlen_to_rtxmod(mp, ap->length) == 0);
+
+	/*
+	 * If we shifted the file offset downward to satisfy an extent size
+	 * hint, increase minlen by that amount so that the allocator won't
+	 * give us an allocation that's too short to cover at least one of the
+	 * blocks that the caller asked for.
+	 */
+	if (ap->offset != orig_offset)
+		minlen += orig_offset - ap->offset;
+
+	/*
+	 * If the offset & length are not perfectly aligned
+	 * then kill prod, it will just get us in trouble.
+	 */
+	div_u64_rem(ap->offset, align, &mod);
+	if (mod || ap->length % align)
+		prod = 1;
+	/*
+	 * Set ralen to be the actual requested length in rtextents.
+	 *
+	 * If the old value was close enough to XFS_BMBT_MAX_EXTLEN that
+	 * we rounded up to it, cut it back so it's valid again.
+	 * Note that if it's a really large request (bigger than
+	 * XFS_BMBT_MAX_EXTLEN), we don't hear about that number, and can't
+	 * adjust the starting point to match it.
+	 */
+	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
+
+	/*
+	 * Lock out modifications to both the RT bitmap and summary inodes
+	 */
+	if (!rtlocked) {
+		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
+		xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
+		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
+		xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
+		rtlocked = true;
+	}
+
+	/*
+	 * If it's an allocation to an empty file at offset 0,
+	 * pick an extent that will space things out in the rt area.
+	 */
+	if (ap->eof && ap->offset == 0) {
+		error = xfs_rtpick_extent(mp, ap->tp, ralen, &rtx);
+		if (error)
+			return error;
+		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
+	} else {
+		ap->blkno = 0;
+	}
+
+	xfs_bmap_adjacent(ap);
+
+	/*
+	 * Realtime allocation, done through xfs_rtallocate_extent.
+	 */
+	if (ignore_locality)
+		rtx = 0;
+	else
+		rtx = xfs_rtb_to_rtx(mp, ap->blkno);
+	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
+	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
+			ap->wasdel, prod, &rtx);
+	if (error)
+		return error;
+
+	if (rtx != NULLRTEXTNO) {
+		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
+		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
+		xfs_bmap_alloc_account(ap);
+		return 0;
+	}
+
+	if (align > mp->m_sb.sb_rextsize) {
+		/*
+		 * We previously enlarged the request length to try to satisfy
+		 * an extent size hint.  The allocator didn't return anything,
+		 * so reset the parameters to the original values and try again
+		 * without alignment criteria.
+		 */
+		ap->offset = orig_offset;
+		ap->length = orig_length;
+		minlen = align = mp->m_sb.sb_rextsize;
+		goto retry;
+	}
+
+	if (!ignore_locality && ap->blkno != 0) {
+		/*
+		 * If we can't allocate near a specific rt extent, try again
+		 * without locality criteria.
+		 */
+		ignore_locality = true;
+		goto retry;
+	}
+
+	ap->blkno = NULLFSBLOCK;
+	ap->length = 0;
+	return 0;
+}
diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
index f7cb9ffe51ca68..a6836da9bebef5 100644
--- a/fs/xfs/xfs_rtalloc.h
+++ b/fs/xfs/xfs_rtalloc.h
@@ -12,27 +12,6 @@ struct xfs_mount;
 struct xfs_trans;
 
 #ifdef CONFIG_XFS_RT
-/*
- * Function prototypes for exported functions.
- */
-
-/*
- * Allocate an extent in the realtime subvolume, with the usual allocation
- * parameters.  The length units are all in realtime extents, as is the
- * result block number.
- */
-int					/* error */
-xfs_rtallocate_extent(
-	struct xfs_trans	*tp,	/* transaction pointer */
-	xfs_rtxnum_t		start,	/* starting rtext number to allocate */
-	xfs_rtxlen_t		minlen,	/* minimum length to allocate */
-	xfs_rtxlen_t		maxlen,	/* maximum length to allocate */
-	xfs_rtxlen_t		*len,	/* out: actual length allocated */
-	int			wasdel,	/* was a delayed allocation extent */
-	xfs_rtxlen_t		prod,	/* extent product factor */
-	xfs_rtxnum_t		*rtblock); /* out: start rtext allocated */
-
-
 /*
  * Initialize realtime fields in the mount structure.
  */
@@ -51,20 +30,6 @@ int					/* error */
 xfs_rtmount_inodes(
 	struct xfs_mount	*mp);	/* file system mount structure */
 
-/*
- * Pick an extent for allocation at the start of a new realtime file.
- * Use the sequence number stored in the atime field of the bitmap inode.
- * Translate this to a fraction of the rtextents, and return the product
- * of rtextents and the fraction.
- * The fraction sequence is 0, 1/2, 1/4, 3/4, 1/8, ..., 7/8, 1/16, ...
- */
-int					/* error */
-xfs_rtpick_extent(
-	struct xfs_mount	*mp,	/* file system mount point */
-	struct xfs_trans	*tp,	/* transaction pointer */
-	xfs_rtxlen_t		len,	/* allocation length (rtextents) */
-	xfs_rtxnum_t		*pick);	/* result rt extent */
-
 /*
  * Grow the realtime area of the filesystem.
  */
@@ -75,8 +40,6 @@ xfs_growfs_rt(
 
 int xfs_rtalloc_reinit_frextents(struct xfs_mount *mp);
 #else
-# define xfs_rtallocate_extent(t,b,min,max,l,f,p,rb)	(-ENOSYS)
-# define xfs_rtpick_extent(m,t,l,rb)			(-ENOSYS)
 # define xfs_growfs_rt(mp,in)				(-ENOSYS)
 # define xfs_rtalloc_reinit_frextents(m)		(0)
 static inline int		/* error */
-- 
2.39.2

Re: [PATCH 05/19] xfs: move xfs_bmap_rtalloc to xfs_rtalloc.c

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:24AM +0100, Christoph Hellwig wrote:
> xfs_bmap_rtalloc is currently in xfs_bmap_util.c, which is a somewhat
> odd spot for it, given that is only called from xfs_bmap.c and calls
> into xfs_rtalloc.c to do the actual work.  Move xfs_bmap_rtalloc to
> xfs_rtalloc.c and mark xfs_rtpick_extent xfs_rtallocate_extent and
> xfs_rtallocate_extent static now that they aren't called from outside
> of xfs_rtalloc.c.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I never understood why xfs_bmap_rtalloc was there either, aside from the
namespacing.  But even then, xfs_rtalloc_bmap?

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

--D

> ---
>  fs/xfs/xfs_bmap_util.c | 131 ---------------------------------------
>  fs/xfs/xfs_rtalloc.c   | 135 ++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_rtalloc.h   |  37 -----------
>  3 files changed, 133 insertions(+), 170 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index d6432a7ef2857d..c2531c28905c09 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -69,137 +69,6 @@ xfs_zero_extent(
>  		GFP_NOFS, 0);
>  }
>  
> -#ifdef CONFIG_XFS_RT
> -int
> -xfs_bmap_rtalloc(
> -	struct xfs_bmalloca	*ap)
> -{
> -	struct xfs_mount	*mp = ap->ip->i_mount;
> -	xfs_fileoff_t		orig_offset = ap->offset;
> -	xfs_rtxnum_t		rtx;
> -	xfs_rtxlen_t		prod = 0;  /* product factor for allocators */
> -	xfs_extlen_t		mod = 0;   /* product factor for allocators */
> -	xfs_rtxlen_t		ralen = 0; /* realtime allocation length */
> -	xfs_extlen_t		align;     /* minimum allocation alignment */
> -	xfs_extlen_t		orig_length = ap->length;
> -	xfs_extlen_t		minlen = mp->m_sb.sb_rextsize;
> -	xfs_rtxlen_t		raminlen;
> -	bool			rtlocked = false;
> -	bool			ignore_locality = false;
> -	int			error;
> -
> -	align = xfs_get_extsz_hint(ap->ip);
> -retry:
> -	prod = xfs_extlen_to_rtxlen(mp, align);
> -	error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> -					align, 1, ap->eof, 0,
> -					ap->conv, &ap->offset, &ap->length);
> -	if (error)
> -		return error;
> -	ASSERT(ap->length);
> -	ASSERT(xfs_extlen_to_rtxmod(mp, ap->length) == 0);
> -
> -	/*
> -	 * If we shifted the file offset downward to satisfy an extent size
> -	 * hint, increase minlen by that amount so that the allocator won't
> -	 * give us an allocation that's too short to cover at least one of the
> -	 * blocks that the caller asked for.
> -	 */
> -	if (ap->offset != orig_offset)
> -		minlen += orig_offset - ap->offset;
> -
> -	/*
> -	 * If the offset & length are not perfectly aligned
> -	 * then kill prod, it will just get us in trouble.
> -	 */
> -	div_u64_rem(ap->offset, align, &mod);
> -	if (mod || ap->length % align)
> -		prod = 1;
> -	/*
> -	 * Set ralen to be the actual requested length in rtextents.
> -	 *
> -	 * If the old value was close enough to XFS_BMBT_MAX_EXTLEN that
> -	 * we rounded up to it, cut it back so it's valid again.
> -	 * Note that if it's a really large request (bigger than
> -	 * XFS_BMBT_MAX_EXTLEN), we don't hear about that number, and can't
> -	 * adjust the starting point to match it.
> -	 */
> -	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
> -
> -	/*
> -	 * Lock out modifications to both the RT bitmap and summary inodes
> -	 */
> -	if (!rtlocked) {
> -		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> -		xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> -		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> -		xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> -		rtlocked = true;
> -	}
> -
> -	/*
> -	 * If it's an allocation to an empty file at offset 0,
> -	 * pick an extent that will space things out in the rt area.
> -	 */
> -	if (ap->eof && ap->offset == 0) {
> -		error = xfs_rtpick_extent(mp, ap->tp, ralen, &rtx);
> -		if (error)
> -			return error;
> -		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
> -	} else {
> -		ap->blkno = 0;
> -	}
> -
> -	xfs_bmap_adjacent(ap);
> -
> -	/*
> -	 * Realtime allocation, done through xfs_rtallocate_extent.
> -	 */
> -	if (ignore_locality)
> -		rtx = 0;
> -	else
> -		rtx = xfs_rtb_to_rtx(mp, ap->blkno);
> -	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
> -	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
> -			ap->wasdel, prod, &rtx);
> -	if (error)
> -		return error;
> -
> -	if (rtx != NULLRTEXTNO) {
> -		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
> -		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
> -		xfs_bmap_alloc_account(ap);
> -		return 0;
> -	}
> -
> -	if (align > mp->m_sb.sb_rextsize) {
> -		/*
> -		 * We previously enlarged the request length to try to satisfy
> -		 * an extent size hint.  The allocator didn't return anything,
> -		 * so reset the parameters to the original values and try again
> -		 * without alignment criteria.
> -		 */
> -		ap->offset = orig_offset;
> -		ap->length = orig_length;
> -		minlen = align = mp->m_sb.sb_rextsize;
> -		goto retry;
> -	}
> -
> -	if (!ignore_locality && ap->blkno != 0) {
> -		/*
> -		 * If we can't allocate near a specific rt extent, try again
> -		 * without locality criteria.
> -		 */
> -		ignore_locality = true;
> -		goto retry;
> -	}
> -
> -	ap->blkno = NULLFSBLOCK;
> -	ap->length = 0;
> -	return 0;
> -}
> -#endif /* CONFIG_XFS_RT */
> -
>  /*
>   * Extent tree block counting routines.
>   */
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index fe98a96a26484f..74edea8579818d 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -14,12 +14,14 @@
>  #include "xfs_inode.h"
>  #include "xfs_bmap.h"
>  #include "xfs_bmap_btree.h"
> +#include "xfs_bmap_util.h"
>  #include "xfs_trans.h"
>  #include "xfs_trans_space.h"
>  #include "xfs_icache.h"
>  #include "xfs_rtalloc.h"
>  #include "xfs_sb.h"
>  #include "xfs_rtbitmap.h"
> +#include "xfs_quota.h"
>  
>  /*
>   * Read and return the summary information for a given extent size,
> @@ -1166,7 +1168,7 @@ xfs_growfs_rt(
>   * parameters.  The length units are all in realtime extents, as is the
>   * result block number.
>   */
> -int
> +static int
>  xfs_rtallocate_extent(
>  	struct xfs_trans	*tp,
>  	xfs_rtxnum_t		start,	/* starting rtext number to allocate */
> @@ -1414,7 +1416,7 @@ xfs_rtunmount_inodes(
>   * of rtextents and the fraction.
>   * The fraction sequence is 0, 1/2, 1/4, 3/4, 1/8, ..., 7/8, 1/16, ...
>   */
> -int						/* error */
> +static int
>  xfs_rtpick_extent(
>  	xfs_mount_t		*mp,		/* file system mount point */
>  	xfs_trans_t		*tp,		/* transaction pointer */
> @@ -1453,3 +1455,132 @@ xfs_rtpick_extent(
>  	*pick = b;
>  	return 0;
>  }
> +
> +int
> +xfs_bmap_rtalloc(
> +	struct xfs_bmalloca	*ap)
> +{
> +	struct xfs_mount	*mp = ap->ip->i_mount;
> +	xfs_fileoff_t		orig_offset = ap->offset;
> +	xfs_rtxnum_t		rtx;
> +	xfs_rtxlen_t		prod = 0;  /* product factor for allocators */
> +	xfs_extlen_t		mod = 0;   /* product factor for allocators */
> +	xfs_rtxlen_t		ralen = 0; /* realtime allocation length */
> +	xfs_extlen_t		align;     /* minimum allocation alignment */
> +	xfs_extlen_t		orig_length = ap->length;
> +	xfs_extlen_t		minlen = mp->m_sb.sb_rextsize;
> +	xfs_rtxlen_t		raminlen;
> +	bool			rtlocked = false;
> +	bool			ignore_locality = false;
> +	int			error;
> +
> +	align = xfs_get_extsz_hint(ap->ip);
> +retry:
> +	prod = xfs_extlen_to_rtxlen(mp, align);
> +	error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> +					align, 1, ap->eof, 0,
> +					ap->conv, &ap->offset, &ap->length);
> +	if (error)
> +		return error;
> +	ASSERT(ap->length);
> +	ASSERT(xfs_extlen_to_rtxmod(mp, ap->length) == 0);
> +
> +	/*
> +	 * If we shifted the file offset downward to satisfy an extent size
> +	 * hint, increase minlen by that amount so that the allocator won't
> +	 * give us an allocation that's too short to cover at least one of the
> +	 * blocks that the caller asked for.
> +	 */
> +	if (ap->offset != orig_offset)
> +		minlen += orig_offset - ap->offset;
> +
> +	/*
> +	 * If the offset & length are not perfectly aligned
> +	 * then kill prod, it will just get us in trouble.
> +	 */
> +	div_u64_rem(ap->offset, align, &mod);
> +	if (mod || ap->length % align)
> +		prod = 1;
> +	/*
> +	 * Set ralen to be the actual requested length in rtextents.
> +	 *
> +	 * If the old value was close enough to XFS_BMBT_MAX_EXTLEN that
> +	 * we rounded up to it, cut it back so it's valid again.
> +	 * Note that if it's a really large request (bigger than
> +	 * XFS_BMBT_MAX_EXTLEN), we don't hear about that number, and can't
> +	 * adjust the starting point to match it.
> +	 */
> +	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
> +
> +	/*
> +	 * Lock out modifications to both the RT bitmap and summary inodes
> +	 */
> +	if (!rtlocked) {
> +		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> +		xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> +		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> +		xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> +		rtlocked = true;
> +	}
> +
> +	/*
> +	 * If it's an allocation to an empty file at offset 0,
> +	 * pick an extent that will space things out in the rt area.
> +	 */
> +	if (ap->eof && ap->offset == 0) {
> +		error = xfs_rtpick_extent(mp, ap->tp, ralen, &rtx);
> +		if (error)
> +			return error;
> +		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
> +	} else {
> +		ap->blkno = 0;
> +	}
> +
> +	xfs_bmap_adjacent(ap);
> +
> +	/*
> +	 * Realtime allocation, done through xfs_rtallocate_extent.
> +	 */
> +	if (ignore_locality)
> +		rtx = 0;
> +	else
> +		rtx = xfs_rtb_to_rtx(mp, ap->blkno);
> +	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
> +	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
> +			ap->wasdel, prod, &rtx);
> +	if (error)
> +		return error;
> +
> +	if (rtx != NULLRTEXTNO) {
> +		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
> +		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
> +		xfs_bmap_alloc_account(ap);
> +		return 0;
> +	}
> +
> +	if (align > mp->m_sb.sb_rextsize) {
> +		/*
> +		 * We previously enlarged the request length to try to satisfy
> +		 * an extent size hint.  The allocator didn't return anything,
> +		 * so reset the parameters to the original values and try again
> +		 * without alignment criteria.
> +		 */
> +		ap->offset = orig_offset;
> +		ap->length = orig_length;
> +		minlen = align = mp->m_sb.sb_rextsize;
> +		goto retry;
> +	}
> +
> +	if (!ignore_locality && ap->blkno != 0) {
> +		/*
> +		 * If we can't allocate near a specific rt extent, try again
> +		 * without locality criteria.
> +		 */
> +		ignore_locality = true;
> +		goto retry;
> +	}
> +
> +	ap->blkno = NULLFSBLOCK;
> +	ap->length = 0;
> +	return 0;
> +}
> diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> index f7cb9ffe51ca68..a6836da9bebef5 100644
> --- a/fs/xfs/xfs_rtalloc.h
> +++ b/fs/xfs/xfs_rtalloc.h
> @@ -12,27 +12,6 @@ struct xfs_mount;
>  struct xfs_trans;
>  
>  #ifdef CONFIG_XFS_RT
> -/*
> - * Function prototypes for exported functions.
> - */
> -
> -/*
> - * Allocate an extent in the realtime subvolume, with the usual allocation
> - * parameters.  The length units are all in realtime extents, as is the
> - * result block number.
> - */
> -int					/* error */
> -xfs_rtallocate_extent(
> -	struct xfs_trans	*tp,	/* transaction pointer */
> -	xfs_rtxnum_t		start,	/* starting rtext number to allocate */
> -	xfs_rtxlen_t		minlen,	/* minimum length to allocate */
> -	xfs_rtxlen_t		maxlen,	/* maximum length to allocate */
> -	xfs_rtxlen_t		*len,	/* out: actual length allocated */
> -	int			wasdel,	/* was a delayed allocation extent */
> -	xfs_rtxlen_t		prod,	/* extent product factor */
> -	xfs_rtxnum_t		*rtblock); /* out: start rtext allocated */
> -
> -
>  /*
>   * Initialize realtime fields in the mount structure.
>   */
> @@ -51,20 +30,6 @@ int					/* error */
>  xfs_rtmount_inodes(
>  	struct xfs_mount	*mp);	/* file system mount structure */
>  
> -/*
> - * Pick an extent for allocation at the start of a new realtime file.
> - * Use the sequence number stored in the atime field of the bitmap inode.
> - * Translate this to a fraction of the rtextents, and return the product
> - * of rtextents and the fraction.
> - * The fraction sequence is 0, 1/2, 1/4, 3/4, 1/8, ..., 7/8, 1/16, ...
> - */
> -int					/* error */
> -xfs_rtpick_extent(
> -	struct xfs_mount	*mp,	/* file system mount point */
> -	struct xfs_trans	*tp,	/* transaction pointer */
> -	xfs_rtxlen_t		len,	/* allocation length (rtextents) */
> -	xfs_rtxnum_t		*pick);	/* result rt extent */
> -
>  /*
>   * Grow the realtime area of the filesystem.
>   */
> @@ -75,8 +40,6 @@ xfs_growfs_rt(
>  
>  int xfs_rtalloc_reinit_frextents(struct xfs_mount *mp);
>  #else
> -# define xfs_rtallocate_extent(t,b,min,max,l,f,p,rb)	(-ENOSYS)
> -# define xfs_rtpick_extent(m,t,l,rb)			(-ENOSYS)
>  # define xfs_growfs_rt(mp,in)				(-ENOSYS)
>  # define xfs_rtalloc_reinit_frextents(m)		(0)
>  static inline int		/* error */
> -- 
> 2.39.2
> 
> 

Re: [PATCH 05/19] xfs: move xfs_bmap_rtalloc to xfs_rtalloc.c

[Christoph Hellwig]

On Thu, Dec 14, 2023 at 12:48:38PM -0800, Darrick J. Wong wrote:
> On Thu, Dec 14, 2023 at 07:34:24AM +0100, Christoph Hellwig wrote:
> > xfs_bmap_rtalloc is currently in xfs_bmap_util.c, which is a somewhat
> > odd spot for it, given that is only called from xfs_bmap.c and calls
> > into xfs_rtalloc.c to do the actual work.  Move xfs_bmap_rtalloc to
> > xfs_rtalloc.c and mark xfs_rtpick_extent xfs_rtallocate_extent and
> > xfs_rtallocate_extent static now that they aren't called from outside
> > of xfs_rtalloc.c.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I never understood why xfs_bmap_rtalloc was there either, aside from the
> namespacing.  But even then, xfs_rtalloc_bmap?

Fine with me..

Re: [PATCH 05/19] xfs: move xfs_bmap_rtalloc to xfs_rtalloc.c

[Christoph Hellwig]

On Fri, Dec 15, 2023 at 05:09:07AM +0100, Christoph Hellwig wrote:
> On Thu, Dec 14, 2023 at 12:48:38PM -0800, Darrick J. Wong wrote:
> > On Thu, Dec 14, 2023 at 07:34:24AM +0100, Christoph Hellwig wrote:
> > > xfs_bmap_rtalloc is currently in xfs_bmap_util.c, which is a somewhat
> > > odd spot for it, given that is only called from xfs_bmap.c and calls
> > > into xfs_rtalloc.c to do the actual work.  Move xfs_bmap_rtalloc to
> > > xfs_rtalloc.c and mark xfs_rtpick_extent xfs_rtallocate_extent and
> > > xfs_rtallocate_extent static now that they aren't called from outside
> > > of xfs_rtalloc.c.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > I never understood why xfs_bmap_rtalloc was there either, aside from the
> > namespacing.  But even then, xfs_rtalloc_bmap?
> 
> Fine with me..

Actually..  Given that it purely is a block allocator and doesn't
do any bmapping at all, the _bmap postfix is rather odd.

Something like xfs_rtallocate_extent would fit better, but the name only
becomes available after the last patch.

[PATCH 06/19] xfs: return -ENOSPC from xfs_rtallocate_*

[Christoph Hellwig]

Just return -ENOSPC instead of returning 0 and setting the return rt
extent number to NULLRTEXTNO.  This is turn removes all users of
NULLRTEXTNO, so remove that as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_types.h |   1 -
 fs/xfs/xfs_rtalloc.c      | 211 +++++++++++++-------------------------
 2 files changed, 71 insertions(+), 141 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 533200c4ccc25a..c3636ea21ecd05 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -51,7 +51,6 @@ typedef void *		xfs_failaddr_t;
 #define	NULLRFSBLOCK	((xfs_rfsblock_t)-1)
 #define	NULLRTBLOCK	((xfs_rtblock_t)-1)
 #define	NULLFILEOFF	((xfs_fileoff_t)-1)
-#define	NULLRTEXTNO	((xfs_rtxnum_t)-1)
 
 #define	NULLAGBLOCK	((xfs_agblock_t)-1)
 #define	NULLAGNUMBER	((xfs_agnumber_t)-1)
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 74edea8579818d..dac148d53af3ec 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -156,17 +156,17 @@ xfs_rtallocate_range(
 	 * properly update the summary.
 	 */
 	error = xfs_rtfind_back(args, start, 0, &preblock);
-	if (error) {
+	if (error)
 		return error;
-	}
+
 	/*
 	 * Find the next allocated block (end of free extent).
 	 */
 	error = xfs_rtfind_forw(args, end, mp->m_sb.sb_rextents - 1,
 			&postblock);
-	if (error) {
+	if (error)
 		return error;
-	}
+
 	/*
 	 * Decrement the summary information corresponding to the entire
 	 * (old) free extent.
@@ -174,9 +174,9 @@ xfs_rtallocate_range(
 	error = xfs_rtmodify_summary(args,
 			XFS_RTBLOCKLOG(postblock + 1 - preblock),
 			xfs_rtx_to_rbmblock(mp, preblock), -1);
-	if (error) {
+	if (error)
 		return error;
-	}
+
 	/*
 	 * If there are blocks not being allocated at the front of the
 	 * old extent, add summary data for them to be free.
@@ -185,10 +185,10 @@ xfs_rtallocate_range(
 		error = xfs_rtmodify_summary(args,
 				XFS_RTBLOCKLOG(start - preblock),
 				xfs_rtx_to_rbmblock(mp, preblock), 1);
-		if (error) {
+		if (error)
 			return error;
-		}
 	}
+
 	/*
 	 * If there are blocks not being allocated at the end of the
 	 * old extent, add summary data for them to be free.
@@ -197,15 +197,14 @@ xfs_rtallocate_range(
 		error = xfs_rtmodify_summary(args,
 				XFS_RTBLOCKLOG(postblock - end),
 				xfs_rtx_to_rbmblock(mp, end + 1), 1);
-		if (error) {
+		if (error)
 			return error;
-		}
 	}
+
 	/*
 	 * Modify the bitmap to mark this extent allocated.
 	 */
-	error = xfs_rtmodify_range(args, start, len, 0);
-	return error;
+	return xfs_rtmodify_range(args, start, len, 0);
 }
 
 /*
@@ -267,17 +266,17 @@ xfs_rtallocate_extent_block(
 		 * If it's not so then next will contain the first non-free.
 		 */
 		error = xfs_rtcheck_range(args, i, maxlen, 1, &next, &stat);
-		if (error) {
+		if (error)
 			return error;
-		}
+
 		if (stat) {
 			/*
 			 * i for maxlen is all free, allocate and return that.
 			 */
 			error = xfs_rtallocate_range(args, i, maxlen);
-			if (error) {
+			if (error)
 				return error;
-			}
+
 			*len = maxlen;
 			*rtx = i;
 			return 0;
@@ -302,9 +301,8 @@ xfs_rtallocate_extent_block(
 		 */
 		if (next < end) {
 			error = xfs_rtfind_forw(args, next, end, &i);
-			if (error) {
+			if (error)
 				return error;
-			}
 		} else
 			break;
 	}
@@ -327,9 +325,8 @@ xfs_rtallocate_extent_block(
 		 * Allocate besti for bestlen & return that.
 		 */
 		error = xfs_rtallocate_range(args, besti, bestlen);
-		if (error) {
+		if (error)
 			return error;
-		}
 		*len = bestlen;
 		*rtx = besti;
 		return 0;
@@ -338,8 +335,7 @@ xfs_rtallocate_extent_block(
 	 * Allocation failed.  Set *nextp to the next block to try.
 	 */
 	*nextp = next;
-	*rtx = NULLRTEXTNO;
-	return 0;
+	return -ENOSPC;
 }
 
 /*
@@ -369,17 +365,16 @@ xfs_rtallocate_extent_exact(
 	 * Check if the range in question (for maxlen) is free.
 	 */
 	error = xfs_rtcheck_range(args, start, maxlen, 1, &next, &isfree);
-	if (error) {
+	if (error)
 		return error;
-	}
+
 	if (isfree) {
 		/*
 		 * If it is, allocate it and return success.
 		 */
 		error = xfs_rtallocate_range(args, start, maxlen);
-		if (error) {
+		if (error)
 			return error;
-		}
 		*len = maxlen;
 		*rtx = start;
 		return 0;
@@ -388,33 +383,23 @@ xfs_rtallocate_extent_exact(
 	 * If not, allocate what there is, if it's at least minlen.
 	 */
 	maxlen = next - start;
-	if (maxlen < minlen) {
-		/*
-		 * Failed, return failure status.
-		 */
-		*rtx = NULLRTEXTNO;
-		return 0;
-	}
+	if (maxlen < minlen)
+		return -ENOSPC;
+
 	/*
 	 * Trim off tail of extent, if prod is specified.
 	 */
 	if (prod > 1 && (i = maxlen % prod)) {
 		maxlen -= i;
-		if (maxlen < minlen) {
-			/*
-			 * Now we can't do it, return failure status.
-			 */
-			*rtx = NULLRTEXTNO;
-			return 0;
-		}
+		if (maxlen < minlen)
+			return -ENOSPC;
 	}
 	/*
 	 * Allocate what we can and return it.
 	 */
 	error = xfs_rtallocate_range(args, start, maxlen);
-	if (error) {
+	if (error)
 		return error;
-	}
 	*len = maxlen;
 	*rtx = start;
 	return 0;
@@ -443,7 +428,6 @@ xfs_rtallocate_extent_near(
 	int			j;	/* secondary loop control */
 	int			log2len; /* log2 of minlen */
 	xfs_rtxnum_t		n;	/* next rtext to try */
-	xfs_rtxnum_t		r;	/* result rtext */
 
 	ASSERT(minlen % prod == 0);
 	ASSERT(maxlen % prod == 0);
@@ -457,26 +441,18 @@ xfs_rtallocate_extent_near(
 
 	/* Make sure we don't run off the end of the rt volume. */
 	maxlen = xfs_rtallocate_clamp_len(mp, start, maxlen, prod);
-	if (maxlen < minlen) {
-		*rtx = NULLRTEXTNO;
-		return 0;
-	}
+	if (maxlen < minlen)
+		return -ENOSPC;
 
 	/*
 	 * Try the exact allocation first.
 	 */
 	error = xfs_rtallocate_extent_exact(args, start, minlen, maxlen, len,
-			prod, &r);
-	if (error) {
+			prod, rtx);
+	if (error != -ENOSPC)
 		return error;
-	}
-	/*
-	 * If the exact allocation worked, return that.
-	 */
-	if (r != NULLRTEXTNO) {
-		*rtx = r;
-		return 0;
-	}
+
+
 	bbno = xfs_rtx_to_rbmblock(mp, start);
 	i = 0;
 	j = -1;
@@ -492,9 +468,9 @@ xfs_rtallocate_extent_near(
 		 */
 		error = xfs_rtany_summary(args, log2len, mp->m_rsumlevels - 1,
 				bbno + i, &maxlog);
-		if (error) {
+		if (error)
 			return error;
-		}
+
 		/*
 		 * If there are any useful extents starting here, try
 		 * allocating one.
@@ -513,17 +489,9 @@ xfs_rtallocate_extent_near(
 				 */
 				error = xfs_rtallocate_extent_block(args,
 						bbno + i, minlen, maxavail, len,
-						&n, prod, &r);
-				if (error) {
+						&n, prod, rtx);
+				if (error != -ENOSPC)
 					return error;
-				}
-				/*
-				 * If it worked, return it.
-				 */
-				if (r != NULLRTEXTNO) {
-					*rtx = r;
-					return 0;
-				}
 			}
 			/*
 			 * On the negative side of the starting location.
@@ -557,17 +525,9 @@ xfs_rtallocate_extent_near(
 					error = xfs_rtallocate_extent_block(args,
 							bbno + j, minlen,
 							maxavail, len, &n, prod,
-							&r);
-					if (error) {
+							rtx);
+					if (error != -ENOSPC)
 						return error;
-					}
-					/*
-					 * If it works, return the extent.
-					 */
-					if (r != NULLRTEXTNO) {
-						*rtx = r;
-						return 0;
-					}
 				}
 			}
 		}
@@ -601,8 +561,7 @@ xfs_rtallocate_extent_near(
 		else
 			break;
 	}
-	*rtx = NULLRTEXTNO;
-	return 0;
+	return -ENOSPC;
 }
 
 /*
@@ -624,7 +583,6 @@ xfs_rtallocate_extent_size(
 	xfs_fileoff_t		i;	/* bitmap block number */
 	int			l;	/* level number (loop control) */
 	xfs_rtxnum_t		n;	/* next rtext to be tried */
-	xfs_rtxnum_t		r;	/* result rtext number */
 	xfs_suminfo_t		sum;	/* summary information for extents */
 
 	ASSERT(minlen % prod == 0);
@@ -647,9 +605,8 @@ xfs_rtallocate_extent_size(
 			 * Get the summary for this level/block.
 			 */
 			error = xfs_rtget_summary(args, l, i, &sum);
-			if (error) {
+			if (error)
 				return error;
-			}
 			/*
 			 * Nothing there, on to the next block.
 			 */
@@ -659,17 +616,9 @@ xfs_rtallocate_extent_size(
 			 * Try allocating the extent.
 			 */
 			error = xfs_rtallocate_extent_block(args, i, maxlen,
-					maxlen, len, &n, prod, &r);
-			if (error) {
+					maxlen, len, &n, prod, rtx);
+			if (error != -ENOSPC)
 				return error;
-			}
-			/*
-			 * If it worked, return that.
-			 */
-			if (r != NULLRTEXTNO) {
-				*rtx = r;
-				return 0;
-			}
 			/*
 			 * If the "next block to try" returned from the
 			 * allocator is beyond the next bitmap block,
@@ -683,10 +632,8 @@ xfs_rtallocate_extent_size(
 	 * Didn't find any maxlen blocks.  Try smaller ones, unless
 	 * we're asking for a fixed size extent.
 	 */
-	if (minlen > --maxlen) {
-		*rtx = NULLRTEXTNO;
-		return 0;
-	}
+	if (minlen > --maxlen)
+		return -ENOSPC;
 	ASSERT(minlen != 0);
 	ASSERT(maxlen != 0);
 
@@ -705,9 +652,9 @@ xfs_rtallocate_extent_size(
 			 * Get the summary information for this level/block.
 			 */
 			error =	xfs_rtget_summary(args, l, i, &sum);
-			if (error) {
+			if (error)
 				return error;
-			}
+
 			/*
 			 * If nothing there, go on to next.
 			 */
@@ -721,17 +668,10 @@ xfs_rtallocate_extent_size(
 			error = xfs_rtallocate_extent_block(args, i,
 					XFS_RTMAX(minlen, 1 << l),
 					XFS_RTMIN(maxlen, (1 << (l + 1)) - 1),
-					len, &n, prod, &r);
-			if (error) {
+					len, &n, prod, rtx);
+			if (error != -ENOSPC)
 				return error;
-			}
-			/*
-			 * If it worked, return that extent.
-			 */
-			if (r != NULLRTEXTNO) {
-				*rtx = r;
-				return 0;
-			}
+
 			/*
 			 * If the "next block to try" returned from the
 			 * allocator is beyond the next bitmap block,
@@ -744,8 +684,7 @@ xfs_rtallocate_extent_size(
 	/*
 	 * Got nothing, return failure.
 	 */
-	*rtx = NULLRTEXTNO;
-	return 0;
+	return -ENOSPC;
 }
 
 /*
@@ -1177,14 +1116,13 @@ xfs_rtallocate_extent(
 	xfs_rtxlen_t		*len,	/* out: actual length allocated */
 	int			wasdel,	/* was a delayed allocation extent */
 	xfs_rtxlen_t		prod,	/* extent product factor */
-	xfs_rtxnum_t		*rtblock) /* out: start rtext allocated */
+	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
 {
 	struct xfs_rtalloc_args	args = {
 		.mp		= tp->t_mountp,
 		.tp		= tp,
 	};
 	int			error;	/* error value */
-	xfs_rtxnum_t		r;	/* result allocated rtext */
 
 	ASSERT(xfs_isilocked(args.mp->m_rbmip, XFS_ILOCK_EXCL));
 	ASSERT(minlen > 0 && minlen <= maxlen);
@@ -1199,42 +1137,35 @@ xfs_rtallocate_extent(
 			maxlen -= i;
 		if ((i = minlen % prod))
 			minlen += prod - i;
-		if (maxlen < minlen) {
-			*rtblock = NULLRTEXTNO;
-			return 0;
-		}
+		if (maxlen < minlen)
+			return -ENOSPC;
 	}
 
 retry:
 	if (start == 0) {
 		error = xfs_rtallocate_extent_size(&args, minlen,
-				maxlen, len, prod, &r);
+				maxlen, len, prod, rtx);
 	} else {
 		error = xfs_rtallocate_extent_near(&args, start, minlen,
-				maxlen, len, prod, &r);
+				maxlen, len, prod, rtx);
 	}
-
 	xfs_rtbuf_cache_relse(&args);
-	if (error)
+	if (error) {
+		if (error == -ENOSPC && prod > 1) {
+			prod = 1;
+			goto retry;
+		}
 		return error;
+	}
 
 	/*
 	 * If it worked, update the superblock.
 	 */
-	if (r != NULLRTEXTNO) {
-		long	slen = (long)*len;
-
-		ASSERT(*len >= minlen && *len <= maxlen);
-		if (wasdel)
-			xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FREXTENTS, -slen);
-		else
-			xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, -slen);
-	} else if (prod > 1) {
-		prod = 1;
-		goto retry;
-	}
-
-	*rtblock = r;
+	ASSERT(*len >= minlen && *len <= maxlen);
+	if (wasdel)
+		xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FREXTENTS, -(long)*len);
+	else
+		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, -(long)*len);
 	return 0;
 }
 
@@ -1548,16 +1479,16 @@ xfs_bmap_rtalloc(
 	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
 	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
 			ap->wasdel, prod, &rtx);
-	if (error)
-		return error;
-
-	if (rtx != NULLRTEXTNO) {
+	if (!error) {
 		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
 		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
 		xfs_bmap_alloc_account(ap);
 		return 0;
 	}
 
+	if (error != -ENOSPC)
+		return error;
+
 	if (align > mp->m_sb.sb_rextsize) {
 		/*
 		 * We previously enlarged the request length to try to satisfy
-- 
2.39.2

Re: [PATCH 06/19] xfs: return -ENOSPC from xfs_rtallocate_*

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:25AM +0100, Christoph Hellwig wrote:
> Just return -ENOSPC instead of returning 0 and setting the return rt
> extent number to NULLRTEXTNO.  This is turn removes all users of
> NULLRTEXTNO, so remove that as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_types.h |   1 -
>  fs/xfs/xfs_rtalloc.c      | 211 +++++++++++++-------------------------
>  2 files changed, 71 insertions(+), 141 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 533200c4ccc25a..c3636ea21ecd05 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -51,7 +51,6 @@ typedef void *		xfs_failaddr_t;
>  #define	NULLRFSBLOCK	((xfs_rfsblock_t)-1)
>  #define	NULLRTBLOCK	((xfs_rtblock_t)-1)
>  #define	NULLFILEOFF	((xfs_fileoff_t)-1)
> -#define	NULLRTEXTNO	((xfs_rtxnum_t)-1)

Getting rid of this cleans out a nice quantity of if test cruft in the
rt modernization series, so...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  #define	NULLAGBLOCK	((xfs_agblock_t)-1)
>  #define	NULLAGNUMBER	((xfs_agnumber_t)-1)
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 74edea8579818d..dac148d53af3ec 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -156,17 +156,17 @@ xfs_rtallocate_range(
>  	 * properly update the summary.
>  	 */
>  	error = xfs_rtfind_back(args, start, 0, &preblock);
> -	if (error) {
> +	if (error)
>  		return error;
> -	}
> +
>  	/*
>  	 * Find the next allocated block (end of free extent).
>  	 */
>  	error = xfs_rtfind_forw(args, end, mp->m_sb.sb_rextents - 1,
>  			&postblock);
> -	if (error) {
> +	if (error)
>  		return error;
> -	}
> +
>  	/*
>  	 * Decrement the summary information corresponding to the entire
>  	 * (old) free extent.
> @@ -174,9 +174,9 @@ xfs_rtallocate_range(
>  	error = xfs_rtmodify_summary(args,
>  			XFS_RTBLOCKLOG(postblock + 1 - preblock),
>  			xfs_rtx_to_rbmblock(mp, preblock), -1);
> -	if (error) {
> +	if (error)
>  		return error;
> -	}
> +
>  	/*
>  	 * If there are blocks not being allocated at the front of the
>  	 * old extent, add summary data for them to be free.
> @@ -185,10 +185,10 @@ xfs_rtallocate_range(
>  		error = xfs_rtmodify_summary(args,
>  				XFS_RTBLOCKLOG(start - preblock),
>  				xfs_rtx_to_rbmblock(mp, preblock), 1);
> -		if (error) {
> +		if (error)
>  			return error;
> -		}
>  	}
> +
>  	/*
>  	 * If there are blocks not being allocated at the end of the
>  	 * old extent, add summary data for them to be free.
> @@ -197,15 +197,14 @@ xfs_rtallocate_range(
>  		error = xfs_rtmodify_summary(args,
>  				XFS_RTBLOCKLOG(postblock - end),
>  				xfs_rtx_to_rbmblock(mp, end + 1), 1);
> -		if (error) {
> +		if (error)
>  			return error;
> -		}
>  	}
> +
>  	/*
>  	 * Modify the bitmap to mark this extent allocated.
>  	 */
> -	error = xfs_rtmodify_range(args, start, len, 0);
> -	return error;
> +	return xfs_rtmodify_range(args, start, len, 0);
>  }
>  
>  /*
> @@ -267,17 +266,17 @@ xfs_rtallocate_extent_block(
>  		 * If it's not so then next will contain the first non-free.
>  		 */
>  		error = xfs_rtcheck_range(args, i, maxlen, 1, &next, &stat);
> -		if (error) {
> +		if (error)
>  			return error;
> -		}
> +
>  		if (stat) {
>  			/*
>  			 * i for maxlen is all free, allocate and return that.
>  			 */
>  			error = xfs_rtallocate_range(args, i, maxlen);
> -			if (error) {
> +			if (error)
>  				return error;
> -			}
> +
>  			*len = maxlen;
>  			*rtx = i;
>  			return 0;
> @@ -302,9 +301,8 @@ xfs_rtallocate_extent_block(
>  		 */
>  		if (next < end) {
>  			error = xfs_rtfind_forw(args, next, end, &i);
> -			if (error) {
> +			if (error)
>  				return error;
> -			}
>  		} else
>  			break;
>  	}
> @@ -327,9 +325,8 @@ xfs_rtallocate_extent_block(
>  		 * Allocate besti for bestlen & return that.
>  		 */
>  		error = xfs_rtallocate_range(args, besti, bestlen);
> -		if (error) {
> +		if (error)
>  			return error;
> -		}
>  		*len = bestlen;
>  		*rtx = besti;
>  		return 0;
> @@ -338,8 +335,7 @@ xfs_rtallocate_extent_block(
>  	 * Allocation failed.  Set *nextp to the next block to try.
>  	 */
>  	*nextp = next;
> -	*rtx = NULLRTEXTNO;
> -	return 0;
> +	return -ENOSPC;
>  }
>  
>  /*
> @@ -369,17 +365,16 @@ xfs_rtallocate_extent_exact(
>  	 * Check if the range in question (for maxlen) is free.
>  	 */
>  	error = xfs_rtcheck_range(args, start, maxlen, 1, &next, &isfree);
> -	if (error) {
> +	if (error)
>  		return error;
> -	}
> +
>  	if (isfree) {
>  		/*
>  		 * If it is, allocate it and return success.
>  		 */
>  		error = xfs_rtallocate_range(args, start, maxlen);
> -		if (error) {
> +		if (error)
>  			return error;
> -		}
>  		*len = maxlen;
>  		*rtx = start;
>  		return 0;
> @@ -388,33 +383,23 @@ xfs_rtallocate_extent_exact(
>  	 * If not, allocate what there is, if it's at least minlen.
>  	 */
>  	maxlen = next - start;
> -	if (maxlen < minlen) {
> -		/*
> -		 * Failed, return failure status.
> -		 */
> -		*rtx = NULLRTEXTNO;
> -		return 0;
> -	}
> +	if (maxlen < minlen)
> +		return -ENOSPC;
> +
>  	/*
>  	 * Trim off tail of extent, if prod is specified.
>  	 */
>  	if (prod > 1 && (i = maxlen % prod)) {
>  		maxlen -= i;
> -		if (maxlen < minlen) {
> -			/*
> -			 * Now we can't do it, return failure status.
> -			 */
> -			*rtx = NULLRTEXTNO;
> -			return 0;
> -		}
> +		if (maxlen < minlen)
> +			return -ENOSPC;
>  	}
>  	/*
>  	 * Allocate what we can and return it.
>  	 */
>  	error = xfs_rtallocate_range(args, start, maxlen);
> -	if (error) {
> +	if (error)
>  		return error;
> -	}
>  	*len = maxlen;
>  	*rtx = start;
>  	return 0;
> @@ -443,7 +428,6 @@ xfs_rtallocate_extent_near(
>  	int			j;	/* secondary loop control */
>  	int			log2len; /* log2 of minlen */
>  	xfs_rtxnum_t		n;	/* next rtext to try */
> -	xfs_rtxnum_t		r;	/* result rtext */
>  
>  	ASSERT(minlen % prod == 0);
>  	ASSERT(maxlen % prod == 0);
> @@ -457,26 +441,18 @@ xfs_rtallocate_extent_near(
>  
>  	/* Make sure we don't run off the end of the rt volume. */
>  	maxlen = xfs_rtallocate_clamp_len(mp, start, maxlen, prod);
> -	if (maxlen < minlen) {
> -		*rtx = NULLRTEXTNO;
> -		return 0;
> -	}
> +	if (maxlen < minlen)
> +		return -ENOSPC;
>  
>  	/*
>  	 * Try the exact allocation first.
>  	 */
>  	error = xfs_rtallocate_extent_exact(args, start, minlen, maxlen, len,
> -			prod, &r);
> -	if (error) {
> +			prod, rtx);
> +	if (error != -ENOSPC)
>  		return error;
> -	}
> -	/*
> -	 * If the exact allocation worked, return that.
> -	 */
> -	if (r != NULLRTEXTNO) {
> -		*rtx = r;
> -		return 0;
> -	}
> +
> +
>  	bbno = xfs_rtx_to_rbmblock(mp, start);
>  	i = 0;
>  	j = -1;
> @@ -492,9 +468,9 @@ xfs_rtallocate_extent_near(
>  		 */
>  		error = xfs_rtany_summary(args, log2len, mp->m_rsumlevels - 1,
>  				bbno + i, &maxlog);
> -		if (error) {
> +		if (error)
>  			return error;
> -		}
> +
>  		/*
>  		 * If there are any useful extents starting here, try
>  		 * allocating one.
> @@ -513,17 +489,9 @@ xfs_rtallocate_extent_near(
>  				 */
>  				error = xfs_rtallocate_extent_block(args,
>  						bbno + i, minlen, maxavail, len,
> -						&n, prod, &r);
> -				if (error) {
> +						&n, prod, rtx);
> +				if (error != -ENOSPC)
>  					return error;
> -				}
> -				/*
> -				 * If it worked, return it.
> -				 */
> -				if (r != NULLRTEXTNO) {
> -					*rtx = r;
> -					return 0;
> -				}
>  			}
>  			/*
>  			 * On the negative side of the starting location.
> @@ -557,17 +525,9 @@ xfs_rtallocate_extent_near(
>  					error = xfs_rtallocate_extent_block(args,
>  							bbno + j, minlen,
>  							maxavail, len, &n, prod,
> -							&r);
> -					if (error) {
> +							rtx);
> +					if (error != -ENOSPC)
>  						return error;
> -					}
> -					/*
> -					 * If it works, return the extent.
> -					 */
> -					if (r != NULLRTEXTNO) {
> -						*rtx = r;
> -						return 0;
> -					}
>  				}
>  			}
>  		}
> @@ -601,8 +561,7 @@ xfs_rtallocate_extent_near(
>  		else
>  			break;
>  	}
> -	*rtx = NULLRTEXTNO;
> -	return 0;
> +	return -ENOSPC;
>  }
>  
>  /*
> @@ -624,7 +583,6 @@ xfs_rtallocate_extent_size(
>  	xfs_fileoff_t		i;	/* bitmap block number */
>  	int			l;	/* level number (loop control) */
>  	xfs_rtxnum_t		n;	/* next rtext to be tried */
> -	xfs_rtxnum_t		r;	/* result rtext number */
>  	xfs_suminfo_t		sum;	/* summary information for extents */
>  
>  	ASSERT(minlen % prod == 0);
> @@ -647,9 +605,8 @@ xfs_rtallocate_extent_size(
>  			 * Get the summary for this level/block.
>  			 */
>  			error = xfs_rtget_summary(args, l, i, &sum);
> -			if (error) {
> +			if (error)
>  				return error;
> -			}
>  			/*
>  			 * Nothing there, on to the next block.
>  			 */
> @@ -659,17 +616,9 @@ xfs_rtallocate_extent_size(
>  			 * Try allocating the extent.
>  			 */
>  			error = xfs_rtallocate_extent_block(args, i, maxlen,
> -					maxlen, len, &n, prod, &r);
> -			if (error) {
> +					maxlen, len, &n, prod, rtx);
> +			if (error != -ENOSPC)
>  				return error;
> -			}
> -			/*
> -			 * If it worked, return that.
> -			 */
> -			if (r != NULLRTEXTNO) {
> -				*rtx = r;
> -				return 0;
> -			}
>  			/*
>  			 * If the "next block to try" returned from the
>  			 * allocator is beyond the next bitmap block,
> @@ -683,10 +632,8 @@ xfs_rtallocate_extent_size(
>  	 * Didn't find any maxlen blocks.  Try smaller ones, unless
>  	 * we're asking for a fixed size extent.
>  	 */
> -	if (minlen > --maxlen) {
> -		*rtx = NULLRTEXTNO;
> -		return 0;
> -	}
> +	if (minlen > --maxlen)
> +		return -ENOSPC;
>  	ASSERT(minlen != 0);
>  	ASSERT(maxlen != 0);
>  
> @@ -705,9 +652,9 @@ xfs_rtallocate_extent_size(
>  			 * Get the summary information for this level/block.
>  			 */
>  			error =	xfs_rtget_summary(args, l, i, &sum);
> -			if (error) {
> +			if (error)
>  				return error;
> -			}
> +
>  			/*
>  			 * If nothing there, go on to next.
>  			 */
> @@ -721,17 +668,10 @@ xfs_rtallocate_extent_size(
>  			error = xfs_rtallocate_extent_block(args, i,
>  					XFS_RTMAX(minlen, 1 << l),
>  					XFS_RTMIN(maxlen, (1 << (l + 1)) - 1),
> -					len, &n, prod, &r);
> -			if (error) {
> +					len, &n, prod, rtx);
> +			if (error != -ENOSPC)
>  				return error;
> -			}
> -			/*
> -			 * If it worked, return that extent.
> -			 */
> -			if (r != NULLRTEXTNO) {
> -				*rtx = r;
> -				return 0;
> -			}
> +
>  			/*
>  			 * If the "next block to try" returned from the
>  			 * allocator is beyond the next bitmap block,
> @@ -744,8 +684,7 @@ xfs_rtallocate_extent_size(
>  	/*
>  	 * Got nothing, return failure.
>  	 */
> -	*rtx = NULLRTEXTNO;
> -	return 0;
> +	return -ENOSPC;
>  }
>  
>  /*
> @@ -1177,14 +1116,13 @@ xfs_rtallocate_extent(
>  	xfs_rtxlen_t		*len,	/* out: actual length allocated */
>  	int			wasdel,	/* was a delayed allocation extent */
>  	xfs_rtxlen_t		prod,	/* extent product factor */
> -	xfs_rtxnum_t		*rtblock) /* out: start rtext allocated */
> +	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
>  {
>  	struct xfs_rtalloc_args	args = {
>  		.mp		= tp->t_mountp,
>  		.tp		= tp,
>  	};
>  	int			error;	/* error value */
> -	xfs_rtxnum_t		r;	/* result allocated rtext */
>  
>  	ASSERT(xfs_isilocked(args.mp->m_rbmip, XFS_ILOCK_EXCL));
>  	ASSERT(minlen > 0 && minlen <= maxlen);
> @@ -1199,42 +1137,35 @@ xfs_rtallocate_extent(
>  			maxlen -= i;
>  		if ((i = minlen % prod))
>  			minlen += prod - i;
> -		if (maxlen < minlen) {
> -			*rtblock = NULLRTEXTNO;
> -			return 0;
> -		}
> +		if (maxlen < minlen)
> +			return -ENOSPC;
>  	}
>  
>  retry:
>  	if (start == 0) {
>  		error = xfs_rtallocate_extent_size(&args, minlen,
> -				maxlen, len, prod, &r);
> +				maxlen, len, prod, rtx);
>  	} else {
>  		error = xfs_rtallocate_extent_near(&args, start, minlen,
> -				maxlen, len, prod, &r);
> +				maxlen, len, prod, rtx);
>  	}
> -
>  	xfs_rtbuf_cache_relse(&args);
> -	if (error)
> +	if (error) {
> +		if (error == -ENOSPC && prod > 1) {
> +			prod = 1;
> +			goto retry;
> +		}
>  		return error;
> +	}
>  
>  	/*
>  	 * If it worked, update the superblock.
>  	 */
> -	if (r != NULLRTEXTNO) {
> -		long	slen = (long)*len;
> -
> -		ASSERT(*len >= minlen && *len <= maxlen);
> -		if (wasdel)
> -			xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FREXTENTS, -slen);
> -		else
> -			xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, -slen);
> -	} else if (prod > 1) {
> -		prod = 1;
> -		goto retry;
> -	}
> -
> -	*rtblock = r;
> +	ASSERT(*len >= minlen && *len <= maxlen);
> +	if (wasdel)
> +		xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FREXTENTS, -(long)*len);
> +	else
> +		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, -(long)*len);
>  	return 0;
>  }
>  
> @@ -1548,16 +1479,16 @@ xfs_bmap_rtalloc(
>  	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
>  	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
>  			ap->wasdel, prod, &rtx);
> -	if (error)
> -		return error;
> -
> -	if (rtx != NULLRTEXTNO) {
> +	if (!error) {
>  		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
>  		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
>  		xfs_bmap_alloc_account(ap);
>  		return 0;
>  	}
>  
> +	if (error != -ENOSPC)
> +		return error;
> +
>  	if (align > mp->m_sb.sb_rextsize) {
>  		/*
>  		 * We previously enlarged the request length to try to satisfy
> -- 
> 2.39.2
> 
> 

[PATCH 07/19] xfs: reflow the tail end of xfs_bmap_rtalloc

[Christoph Hellwig]

Reorder the tail end of xfs_bmap_rtalloc so that the successfully
allocation is in the main path, and the error handling is on a branch.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index dac148d53af3ec..158a631379378e 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1479,39 +1479,39 @@ xfs_bmap_rtalloc(
 	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
 	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
 			ap->wasdel, prod, &rtx);
-	if (!error) {
-		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
-		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
-		xfs_bmap_alloc_account(ap);
-		return 0;
-	}
-
-	if (error != -ENOSPC)
-		return error;
+	if (error == -ENOSPC) {
+		if (align > mp->m_sb.sb_rextsize) {
+			/*
+			 * We previously enlarged the request length to try to
+			 * satisfy an extent size hint.  The allocator didn't
+			 * return anything, so reset the parameters to the
+			 * original values and try again without alignment
+			 * criteria.
+			 */
+			ap->offset = orig_offset;
+			ap->length = orig_length;
+			minlen = align = mp->m_sb.sb_rextsize;
+			goto retry;
+		}
 
-	if (align > mp->m_sb.sb_rextsize) {
-		/*
-		 * We previously enlarged the request length to try to satisfy
-		 * an extent size hint.  The allocator didn't return anything,
-		 * so reset the parameters to the original values and try again
-		 * without alignment criteria.
-		 */
-		ap->offset = orig_offset;
-		ap->length = orig_length;
-		minlen = align = mp->m_sb.sb_rextsize;
-		goto retry;
-	}
+		if (!ignore_locality && ap->blkno != 0) {
+			/*
+			 * If we can't allocate near a specific rt extent, try
+			 * again without locality criteria.
+			 */
+			ignore_locality = true;
+			goto retry;
+		}
 
-	if (!ignore_locality && ap->blkno != 0) {
-		/*
-		 * If we can't allocate near a specific rt extent, try again
-		 * without locality criteria.
-		 */
-		ignore_locality = true;
-		goto retry;
+		ap->blkno = NULLFSBLOCK;
+		ap->length = 0;
+		return 0;
 	}
+	if (error)
+		return error;
 
-	ap->blkno = NULLFSBLOCK;
-	ap->length = 0;
+	ap->blkno = xfs_rtx_to_rtb(mp, rtx);
+	ap->length = xfs_rtxlen_to_extlen(mp, ralen);
+	xfs_bmap_alloc_account(ap);
 	return 0;
 }
-- 
2.39.2

Re: [PATCH 07/19] xfs: reflow the tail end of xfs_bmap_rtalloc

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:26AM +0100, Christoph Hellwig wrote:
> Reorder the tail end of xfs_bmap_rtalloc so that the successfully
> allocation is in the main path, and the error handling is on a branch.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_rtalloc.c | 60 ++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index dac148d53af3ec..158a631379378e 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1479,39 +1479,39 @@ xfs_bmap_rtalloc(
>  	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
>  	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
>  			ap->wasdel, prod, &rtx);
> -	if (!error) {
> -		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
> -		ap->length = xfs_rtxlen_to_extlen(mp, ralen);
> -		xfs_bmap_alloc_account(ap);
> -		return 0;
> -	}
> -
> -	if (error != -ENOSPC)
> -		return error;
> +	if (error == -ENOSPC) {
> +		if (align > mp->m_sb.sb_rextsize) {
> +			/*
> +			 * We previously enlarged the request length to try to
> +			 * satisfy an extent size hint.  The allocator didn't
> +			 * return anything, so reset the parameters to the
> +			 * original values and try again without alignment
> +			 * criteria.
> +			 */
> +			ap->offset = orig_offset;
> +			ap->length = orig_length;
> +			minlen = align = mp->m_sb.sb_rextsize;
> +			goto retry;
> +		}
>  
> -	if (align > mp->m_sb.sb_rextsize) {
> -		/*
> -		 * We previously enlarged the request length to try to satisfy
> -		 * an extent size hint.  The allocator didn't return anything,
> -		 * so reset the parameters to the original values and try again
> -		 * without alignment criteria.
> -		 */
> -		ap->offset = orig_offset;
> -		ap->length = orig_length;
> -		minlen = align = mp->m_sb.sb_rextsize;
> -		goto retry;
> -	}
> +		if (!ignore_locality && ap->blkno != 0) {
> +			/*
> +			 * If we can't allocate near a specific rt extent, try
> +			 * again without locality criteria.
> +			 */
> +			ignore_locality = true;
> +			goto retry;
> +		}
>  
> -	if (!ignore_locality && ap->blkno != 0) {
> -		/*
> -		 * If we can't allocate near a specific rt extent, try again
> -		 * without locality criteria.
> -		 */
> -		ignore_locality = true;
> -		goto retry;
> +		ap->blkno = NULLFSBLOCK;
> +		ap->length = 0;
> +		return 0;
>  	}
> +	if (error)
> +		return error;
>  
> -	ap->blkno = NULLFSBLOCK;
> -	ap->length = 0;
> +	ap->blkno = xfs_rtx_to_rtb(mp, rtx);
> +	ap->length = xfs_rtxlen_to_extlen(mp, ralen);
> +	xfs_bmap_alloc_account(ap);
>  	return 0;
>  }
> -- 
> 2.39.2
> 
> 

[PATCH 08/19] xfs: indicate if xfs_bmap_adjacent changed ap->blkno

[Christoph Hellwig]

Add a return value to xfs_bmap_adjacent to indicate if it did change
ap->blkno or not.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 19 ++++++++++++++-----
 fs/xfs/xfs_bmap_util.h   |  2 +-
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 6722205949ad4c..46a9b22a3733e3 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3044,7 +3044,8 @@ xfs_bmap_extsize_align(
 
 #define XFS_ALLOC_GAP_UNITS	4
 
-void
+/* returns true if ap->blkno was modified */
+bool
 xfs_bmap_adjacent(
 	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
 {
@@ -3079,13 +3080,14 @@ xfs_bmap_adjacent(
 		if (adjust &&
 		    ISVALID(ap->blkno + adjust, ap->prev.br_startblock))
 			ap->blkno += adjust;
+		return true;
 	}
 	/*
 	 * If not at eof, then compare the two neighbor blocks.
 	 * Figure out whether either one gives us a good starting point,
 	 * and pick the better one.
 	 */
-	else if (!ap->eof) {
+	if (!ap->eof) {
 		xfs_fsblock_t	gotbno;		/* right side block number */
 		xfs_fsblock_t	gotdiff=0;	/* right side difference */
 		xfs_fsblock_t	prevbno;	/* left side block number */
@@ -3165,14 +3167,21 @@ xfs_bmap_adjacent(
 		 * If both valid, pick the better one, else the only good
 		 * one, else ap->blkno is already set (to 0 or the inode block).
 		 */
-		if (prevbno != NULLFSBLOCK && gotbno != NULLFSBLOCK)
+		if (prevbno != NULLFSBLOCK && gotbno != NULLFSBLOCK) {
 			ap->blkno = prevdiff <= gotdiff ? prevbno : gotbno;
-		else if (prevbno != NULLFSBLOCK)
+			return true;
+		}
+		if (prevbno != NULLFSBLOCK) {
 			ap->blkno = prevbno;
-		else if (gotbno != NULLFSBLOCK)
+			return true;
+		}
+		if (gotbno != NULLFSBLOCK) {
 			ap->blkno = gotbno;
+			return true;
+		}
 	}
 #undef ISVALID
+	return false;
 }
 
 int
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 6888078f5c31e0..77ecbb753ef207 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -47,7 +47,7 @@ int	xfs_bmap_extsize_align(struct xfs_mount *mp, struct xfs_bmbt_irec *gotp,
 			       struct xfs_bmbt_irec *prevp, xfs_extlen_t extsz,
 			       int rt, int eof, int delay, int convert,
 			       xfs_fileoff_t *offp, xfs_extlen_t *lenp);
-void	xfs_bmap_adjacent(struct xfs_bmalloca *ap);
+bool	xfs_bmap_adjacent(struct xfs_bmalloca *ap);
 int	xfs_bmap_last_extent(struct xfs_trans *tp, struct xfs_inode *ip,
 			     int whichfork, struct xfs_bmbt_irec *rec,
 			     int *is_empty);
-- 
2.39.2

Re: [PATCH 08/19] xfs: indicate if xfs_bmap_adjacent changed ap->blkno

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:27AM +0100, Christoph Hellwig wrote:
> Add a return value to xfs_bmap_adjacent to indicate if it did change
> ap->blkno or not.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks fine to me, though I don't see any immediate callsite changes so I
guess I'll look out for whatever uses the return value.

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

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 19 ++++++++++++++-----
>  fs/xfs/xfs_bmap_util.h   |  2 +-
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6722205949ad4c..46a9b22a3733e3 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3044,7 +3044,8 @@ xfs_bmap_extsize_align(
>  
>  #define XFS_ALLOC_GAP_UNITS	4
>  
> -void
> +/* returns true if ap->blkno was modified */
> +bool
>  xfs_bmap_adjacent(
>  	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
>  {
> @@ -3079,13 +3080,14 @@ xfs_bmap_adjacent(
>  		if (adjust &&
>  		    ISVALID(ap->blkno + adjust, ap->prev.br_startblock))
>  			ap->blkno += adjust;
> +		return true;
>  	}
>  	/*
>  	 * If not at eof, then compare the two neighbor blocks.
>  	 * Figure out whether either one gives us a good starting point,
>  	 * and pick the better one.
>  	 */
> -	else if (!ap->eof) {
> +	if (!ap->eof) {
>  		xfs_fsblock_t	gotbno;		/* right side block number */
>  		xfs_fsblock_t	gotdiff=0;	/* right side difference */
>  		xfs_fsblock_t	prevbno;	/* left side block number */
> @@ -3165,14 +3167,21 @@ xfs_bmap_adjacent(
>  		 * If both valid, pick the better one, else the only good
>  		 * one, else ap->blkno is already set (to 0 or the inode block).
>  		 */
> -		if (prevbno != NULLFSBLOCK && gotbno != NULLFSBLOCK)
> +		if (prevbno != NULLFSBLOCK && gotbno != NULLFSBLOCK) {
>  			ap->blkno = prevdiff <= gotdiff ? prevbno : gotbno;
> -		else if (prevbno != NULLFSBLOCK)
> +			return true;
> +		}
> +		if (prevbno != NULLFSBLOCK) {
>  			ap->blkno = prevbno;
> -		else if (gotbno != NULLFSBLOCK)
> +			return true;
> +		}
> +		if (gotbno != NULLFSBLOCK) {
>  			ap->blkno = gotbno;
> +			return true;
> +		}
>  	}
>  #undef ISVALID
> +	return false;
>  }
>  
>  int
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 6888078f5c31e0..77ecbb753ef207 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -47,7 +47,7 @@ int	xfs_bmap_extsize_align(struct xfs_mount *mp, struct xfs_bmbt_irec *gotp,
>  			       struct xfs_bmbt_irec *prevp, xfs_extlen_t extsz,
>  			       int rt, int eof, int delay, int convert,
>  			       xfs_fileoff_t *offp, xfs_extlen_t *lenp);
> -void	xfs_bmap_adjacent(struct xfs_bmalloca *ap);
> +bool	xfs_bmap_adjacent(struct xfs_bmalloca *ap);
>  int	xfs_bmap_last_extent(struct xfs_trans *tp, struct xfs_inode *ip,
>  			     int whichfork, struct xfs_bmbt_irec *rec,
>  			     int *is_empty);
> -- 
> 2.39.2
> 
> 

[PATCH 09/19] xfs: cleanup picking the start extent hint in xfs_bmap_rtalloc

[Christoph Hellwig]

Clean up the logical in xfs_bmap_rtalloc that tries to find a rtextent
to start the search from by using a separate variable for the hint, not
calling xfs_bmap_adjacent when we want to ignore the locality and avoid
an extra roundtrip converting between block numbers and RT extent
numbers.

As a side-effect this doesn't pointlessly call xfs_rtpick_extent and
increment the start rtextent hint if we are going to ignore the result
anyway.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 158a631379378e..2ce3bcf4b84b76 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1393,7 +1393,8 @@ xfs_bmap_rtalloc(
 {
 	struct xfs_mount	*mp = ap->ip->i_mount;
 	xfs_fileoff_t		orig_offset = ap->offset;
-	xfs_rtxnum_t		rtx;
+	xfs_rtxnum_t		start;	   /* allocation hint rtextent no */
+	xfs_rtxnum_t		rtx;	   /* actually allocated rtextent no */
 	xfs_rtxlen_t		prod = 0;  /* product factor for allocators */
 	xfs_extlen_t		mod = 0;   /* product factor for allocators */
 	xfs_rtxlen_t		ralen = 0; /* realtime allocation length */
@@ -1454,30 +1455,24 @@ xfs_bmap_rtalloc(
 		rtlocked = true;
 	}
 
-	/*
-	 * If it's an allocation to an empty file at offset 0,
-	 * pick an extent that will space things out in the rt area.
-	 */
-	if (ap->eof && ap->offset == 0) {
-		error = xfs_rtpick_extent(mp, ap->tp, ralen, &rtx);
+	if (ignore_locality) {
+		start = 0;
+	} else if (xfs_bmap_adjacent(ap)) {
+		start = xfs_rtb_to_rtx(mp, ap->blkno);
+	} else if (ap->eof && ap->offset == 0) {
+		/*
+		 * If it's an allocation to an empty file at offset 0, pick an
+		 * extent that will space things out in the rt area.
+		 */
+		error = xfs_rtpick_extent(mp, ap->tp, ralen, &start);
 		if (error)
 			return error;
-		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
 	} else {
-		ap->blkno = 0;
+		start = 0;
 	}
 
-	xfs_bmap_adjacent(ap);
-
-	/*
-	 * Realtime allocation, done through xfs_rtallocate_extent.
-	 */
-	if (ignore_locality)
-		rtx = 0;
-	else
-		rtx = xfs_rtb_to_rtx(mp, ap->blkno);
 	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
-	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
+	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
 			ap->wasdel, prod, &rtx);
 	if (error == -ENOSPC) {
 		if (align > mp->m_sb.sb_rextsize) {
@@ -1494,7 +1489,7 @@ xfs_bmap_rtalloc(
 			goto retry;
 		}
 
-		if (!ignore_locality && ap->blkno != 0) {
+		if (!ignore_locality && start != 0) {
 			/*
 			 * If we can't allocate near a specific rt extent, try
 			 * again without locality criteria.
-- 
2.39.2

Re: [PATCH 09/19] xfs: cleanup picking the start extent hint in xfs_bmap_rtalloc

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:28AM +0100, Christoph Hellwig wrote:
> Clean up the logical in xfs_bmap_rtalloc that tries to find a rtextent
> to start the search from by using a separate variable for the hint, not
> calling xfs_bmap_adjacent when we want to ignore the locality and avoid
> an extra roundtrip converting between block numbers and RT extent
> numbers.

Ahah, I had wondered about that...

> As a side-effect this doesn't pointlessly call xfs_rtpick_extent and
> increment the start rtextent hint if we are going to ignore the result
> anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_rtalloc.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 158a631379378e..2ce3bcf4b84b76 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1393,7 +1393,8 @@ xfs_bmap_rtalloc(
>  {
>  	struct xfs_mount	*mp = ap->ip->i_mount;
>  	xfs_fileoff_t		orig_offset = ap->offset;
> -	xfs_rtxnum_t		rtx;
> +	xfs_rtxnum_t		start;	   /* allocation hint rtextent no */
> +	xfs_rtxnum_t		rtx;	   /* actually allocated rtextent no */
>  	xfs_rtxlen_t		prod = 0;  /* product factor for allocators */
>  	xfs_extlen_t		mod = 0;   /* product factor for allocators */
>  	xfs_rtxlen_t		ralen = 0; /* realtime allocation length */
> @@ -1454,30 +1455,24 @@ xfs_bmap_rtalloc(
>  		rtlocked = true;
>  	}
>  
> -	/*
> -	 * If it's an allocation to an empty file at offset 0,
> -	 * pick an extent that will space things out in the rt area.
> -	 */
> -	if (ap->eof && ap->offset == 0) {
> -		error = xfs_rtpick_extent(mp, ap->tp, ralen, &rtx);
> +	if (ignore_locality) {
> +		start = 0;
> +	} else if (xfs_bmap_adjacent(ap)) {
> +		start = xfs_rtb_to_rtx(mp, ap->blkno);
> +	} else if (ap->eof && ap->offset == 0) {
> +		/*
> +		 * If it's an allocation to an empty file at offset 0, pick an
> +		 * extent that will space things out in the rt area.
> +		 */
> +		error = xfs_rtpick_extent(mp, ap->tp, ralen, &start);
>  		if (error)
>  			return error;
> -		ap->blkno = xfs_rtx_to_rtb(mp, rtx);
>  	} else {
> -		ap->blkno = 0;
> +		start = 0;
>  	}

It took me a while to wrap my head around the dual "start = 0" clauses
here, but eventually I figured out that the first one is from below, and
this one here is merely the continuation of the "!adjacent" and
"!emptyfileatoffset0" cases.

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

--D

>  
> -	xfs_bmap_adjacent(ap);
> -
> -	/*
> -	 * Realtime allocation, done through xfs_rtallocate_extent.
> -	 */
> -	if (ignore_locality)
> -		rtx = 0;
> -	else
> -		rtx = xfs_rtb_to_rtx(mp, ap->blkno);
>  	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
> -	error = xfs_rtallocate_extent(ap->tp, rtx, raminlen, ralen, &ralen,
> +	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
>  			ap->wasdel, prod, &rtx);
>  	if (error == -ENOSPC) {
>  		if (align > mp->m_sb.sb_rextsize) {
> @@ -1494,7 +1489,7 @@ xfs_bmap_rtalloc(
>  			goto retry;
>  		}
>  
> -		if (!ignore_locality && ap->blkno != 0) {
> +		if (!ignore_locality && start != 0) {
>  			/*
>  			 * If we can't allocate near a specific rt extent, try
>  			 * again without locality criteria.
> -- 
> 2.39.2
> 
> 

[PATCH 10/19] xfs: move xfs_rtget_summary to xfs_rtbitmap.c

[Christoph Hellwig]

xfs_rtmodify_summary_int is only used inside xfs_rtbitmap.c and to
implement xfs_rtget_summary.  Move xfs_rtget_summary to xfs_rtbitmap.c
as the exported API and mark xfs_rtmodify_summary_int static.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_rtbitmap.c | 14 ++++++++++++++
 fs/xfs/libxfs/xfs_rtbitmap.h |  4 ++--
 fs/xfs/xfs_rtalloc.c         | 16 ----------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 30a2844f62e30f..e67f6f763f7d0f 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -519,6 +519,20 @@ xfs_rtmodify_summary(
 	return xfs_rtmodify_summary_int(args, log, bbno, delta, NULL);
 }
 
+/*
+ * Read and return the summary information for a given extent size, bitmap block
+ * combination.
+ */
+int
+xfs_rtget_summary(
+	struct xfs_rtalloc_args	*args,
+	int			log,	/* log2 of extent size */
+	xfs_fileoff_t		bbno,	/* bitmap block number */
+	xfs_suminfo_t		*sum)	/* out: summary info for this block */
+{
+	return xfs_rtmodify_summary_int(args, log, bbno, 0, sum);
+}
+
 /* Log rtbitmap block from the word @from to the byte before @next. */
 static inline void
 xfs_trans_log_rtbitmap(
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
index 1c84b52de3d424..274dc7dae1faf8 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.h
+++ b/fs/xfs/libxfs/xfs_rtbitmap.h
@@ -321,8 +321,8 @@ int xfs_rtfind_forw(struct xfs_rtalloc_args *args, xfs_rtxnum_t start,
 		xfs_rtxnum_t limit, xfs_rtxnum_t *rtblock);
 int xfs_rtmodify_range(struct xfs_rtalloc_args *args, xfs_rtxnum_t start,
 		xfs_rtxlen_t len, int val);
-int xfs_rtmodify_summary_int(struct xfs_rtalloc_args *args, int log,
-		xfs_fileoff_t bbno, int delta, xfs_suminfo_t *sum);
+int xfs_rtget_summary(struct xfs_rtalloc_args *args, int log,
+		xfs_fileoff_t bbno, xfs_suminfo_t *sum);
 int xfs_rtmodify_summary(struct xfs_rtalloc_args *args, int log,
 		xfs_fileoff_t bbno, int delta);
 int xfs_rtfree_range(struct xfs_rtalloc_args *args, xfs_rtxnum_t start,
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 2ce3bcf4b84b76..fbc60658ef24bf 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -23,22 +23,6 @@
 #include "xfs_rtbitmap.h"
 #include "xfs_quota.h"
 
-/*
- * Read and return the summary information for a given extent size,
- * bitmap block combination.
- * Keeps track of a current summary block, so we don't keep reading
- * it from the buffer cache.
- */
-static int
-xfs_rtget_summary(
-	struct xfs_rtalloc_args	*args,
-	int			log,	/* log2 of extent size */
-	xfs_fileoff_t		bbno,	/* bitmap block number */
-	xfs_suminfo_t		*sum)	/* out: summary info for this block */
-{
-	return xfs_rtmodify_summary_int(args, log, bbno, 0, sum);
-}
-
 /*
  * Return whether there are any free extents in the size range given
  * by low and high, for the bitmap block bbno.
-- 
2.39.2

Re: [PATCH 10/19] xfs: move xfs_rtget_summary to xfs_rtbitmap.c

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:29AM +0100, Christoph Hellwig wrote:
> xfs_rtmodify_summary_int is only used inside xfs_rtbitmap.c and to
> implement xfs_rtget_summary.  Move xfs_rtget_summary to xfs_rtbitmap.c
> as the exported API and mark xfs_rtmodify_summary_int static.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c | 14 ++++++++++++++
>  fs/xfs/libxfs/xfs_rtbitmap.h |  4 ++--
>  fs/xfs/xfs_rtalloc.c         | 16 ----------------
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 30a2844f62e30f..e67f6f763f7d0f 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -519,6 +519,20 @@ xfs_rtmodify_summary(
>  	return xfs_rtmodify_summary_int(args, log, bbno, delta, NULL);
>  }
>  
> +/*
> + * Read and return the summary information for a given extent size, bitmap block
> + * combination.
> + */
> +int
> +xfs_rtget_summary(
> +	struct xfs_rtalloc_args	*args,
> +	int			log,	/* log2 of extent size */
> +	xfs_fileoff_t		bbno,	/* bitmap block number */
> +	xfs_suminfo_t		*sum)	/* out: summary info for this block */
> +{
> +	return xfs_rtmodify_summary_int(args, log, bbno, 0, sum);
> +}
> +
>  /* Log rtbitmap block from the word @from to the byte before @next. */
>  static inline void
>  xfs_trans_log_rtbitmap(
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
> index 1c84b52de3d424..274dc7dae1faf8 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.h
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.h
> @@ -321,8 +321,8 @@ int xfs_rtfind_forw(struct xfs_rtalloc_args *args, xfs_rtxnum_t start,
>  		xfs_rtxnum_t limit, xfs_rtxnum_t *rtblock);
>  int xfs_rtmodify_range(struct xfs_rtalloc_args *args, xfs_rtxnum_t start,
>  		xfs_rtxlen_t len, int val);
> -int xfs_rtmodify_summary_int(struct xfs_rtalloc_args *args, int log,
> -		xfs_fileoff_t bbno, int delta, xfs_suminfo_t *sum);
> +int xfs_rtget_summary(struct xfs_rtalloc_args *args, int log,
> +		xfs_fileoff_t bbno, xfs_suminfo_t *sum);
>  int xfs_rtmodify_summary(struct xfs_rtalloc_args *args, int log,
>  		xfs_fileoff_t bbno, int delta);
>  int xfs_rtfree_range(struct xfs_rtalloc_args *args, xfs_rtxnum_t start,
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 2ce3bcf4b84b76..fbc60658ef24bf 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -23,22 +23,6 @@
>  #include "xfs_rtbitmap.h"
>  #include "xfs_quota.h"
>  
> -/*
> - * Read and return the summary information for a given extent size,
> - * bitmap block combination.
> - * Keeps track of a current summary block, so we don't keep reading
> - * it from the buffer cache.
> - */
> -static int
> -xfs_rtget_summary(
> -	struct xfs_rtalloc_args	*args,
> -	int			log,	/* log2 of extent size */
> -	xfs_fileoff_t		bbno,	/* bitmap block number */
> -	xfs_suminfo_t		*sum)	/* out: summary info for this block */
> -{
> -	return xfs_rtmodify_summary_int(args, log, bbno, 0, sum);
> -}
> -
>  /*
>   * Return whether there are any free extents in the size range given
>   * by low and high, for the bitmap block bbno.
> -- 
> 2.39.2
> 
> 

[PATCH 11/19] xfs: split xfs_rtmodify_summary_int

[Christoph Hellwig]

Inline the logic of xfs_rtmodify_summary_int into xfs_rtmodify_summary
and xfs_rtget_summary instead of having a somewhat awakard helper to
share a little bit of code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_rtbitmap.c | 76 ++++++++++++------------------------
 1 file changed, 25 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index e67f6f763f7d0f..5773e4ea36c624 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -452,71 +452,38 @@ xfs_trans_log_rtsummary(
 }
 
 /*
- * Read and/or modify the summary information for a given extent size,
- * bitmap block combination.
- * Keeps track of a current summary block, so we don't keep reading
- * it from the buffer cache.
- *
- * Summary information is returned in *sum if specified.
- * If no delta is specified, returns summary only.
+ * Modify the summary information for a given extent size, bitmap block
+ * combination.
  */
 int
-xfs_rtmodify_summary_int(
+xfs_rtmodify_summary(
 	struct xfs_rtalloc_args	*args,
 	int			log,	/* log2 of extent size */
 	xfs_fileoff_t		bbno,	/* bitmap block number */
-	int			delta,	/* change to make to summary info */
-	xfs_suminfo_t		*sum)	/* out: summary info for this block */
+	int			delta)	/* in/out: summary block number */
 {
 	struct xfs_mount	*mp = args->mp;
-	int			error;
-	xfs_fileoff_t		sb;	/* summary fsblock */
-	xfs_rtsumoff_t		so;	/* index into the summary file */
+	xfs_rtsumoff_t		so = xfs_rtsumoffs(mp, log, bbno);
 	unsigned int		infoword;
+	xfs_suminfo_t		val;
+	int			error;
 
-	/*
-	 * Compute entry number in the summary file.
-	 */
-	so = xfs_rtsumoffs(mp, log, bbno);
-	/*
-	 * Compute the block number in the summary file.
-	 */
-	sb = xfs_rtsumoffs_to_block(mp, so);
-
-	error = xfs_rtsummary_read_buf(args, sb);
+	error = xfs_rtsummary_read_buf(args, xfs_rtsumoffs_to_block(mp, so));
 	if (error)
 		return error;
 
-	/*
-	 * Point to the summary information, modify/log it, and/or copy it out.
-	 */
 	infoword = xfs_rtsumoffs_to_infoword(mp, so);
-	if (delta) {
-		xfs_suminfo_t	val = xfs_suminfo_add(args, infoword, delta);
-
-		if (mp->m_rsum_cache) {
-			if (val == 0 && log + 1 == mp->m_rsum_cache[bbno])
-				mp->m_rsum_cache[bbno] = log;
-			if (val != 0 && log >= mp->m_rsum_cache[bbno])
-				mp->m_rsum_cache[bbno] = log + 1;
-		}
-		xfs_trans_log_rtsummary(args, infoword);
-		if (sum)
-			*sum = val;
-	} else if (sum) {
-		*sum = xfs_suminfo_get(args, infoword);
+	val = xfs_suminfo_add(args, infoword, delta);
+
+	if (mp->m_rsum_cache) {
+		if (val == 0 && log + 1 == mp->m_rsum_cache[bbno])
+			mp->m_rsum_cache[bbno] = log;
+		if (val != 0 && log >= mp->m_rsum_cache[bbno])
+			mp->m_rsum_cache[bbno] = log + 1;
 	}
-	return 0;
-}
 
-int
-xfs_rtmodify_summary(
-	struct xfs_rtalloc_args	*args,
-	int			log,	/* log2 of extent size */
-	xfs_fileoff_t		bbno,	/* bitmap block number */
-	int			delta)	/* in/out: summary block number */
-{
-	return xfs_rtmodify_summary_int(args, log, bbno, delta, NULL);
+	xfs_trans_log_rtsummary(args, infoword);
+	return 0;
 }
 
 /*
@@ -530,7 +497,14 @@ xfs_rtget_summary(
 	xfs_fileoff_t		bbno,	/* bitmap block number */
 	xfs_suminfo_t		*sum)	/* out: summary info for this block */
 {
-	return xfs_rtmodify_summary_int(args, log, bbno, 0, sum);
+	struct xfs_mount	*mp = args->mp;
+	xfs_rtsumoff_t		so = xfs_rtsumoffs(mp, log, bbno);
+	int			error;
+
+	error = xfs_rtsummary_read_buf(args, xfs_rtsumoffs_to_block(mp, so));
+	if (!error)
+		*sum = xfs_suminfo_get(args, xfs_rtsumoffs_to_infoword(mp, so));
+	return error;
 }
 
 /* Log rtbitmap block from the word @from to the byte before @next. */
-- 
2.39.2

Re: [PATCH 11/19] xfs: split xfs_rtmodify_summary_int

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:30AM +0100, Christoph Hellwig wrote:
> Inline the logic of xfs_rtmodify_summary_int into xfs_rtmodify_summary
> and xfs_rtget_summary instead of having a somewhat awakard helper to

s/awakard/awkward/g

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> share a little bit of code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c | 76 ++++++++++++------------------------
>  1 file changed, 25 insertions(+), 51 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index e67f6f763f7d0f..5773e4ea36c624 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -452,71 +452,38 @@ xfs_trans_log_rtsummary(
>  }
>  
>  /*
> - * Read and/or modify the summary information for a given extent size,
> - * bitmap block combination.
> - * Keeps track of a current summary block, so we don't keep reading
> - * it from the buffer cache.
> - *
> - * Summary information is returned in *sum if specified.
> - * If no delta is specified, returns summary only.
> + * Modify the summary information for a given extent size, bitmap block
> + * combination.
>   */
>  int
> -xfs_rtmodify_summary_int(
> +xfs_rtmodify_summary(
>  	struct xfs_rtalloc_args	*args,
>  	int			log,	/* log2 of extent size */
>  	xfs_fileoff_t		bbno,	/* bitmap block number */
> -	int			delta,	/* change to make to summary info */
> -	xfs_suminfo_t		*sum)	/* out: summary info for this block */
> +	int			delta)	/* in/out: summary block number */
>  {
>  	struct xfs_mount	*mp = args->mp;
> -	int			error;
> -	xfs_fileoff_t		sb;	/* summary fsblock */
> -	xfs_rtsumoff_t		so;	/* index into the summary file */
> +	xfs_rtsumoff_t		so = xfs_rtsumoffs(mp, log, bbno);
>  	unsigned int		infoword;
> +	xfs_suminfo_t		val;
> +	int			error;
>  
> -	/*
> -	 * Compute entry number in the summary file.
> -	 */
> -	so = xfs_rtsumoffs(mp, log, bbno);
> -	/*
> -	 * Compute the block number in the summary file.
> -	 */
> -	sb = xfs_rtsumoffs_to_block(mp, so);
> -
> -	error = xfs_rtsummary_read_buf(args, sb);
> +	error = xfs_rtsummary_read_buf(args, xfs_rtsumoffs_to_block(mp, so));
>  	if (error)
>  		return error;
>  
> -	/*
> -	 * Point to the summary information, modify/log it, and/or copy it out.
> -	 */
>  	infoword = xfs_rtsumoffs_to_infoword(mp, so);
> -	if (delta) {
> -		xfs_suminfo_t	val = xfs_suminfo_add(args, infoword, delta);
> -
> -		if (mp->m_rsum_cache) {
> -			if (val == 0 && log + 1 == mp->m_rsum_cache[bbno])
> -				mp->m_rsum_cache[bbno] = log;
> -			if (val != 0 && log >= mp->m_rsum_cache[bbno])
> -				mp->m_rsum_cache[bbno] = log + 1;
> -		}
> -		xfs_trans_log_rtsummary(args, infoword);
> -		if (sum)
> -			*sum = val;
> -	} else if (sum) {
> -		*sum = xfs_suminfo_get(args, infoword);
> +	val = xfs_suminfo_add(args, infoword, delta);
> +
> +	if (mp->m_rsum_cache) {
> +		if (val == 0 && log + 1 == mp->m_rsum_cache[bbno])
> +			mp->m_rsum_cache[bbno] = log;
> +		if (val != 0 && log >= mp->m_rsum_cache[bbno])
> +			mp->m_rsum_cache[bbno] = log + 1;
>  	}
> -	return 0;
> -}
>  
> -int
> -xfs_rtmodify_summary(
> -	struct xfs_rtalloc_args	*args,
> -	int			log,	/* log2 of extent size */
> -	xfs_fileoff_t		bbno,	/* bitmap block number */
> -	int			delta)	/* in/out: summary block number */
> -{
> -	return xfs_rtmodify_summary_int(args, log, bbno, delta, NULL);
> +	xfs_trans_log_rtsummary(args, infoword);
> +	return 0;
>  }
>  
>  /*
> @@ -530,7 +497,14 @@ xfs_rtget_summary(
>  	xfs_fileoff_t		bbno,	/* bitmap block number */
>  	xfs_suminfo_t		*sum)	/* out: summary info for this block */
>  {
> -	return xfs_rtmodify_summary_int(args, log, bbno, 0, sum);
> +	struct xfs_mount	*mp = args->mp;
> +	xfs_rtsumoff_t		so = xfs_rtsumoffs(mp, log, bbno);
> +	int			error;
> +
> +	error = xfs_rtsummary_read_buf(args, xfs_rtsumoffs_to_block(mp, so));
> +	if (!error)
> +		*sum = xfs_suminfo_get(args, xfs_rtsumoffs_to_infoword(mp, so));
> +	return error;
>  }
>  
>  /* Log rtbitmap block from the word @from to the byte before @next. */
> -- 
> 2.39.2
> 
> 

[PATCH 12/19] xfs: tidy up xfs_rtallocate_extent_block

[Christoph Hellwig]

Share the xfs_rtallocate_range logic by breaking out of the loop
instead of duplicating it, invert a check so that the early
return case comes first instead of in an else, and handle the
successful case in the straight line instead a branch in the tail
of the function.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index fbc60658ef24bf..5f42422a976a3e 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -257,13 +257,9 @@ xfs_rtallocate_extent_block(
 			/*
 			 * i for maxlen is all free, allocate and return that.
 			 */
-			error = xfs_rtallocate_range(args, i, maxlen);
-			if (error)
-				return error;
-
-			*len = maxlen;
-			*rtx = i;
-			return 0;
+			bestlen = maxlen;
+			besti = i;
+			break;
 		}
 		/*
 		 * In the case where we have a variable-sized allocation
@@ -283,43 +279,44 @@ xfs_rtallocate_extent_block(
 		/*
 		 * If not done yet, find the start of the next free space.
 		 */
-		if (next < end) {
-			error = xfs_rtfind_forw(args, next, end, &i);
-			if (error)
-				return error;
-		} else
+		if (next >= end)
 			break;
+		error = xfs_rtfind_forw(args, next, end, &i);
+		if (error)
+			return error;
 	}
+
 	/*
 	 * Searched the whole thing & didn't find a maxlen free extent.
 	 */
-	if (minlen <= maxlen && besti != -1) {
-		xfs_rtxlen_t	p;	/* amount to trim length by */
-
+	if (maxlen < minlen || besti == -1) {
 		/*
-		 * If size should be a multiple of prod, make that so.
+		 * Allocation failed.  Set *nextp to the next block to try.
 		 */
-		if (prod > 1) {
-			div_u64_rem(bestlen, prod, &p);
-			if (p)
-				bestlen -= p;
-		}
+		*nextp = next;
+		return -ENOSPC;
+	}
 
-		/*
-		 * Allocate besti for bestlen & return that.
-		 */
-		error = xfs_rtallocate_range(args, besti, bestlen);
-		if (error)
-			return error;
-		*len = bestlen;
-		*rtx = besti;
-		return 0;
+	/*
+	 * If size should be a multiple of prod, make that so.
+	 */
+	if (prod > 1) {
+		xfs_rtxlen_t	p;	/* amount to trim length by */
+
+		div_u64_rem(bestlen, prod, &p);
+		if (p)
+			bestlen -= p;
 	}
+
 	/*
-	 * Allocation failed.  Set *nextp to the next block to try.
+	 * Allocate besti for bestlen & return that.
 	 */
-	*nextp = next;
-	return -ENOSPC;
+	error = xfs_rtallocate_range(args, besti, bestlen);
+	if (error)
+		return error;
+	*len = bestlen;
+	*rtx = besti;
+	return 0;
 }
 
 /*
-- 
2.39.2

Re: [PATCH 12/19] xfs: tidy up xfs_rtallocate_extent_block

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:31AM +0100, Christoph Hellwig wrote:
> Share the xfs_rtallocate_range logic by breaking out of the loop
> instead of duplicating it, invert a check so that the early
> return case comes first instead of in an else, and handle the
> successful case in the straight line instead a branch in the tail
> of the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_rtalloc.c | 63 +++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index fbc60658ef24bf..5f42422a976a3e 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -257,13 +257,9 @@ xfs_rtallocate_extent_block(
>  			/*
>  			 * i for maxlen is all free, allocate and return that.
>  			 */
> -			error = xfs_rtallocate_range(args, i, maxlen);
> -			if (error)
> -				return error;
> -
> -			*len = maxlen;
> -			*rtx = i;
> -			return 0;
> +			bestlen = maxlen;
> +			besti = i;
> +			break;

This return->break change gave me pause for a while...

>  		}
>  		/*
>  		 * In the case where we have a variable-sized allocation
> @@ -283,43 +279,44 @@ xfs_rtallocate_extent_block(
>  		/*
>  		 * If not done yet, find the start of the next free space.
>  		 */
> -		if (next < end) {
> -			error = xfs_rtfind_forw(args, next, end, &i);
> -			if (error)
> -				return error;
> -		} else
> +		if (next >= end)
>  			break;
> +		error = xfs_rtfind_forw(args, next, end, &i);
> +		if (error)
> +			return error;
>  	}
> +
>  	/*
>  	 * Searched the whole thing & didn't find a maxlen free extent.
>  	 */
> -	if (minlen <= maxlen && besti != -1) {
> -		xfs_rtxlen_t	p;	/* amount to trim length by */
> -
> +	if (maxlen < minlen || besti == -1) {

...because I was worried about accidentally ending up in this clause
if maxlen < minlen.  I /think/ it's the case that this function never
gets called with that true, right?

(Would this be a good place to add a ebugging assertion?)

--D

>  		/*
> -		 * If size should be a multiple of prod, make that so.
> +		 * Allocation failed.  Set *nextp to the next block to try.
>  		 */
> -		if (prod > 1) {
> -			div_u64_rem(bestlen, prod, &p);
> -			if (p)
> -				bestlen -= p;
> -		}
> +		*nextp = next;
> +		return -ENOSPC;
> +	}
>  
> -		/*
> -		 * Allocate besti for bestlen & return that.
> -		 */
> -		error = xfs_rtallocate_range(args, besti, bestlen);
> -		if (error)
> -			return error;
> -		*len = bestlen;
> -		*rtx = besti;
> -		return 0;
> +	/*
> +	 * If size should be a multiple of prod, make that so.
> +	 */
> +	if (prod > 1) {
> +		xfs_rtxlen_t	p;	/* amount to trim length by */
> +
> +		div_u64_rem(bestlen, prod, &p);
> +		if (p)
> +			bestlen -= p;
>  	}
> +
>  	/*
> -	 * Allocation failed.  Set *nextp to the next block to try.
> +	 * Allocate besti for bestlen & return that.
>  	 */
> -	*nextp = next;
> -	return -ENOSPC;
> +	error = xfs_rtallocate_range(args, besti, bestlen);
> +	if (error)
> +		return error;
> +	*len = bestlen;
> +	*rtx = besti;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.39.2
> 
> 

Re: [PATCH 12/19] xfs: tidy up xfs_rtallocate_extent_block

[Christoph Hellwig]

On Thu, Dec 14, 2023 at 01:16:20PM -0800, Darrick J. Wong wrote:
> > +	if (maxlen < minlen || besti == -1) {
> 
> ...because I was worried about accidentally ending up in this clause
> if maxlen < minlen.  I /think/ it's the case that this function never
> gets called with that true, right?
> 
> (Would this be a good place to add a ebugging assertion?)

I'll look into it.

[PATCH 13/19] xfs: tidy up xfs_rtallocate_extent_exact

[Christoph Hellwig]

Use common code for both xfs_rtallocate_range calls by moving
the !isfree logic into the non-default branch.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 5f42422a976a3e..ea6f221c6a193c 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -349,32 +349,24 @@ xfs_rtallocate_extent_exact(
 	if (error)
 		return error;
 
-	if (isfree) {
+	if (!isfree) {
 		/*
-		 * If it is, allocate it and return success.
+		 * If not, allocate what there is, if it's at least minlen.
 		 */
-		error = xfs_rtallocate_range(args, start, maxlen);
-		if (error)
-			return error;
-		*len = maxlen;
-		*rtx = start;
-		return 0;
-	}
-	/*
-	 * If not, allocate what there is, if it's at least minlen.
-	 */
-	maxlen = next - start;
-	if (maxlen < minlen)
-		return -ENOSPC;
-
-	/*
-	 * Trim off tail of extent, if prod is specified.
-	 */
-	if (prod > 1 && (i = maxlen % prod)) {
-		maxlen -= i;
+		maxlen = next - start;
 		if (maxlen < minlen)
 			return -ENOSPC;
+
+		/*
+		 * Trim off tail of extent, if prod is specified.
+		 */
+		if (prod > 1 && (i = maxlen % prod)) {
+			maxlen -= i;
+			if (maxlen < minlen)
+				return -ENOSPC;
+		}
 	}
+
 	/*
 	 * Allocate what we can and return it.
 	 */
-- 
2.39.2

Re: [PATCH 13/19] xfs: tidy up xfs_rtallocate_extent_exact

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:32AM +0100, Christoph Hellwig wrote:
> Use common code for both xfs_rtallocate_range calls by moving
> the !isfree logic into the non-default branch.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_rtalloc.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 5f42422a976a3e..ea6f221c6a193c 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -349,32 +349,24 @@ xfs_rtallocate_extent_exact(
>  	if (error)
>  		return error;
>  
> -	if (isfree) {
> +	if (!isfree) {
>  		/*
> -		 * If it is, allocate it and return success.
> +		 * If not, allocate what there is, if it's at least minlen.
>  		 */
> -		error = xfs_rtallocate_range(args, start, maxlen);
> -		if (error)
> -			return error;
> -		*len = maxlen;
> -		*rtx = start;
> -		return 0;
> -	}
> -	/*
> -	 * If not, allocate what there is, if it's at least minlen.
> -	 */
> -	maxlen = next - start;
> -	if (maxlen < minlen)
> -		return -ENOSPC;
> -
> -	/*
> -	 * Trim off tail of extent, if prod is specified.
> -	 */
> -	if (prod > 1 && (i = maxlen % prod)) {
> -		maxlen -= i;
> +		maxlen = next - start;
>  		if (maxlen < minlen)
>  			return -ENOSPC;
> +
> +		/*
> +		 * Trim off tail of extent, if prod is specified.
> +		 */
> +		if (prod > 1 && (i = maxlen % prod)) {
> +			maxlen -= i;
> +			if (maxlen < minlen)
> +				return -ENOSPC;
> +		}
>  	}
> +
>  	/*
>  	 * Allocate what we can and return it.
>  	 */
> -- 
> 2.39.2
> 
> 

[PATCH 14/19] xfs: factor out a xfs_rtalloc_sumlevel helper

[Christoph Hellwig]

xfs_rtallocate_extent_size has two loops with nearly identical logic
in them.  Split that logic into a separate xfs_rtalloc_sumlevel helper.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index ea6f221c6a193c..2e578e726e9137 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -537,6 +537,52 @@ xfs_rtallocate_extent_near(
 	return -ENOSPC;
 }
 
+static int
+xfs_rtalloc_sumlevel(
+	struct xfs_rtalloc_args	*args,
+	int			l,	/* level number */
+	xfs_rtxlen_t		minlen,	/* minimum length to allocate */
+	xfs_rtxlen_t		maxlen,	/* maximum length to allocate */
+	xfs_rtxlen_t		prod,	/* extent product factor */
+	xfs_rtxlen_t		*len,	/* out: actual length allocated */
+	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
+{
+	xfs_fileoff_t		i;	/* bitmap block number */
+
+	for (i = 0; i < args->mp->m_sb.sb_rbmblocks; i++) {
+		xfs_suminfo_t	sum;	/* summary information for extents */
+		xfs_rtxnum_t	n;	/* next rtext to be tried */
+		int		error;
+
+		error = xfs_rtget_summary(args, l, i, &sum);
+		if (error)
+			return error;
+
+		/*
+		 * Nothing there, on to the next block.
+		 */
+		if (!sum)
+			continue;
+
+		/*
+		 * Try allocating the extent.
+		 */
+		error = xfs_rtallocate_extent_block(args, i, minlen, maxlen,
+				len, &n, prod, rtx);
+		if (error != -ENOSPC)
+			return error;
+
+		/*
+		 * If the "next block to try" returned from the allocator is
+		 * beyond the next bitmap block, skip to that bitmap block.
+		 */
+		if (xfs_rtx_to_rbmblock(args->mp, n) > i + 1)
+			i = xfs_rtx_to_rbmblock(args->mp, n) - 1;
+	}
+
+	return -ENOSPC;
+}
+
 /*
  * Allocate an extent of length minlen<=len<=maxlen, with no position
  * specified.  If we don't get maxlen then use prod to trim
@@ -551,12 +597,8 @@ xfs_rtallocate_extent_size(
 	xfs_rtxlen_t		prod,	/* extent product factor */
 	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
 {
-	struct xfs_mount	*mp = args->mp;
 	int			error;
-	xfs_fileoff_t		i;	/* bitmap block number */
 	int			l;	/* level number (loop control) */
-	xfs_rtxnum_t		n;	/* next rtext to be tried */
-	xfs_suminfo_t		sum;	/* summary information for extents */
 
 	ASSERT(minlen % prod == 0);
 	ASSERT(maxlen % prod == 0);
@@ -564,46 +606,23 @@ xfs_rtallocate_extent_size(
 
 	/*
 	 * Loop over all the levels starting with maxlen.
-	 * At each level, look at all the bitmap blocks, to see if there
-	 * are extents starting there that are long enough (>= maxlen).
-	 * Note, only on the initial level can the allocation fail if
-	 * the summary says there's an extent.
+	 *
+	 * At each level, look at all the bitmap blocks, to see if there are
+	 * extents starting there that are long enough (>= maxlen).
+	 *
+	 * Note, only on the initial level can the allocation fail if the
+	 * summary says there's an extent.
 	 */
-	for (l = xfs_highbit32(maxlen); l < mp->m_rsumlevels; l++) {
-		/*
-		 * Loop over all the bitmap blocks.
-		 */
-		for (i = 0; i < mp->m_sb.sb_rbmblocks; i++) {
-			/*
-			 * Get the summary for this level/block.
-			 */
-			error = xfs_rtget_summary(args, l, i, &sum);
-			if (error)
-				return error;
-			/*
-			 * Nothing there, on to the next block.
-			 */
-			if (!sum)
-				continue;
-			/*
-			 * Try allocating the extent.
-			 */
-			error = xfs_rtallocate_extent_block(args, i, maxlen,
-					maxlen, len, &n, prod, rtx);
-			if (error != -ENOSPC)
-				return error;
-			/*
-			 * If the "next block to try" returned from the
-			 * allocator is beyond the next bitmap block,
-			 * skip to that bitmap block.
-			 */
-			if (xfs_rtx_to_rbmblock(mp, n) > i + 1)
-				i = xfs_rtx_to_rbmblock(mp, n) - 1;
-		}
+	for (l = xfs_highbit32(maxlen); l < args->mp->m_rsumlevels; l++) {
+		error = xfs_rtalloc_sumlevel(args, l, minlen, maxlen, prod, len,
+				rtx);
+		if (error != -ENOSPC)
+			return error;
 	}
+
 	/*
-	 * Didn't find any maxlen blocks.  Try smaller ones, unless
-	 * we're asking for a fixed size extent.
+	 * Didn't find any maxlen blocks.  Try smaller ones, unless we are
+	 * looking for a fixed size extent.
 	 */
 	if (minlen > --maxlen)
 		return -ENOSPC;
@@ -612,51 +631,19 @@ xfs_rtallocate_extent_size(
 
 	/*
 	 * Loop over sizes, from maxlen down to minlen.
-	 * This time, when we do the allocations, allow smaller ones
-	 * to succeed.
+	 *
+	 * This time, when we do the allocations, allow smaller ones to succeed,
+	 * but make sure the specified minlen/maxlen are in the possible range
+	 * for this summary level.
 	 */
 	for (l = xfs_highbit32(maxlen); l >= xfs_highbit32(minlen); l--) {
-		/*
-		 * Loop over all the bitmap blocks, try an allocation
-		 * starting in that block.
-		 */
-		for (i = 0; i < mp->m_sb.sb_rbmblocks; i++) {
-			/*
-			 * Get the summary information for this level/block.
-			 */
-			error =	xfs_rtget_summary(args, l, i, &sum);
-			if (error)
-				return error;
-
-			/*
-			 * If nothing there, go on to next.
-			 */
-			if (!sum)
-				continue;
-			/*
-			 * Try the allocation.  Make sure the specified
-			 * minlen/maxlen are in the possible range for
-			 * this summary level.
-			 */
-			error = xfs_rtallocate_extent_block(args, i,
-					XFS_RTMAX(minlen, 1 << l),
-					XFS_RTMIN(maxlen, (1 << (l + 1)) - 1),
-					len, &n, prod, rtx);
-			if (error != -ENOSPC)
-				return error;
-
-			/*
-			 * If the "next block to try" returned from the
-			 * allocator is beyond the next bitmap block,
-			 * skip to that bitmap block.
-			 */
-			if (xfs_rtx_to_rbmblock(mp, n) > i + 1)
-				i = xfs_rtx_to_rbmblock(mp, n) - 1;
-		}
+		error = xfs_rtalloc_sumlevel(args, l, XFS_RTMAX(minlen, 1 << l),
+				XFS_RTMIN(maxlen, (1 << (l + 1)) - 1), prod,
+				len, rtx);
+		if (error != -ENOSPC)
+			return error;
 	}
-	/*
-	 * Got nothing, return failure.
-	 */
+
 	return -ENOSPC;
 }
 
-- 
2.39.2

Re: [PATCH 14/19] xfs: factor out a xfs_rtalloc_sumlevel helper

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:33AM +0100, Christoph Hellwig wrote:
> xfs_rtallocate_extent_size has two loops with nearly identical logic
> in them.  Split that logic into a separate xfs_rtalloc_sumlevel helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Pretty simple hoist,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_rtalloc.c | 153 ++++++++++++++++++++-----------------------
>  1 file changed, 70 insertions(+), 83 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index ea6f221c6a193c..2e578e726e9137 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -537,6 +537,52 @@ xfs_rtallocate_extent_near(
>  	return -ENOSPC;
>  }
>  
> +static int
> +xfs_rtalloc_sumlevel(
> +	struct xfs_rtalloc_args	*args,
> +	int			l,	/* level number */
> +	xfs_rtxlen_t		minlen,	/* minimum length to allocate */
> +	xfs_rtxlen_t		maxlen,	/* maximum length to allocate */
> +	xfs_rtxlen_t		prod,	/* extent product factor */
> +	xfs_rtxlen_t		*len,	/* out: actual length allocated */
> +	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
> +{
> +	xfs_fileoff_t		i;	/* bitmap block number */
> +
> +	for (i = 0; i < args->mp->m_sb.sb_rbmblocks; i++) {
> +		xfs_suminfo_t	sum;	/* summary information for extents */
> +		xfs_rtxnum_t	n;	/* next rtext to be tried */
> +		int		error;
> +
> +		error = xfs_rtget_summary(args, l, i, &sum);
> +		if (error)
> +			return error;
> +
> +		/*
> +		 * Nothing there, on to the next block.
> +		 */
> +		if (!sum)
> +			continue;
> +
> +		/*
> +		 * Try allocating the extent.
> +		 */
> +		error = xfs_rtallocate_extent_block(args, i, minlen, maxlen,
> +				len, &n, prod, rtx);
> +		if (error != -ENOSPC)
> +			return error;
> +
> +		/*
> +		 * If the "next block to try" returned from the allocator is
> +		 * beyond the next bitmap block, skip to that bitmap block.
> +		 */
> +		if (xfs_rtx_to_rbmblock(args->mp, n) > i + 1)
> +			i = xfs_rtx_to_rbmblock(args->mp, n) - 1;
> +	}
> +
> +	return -ENOSPC;
> +}
> +
>  /*
>   * Allocate an extent of length minlen<=len<=maxlen, with no position
>   * specified.  If we don't get maxlen then use prod to trim
> @@ -551,12 +597,8 @@ xfs_rtallocate_extent_size(
>  	xfs_rtxlen_t		prod,	/* extent product factor */
>  	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
>  {
> -	struct xfs_mount	*mp = args->mp;
>  	int			error;
> -	xfs_fileoff_t		i;	/* bitmap block number */
>  	int			l;	/* level number (loop control) */
> -	xfs_rtxnum_t		n;	/* next rtext to be tried */
> -	xfs_suminfo_t		sum;	/* summary information for extents */
>  
>  	ASSERT(minlen % prod == 0);
>  	ASSERT(maxlen % prod == 0);
> @@ -564,46 +606,23 @@ xfs_rtallocate_extent_size(
>  
>  	/*
>  	 * Loop over all the levels starting with maxlen.
> -	 * At each level, look at all the bitmap blocks, to see if there
> -	 * are extents starting there that are long enough (>= maxlen).
> -	 * Note, only on the initial level can the allocation fail if
> -	 * the summary says there's an extent.
> +	 *
> +	 * At each level, look at all the bitmap blocks, to see if there are
> +	 * extents starting there that are long enough (>= maxlen).
> +	 *
> +	 * Note, only on the initial level can the allocation fail if the
> +	 * summary says there's an extent.
>  	 */
> -	for (l = xfs_highbit32(maxlen); l < mp->m_rsumlevels; l++) {
> -		/*
> -		 * Loop over all the bitmap blocks.
> -		 */
> -		for (i = 0; i < mp->m_sb.sb_rbmblocks; i++) {
> -			/*
> -			 * Get the summary for this level/block.
> -			 */
> -			error = xfs_rtget_summary(args, l, i, &sum);
> -			if (error)
> -				return error;
> -			/*
> -			 * Nothing there, on to the next block.
> -			 */
> -			if (!sum)
> -				continue;
> -			/*
> -			 * Try allocating the extent.
> -			 */
> -			error = xfs_rtallocate_extent_block(args, i, maxlen,
> -					maxlen, len, &n, prod, rtx);
> -			if (error != -ENOSPC)
> -				return error;
> -			/*
> -			 * If the "next block to try" returned from the
> -			 * allocator is beyond the next bitmap block,
> -			 * skip to that bitmap block.
> -			 */
> -			if (xfs_rtx_to_rbmblock(mp, n) > i + 1)
> -				i = xfs_rtx_to_rbmblock(mp, n) - 1;
> -		}
> +	for (l = xfs_highbit32(maxlen); l < args->mp->m_rsumlevels; l++) {
> +		error = xfs_rtalloc_sumlevel(args, l, minlen, maxlen, prod, len,
> +				rtx);
> +		if (error != -ENOSPC)
> +			return error;
>  	}
> +
>  	/*
> -	 * Didn't find any maxlen blocks.  Try smaller ones, unless
> -	 * we're asking for a fixed size extent.
> +	 * Didn't find any maxlen blocks.  Try smaller ones, unless we are
> +	 * looking for a fixed size extent.
>  	 */
>  	if (minlen > --maxlen)
>  		return -ENOSPC;
> @@ -612,51 +631,19 @@ xfs_rtallocate_extent_size(
>  
>  	/*
>  	 * Loop over sizes, from maxlen down to minlen.
> -	 * This time, when we do the allocations, allow smaller ones
> -	 * to succeed.
> +	 *
> +	 * This time, when we do the allocations, allow smaller ones to succeed,
> +	 * but make sure the specified minlen/maxlen are in the possible range
> +	 * for this summary level.
>  	 */
>  	for (l = xfs_highbit32(maxlen); l >= xfs_highbit32(minlen); l--) {
> -		/*
> -		 * Loop over all the bitmap blocks, try an allocation
> -		 * starting in that block.
> -		 */
> -		for (i = 0; i < mp->m_sb.sb_rbmblocks; i++) {
> -			/*
> -			 * Get the summary information for this level/block.
> -			 */
> -			error =	xfs_rtget_summary(args, l, i, &sum);
> -			if (error)
> -				return error;
> -
> -			/*
> -			 * If nothing there, go on to next.
> -			 */
> -			if (!sum)
> -				continue;
> -			/*
> -			 * Try the allocation.  Make sure the specified
> -			 * minlen/maxlen are in the possible range for
> -			 * this summary level.
> -			 */
> -			error = xfs_rtallocate_extent_block(args, i,
> -					XFS_RTMAX(minlen, 1 << l),
> -					XFS_RTMIN(maxlen, (1 << (l + 1)) - 1),
> -					len, &n, prod, rtx);
> -			if (error != -ENOSPC)
> -				return error;
> -
> -			/*
> -			 * If the "next block to try" returned from the
> -			 * allocator is beyond the next bitmap block,
> -			 * skip to that bitmap block.
> -			 */
> -			if (xfs_rtx_to_rbmblock(mp, n) > i + 1)
> -				i = xfs_rtx_to_rbmblock(mp, n) - 1;
> -		}
> +		error = xfs_rtalloc_sumlevel(args, l, XFS_RTMAX(minlen, 1 << l),
> +				XFS_RTMIN(maxlen, (1 << (l + 1)) - 1), prod,
> +				len, rtx);
> +		if (error != -ENOSPC)
> +			return error;
>  	}
> -	/*
> -	 * Got nothing, return failure.
> -	 */
> +
>  	return -ENOSPC;
>  }
>  
> -- 
> 2.39.2
> 
> 

[PATCH 15/19] xfs: remove rt-wrappers from xfs_format.h

[Christoph Hellwig]

xfs_format.h has a bunch odd wrappers for helper functions and mount
structure access using RT* prefixes.  Replace them with their open coded
versions (for those that weren't entirely unused) and remove the wrappers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_format.h   |  8 --------
 fs/xfs/libxfs/xfs_rtbitmap.c | 24 ++++++++++++------------
 fs/xfs/scrub/rtsummary.c     |  2 +-
 fs/xfs/xfs_rtalloc.c         |  6 +++---
 4 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 9a88aba1589f87..82a4ab2d89e9f0 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1156,20 +1156,12 @@ static inline bool xfs_dinode_has_large_extent_counts(
 #define	XFS_DFL_RTEXTSIZE	(64 * 1024)	        /* 64kB */
 #define	XFS_MIN_RTEXTSIZE	(4 * 1024)		/* 4kB */
 
-#define	XFS_BLOCKSIZE(mp)	((mp)->m_sb.sb_blocksize)
-#define	XFS_BLOCKMASK(mp)	((mp)->m_blockmask)
-
 /*
  * RT bit manipulation macros.
  */
 #define	XFS_RTMIN(a,b)	((a) < (b) ? (a) : (b))
 #define	XFS_RTMAX(a,b)	((a) > (b) ? (a) : (b))
 
-#define	XFS_RTLOBIT(w)	xfs_lowbit32(w)
-#define	XFS_RTHIBIT(w)	xfs_highbit32(w)
-
-#define	XFS_RTBLOCKLOG(b)	xfs_highbit64(b)
-
 /*
  * Dquot and dquot block format definitions
  */
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 5773e4ea36c624..4185ccf83bab68 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -195,7 +195,7 @@ xfs_rtfind_back(
 			/*
 			 * Different.  Mark where we are and return.
 			 */
-			i = bit - XFS_RTHIBIT(wdiff);
+			i = bit - xfs_highbit32(wdiff);
 			*rtx = start - i + 1;
 			return 0;
 		}
@@ -233,7 +233,7 @@ xfs_rtfind_back(
 			/*
 			 * Different, mark where we are and return.
 			 */
-			i += XFS_NBWORD - 1 - XFS_RTHIBIT(wdiff);
+			i += XFS_NBWORD - 1 - xfs_highbit32(wdiff);
 			*rtx = start - i + 1;
 			return 0;
 		}
@@ -272,7 +272,7 @@ xfs_rtfind_back(
 			/*
 			 * Different, mark where we are and return.
 			 */
-			i += XFS_NBWORD - 1 - XFS_RTHIBIT(wdiff);
+			i += XFS_NBWORD - 1 - xfs_highbit32(wdiff);
 			*rtx = start - i + 1;
 			return 0;
 		} else
@@ -348,7 +348,7 @@ xfs_rtfind_forw(
 			/*
 			 * Different.  Mark where we are and return.
 			 */
-			i = XFS_RTLOBIT(wdiff) - bit;
+			i = xfs_lowbit32(wdiff) - bit;
 			*rtx = start + i - 1;
 			return 0;
 		}
@@ -386,7 +386,7 @@ xfs_rtfind_forw(
 			/*
 			 * Different, mark where we are and return.
 			 */
-			i += XFS_RTLOBIT(wdiff);
+			i += xfs_lowbit32(wdiff);
 			*rtx = start + i - 1;
 			return 0;
 		}
@@ -423,7 +423,7 @@ xfs_rtfind_forw(
 			/*
 			 * Different, mark where we are and return.
 			 */
-			i += XFS_RTLOBIT(wdiff);
+			i += xfs_lowbit32(wdiff);
 			*rtx = start + i - 1;
 			return 0;
 		} else
@@ -708,7 +708,7 @@ xfs_rtfree_range(
 	 */
 	if (preblock < start) {
 		error = xfs_rtmodify_summary(args,
-				XFS_RTBLOCKLOG(start - preblock),
+				xfs_highbit64(start - preblock),
 				xfs_rtx_to_rbmblock(mp, preblock), -1);
 		if (error) {
 			return error;
@@ -720,7 +720,7 @@ xfs_rtfree_range(
 	 */
 	if (postblock > end) {
 		error = xfs_rtmodify_summary(args,
-				XFS_RTBLOCKLOG(postblock - end),
+				xfs_highbit64(postblock - end),
 				xfs_rtx_to_rbmblock(mp, end + 1), -1);
 		if (error) {
 			return error;
@@ -731,7 +731,7 @@ xfs_rtfree_range(
 	 * (new) free extent.
 	 */
 	return xfs_rtmodify_summary(args,
-			XFS_RTBLOCKLOG(postblock + 1 - preblock),
+			xfs_highbit64(postblock + 1 - preblock),
 			xfs_rtx_to_rbmblock(mp, preblock), 1);
 }
 
@@ -800,7 +800,7 @@ xfs_rtcheck_range(
 			/*
 			 * Different, compute first wrong bit and return.
 			 */
-			i = XFS_RTLOBIT(wdiff) - bit;
+			i = xfs_lowbit32(wdiff) - bit;
 			*new = start + i;
 			*stat = 0;
 			return 0;
@@ -839,7 +839,7 @@ xfs_rtcheck_range(
 			/*
 			 * Different, compute first wrong bit and return.
 			 */
-			i += XFS_RTLOBIT(wdiff);
+			i += xfs_lowbit32(wdiff);
 			*new = start + i;
 			*stat = 0;
 			return 0;
@@ -877,7 +877,7 @@ xfs_rtcheck_range(
 			/*
 			 * Different, compute first wrong bit and return.
 			 */
-			i += XFS_RTLOBIT(wdiff);
+			i += xfs_lowbit32(wdiff);
 			*new = start + i;
 			*stat = 0;
 			return 0;
diff --git a/fs/xfs/scrub/rtsummary.c b/fs/xfs/scrub/rtsummary.c
index 8b15c47408d031..0689025aa4849d 100644
--- a/fs/xfs/scrub/rtsummary.c
+++ b/fs/xfs/scrub/rtsummary.c
@@ -143,7 +143,7 @@ xchk_rtsum_record_free(
 
 	/* Compute the relevant location in the rtsum file. */
 	rbmoff = xfs_rtx_to_rbmblock(mp, rec->ar_startext);
-	lenlog = XFS_RTBLOCKLOG(rec->ar_extcount);
+	lenlog = xfs_highbit64(rec->ar_extcount);
 	offs = xfs_rtsumoffs(mp, lenlog, rbmoff);
 
 	rtbno = xfs_rtx_to_rtb(mp, rec->ar_startext);
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 2e578e726e9137..70b4eb840df4f3 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -156,7 +156,7 @@ xfs_rtallocate_range(
 	 * (old) free extent.
 	 */
 	error = xfs_rtmodify_summary(args,
-			XFS_RTBLOCKLOG(postblock + 1 - preblock),
+			xfs_highbit64(postblock + 1 - preblock),
 			xfs_rtx_to_rbmblock(mp, preblock), -1);
 	if (error)
 		return error;
@@ -167,7 +167,7 @@ xfs_rtallocate_range(
 	 */
 	if (preblock < start) {
 		error = xfs_rtmodify_summary(args,
-				XFS_RTBLOCKLOG(start - preblock),
+				xfs_highbit64(start - preblock),
 				xfs_rtx_to_rbmblock(mp, preblock), 1);
 		if (error)
 			return error;
@@ -179,7 +179,7 @@ xfs_rtallocate_range(
 	 */
 	if (postblock > end) {
 		error = xfs_rtmodify_summary(args,
-				XFS_RTBLOCKLOG(postblock - end),
+				xfs_highbit64(postblock - end),
 				xfs_rtx_to_rbmblock(mp, end + 1), 1);
 		if (error)
 			return error;
-- 
2.39.2

Re: [PATCH 15/19] xfs: remove rt-wrappers from xfs_format.h

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:34AM +0100, Christoph Hellwig wrote:
> xfs_format.h has a bunch odd wrappers for helper functions and mount
> structure access using RT* prefixes.  Replace them with their open coded
> versions (for those that weren't entirely unused) and remove the wrappers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_format.h   |  8 --------
>  fs/xfs/libxfs/xfs_rtbitmap.c | 24 ++++++++++++------------
>  fs/xfs/scrub/rtsummary.c     |  2 +-
>  fs/xfs/xfs_rtalloc.c         |  6 +++---
>  4 files changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 9a88aba1589f87..82a4ab2d89e9f0 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1156,20 +1156,12 @@ static inline bool xfs_dinode_has_large_extent_counts(
>  #define	XFS_DFL_RTEXTSIZE	(64 * 1024)	        /* 64kB */
>  #define	XFS_MIN_RTEXTSIZE	(4 * 1024)		/* 4kB */
>  
> -#define	XFS_BLOCKSIZE(mp)	((mp)->m_sb.sb_blocksize)
> -#define	XFS_BLOCKMASK(mp)	((mp)->m_blockmask)

Apparently I forgot to get rid of these when I demacro'd the code.
Thanks for picking that up,

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

--D

> -
>  /*
>   * RT bit manipulation macros.
>   */
>  #define	XFS_RTMIN(a,b)	((a) < (b) ? (a) : (b))
>  #define	XFS_RTMAX(a,b)	((a) > (b) ? (a) : (b))
>  
> -#define	XFS_RTLOBIT(w)	xfs_lowbit32(w)
> -#define	XFS_RTHIBIT(w)	xfs_highbit32(w)
> -
> -#define	XFS_RTBLOCKLOG(b)	xfs_highbit64(b)
> -
>  /*
>   * Dquot and dquot block format definitions
>   */
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 5773e4ea36c624..4185ccf83bab68 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -195,7 +195,7 @@ xfs_rtfind_back(
>  			/*
>  			 * Different.  Mark where we are and return.
>  			 */
> -			i = bit - XFS_RTHIBIT(wdiff);
> +			i = bit - xfs_highbit32(wdiff);
>  			*rtx = start - i + 1;
>  			return 0;
>  		}
> @@ -233,7 +233,7 @@ xfs_rtfind_back(
>  			/*
>  			 * Different, mark where we are and return.
>  			 */
> -			i += XFS_NBWORD - 1 - XFS_RTHIBIT(wdiff);
> +			i += XFS_NBWORD - 1 - xfs_highbit32(wdiff);
>  			*rtx = start - i + 1;
>  			return 0;
>  		}
> @@ -272,7 +272,7 @@ xfs_rtfind_back(
>  			/*
>  			 * Different, mark where we are and return.
>  			 */
> -			i += XFS_NBWORD - 1 - XFS_RTHIBIT(wdiff);
> +			i += XFS_NBWORD - 1 - xfs_highbit32(wdiff);
>  			*rtx = start - i + 1;
>  			return 0;
>  		} else
> @@ -348,7 +348,7 @@ xfs_rtfind_forw(
>  			/*
>  			 * Different.  Mark where we are and return.
>  			 */
> -			i = XFS_RTLOBIT(wdiff) - bit;
> +			i = xfs_lowbit32(wdiff) - bit;
>  			*rtx = start + i - 1;
>  			return 0;
>  		}
> @@ -386,7 +386,7 @@ xfs_rtfind_forw(
>  			/*
>  			 * Different, mark where we are and return.
>  			 */
> -			i += XFS_RTLOBIT(wdiff);
> +			i += xfs_lowbit32(wdiff);
>  			*rtx = start + i - 1;
>  			return 0;
>  		}
> @@ -423,7 +423,7 @@ xfs_rtfind_forw(
>  			/*
>  			 * Different, mark where we are and return.
>  			 */
> -			i += XFS_RTLOBIT(wdiff);
> +			i += xfs_lowbit32(wdiff);
>  			*rtx = start + i - 1;
>  			return 0;
>  		} else
> @@ -708,7 +708,7 @@ xfs_rtfree_range(
>  	 */
>  	if (preblock < start) {
>  		error = xfs_rtmodify_summary(args,
> -				XFS_RTBLOCKLOG(start - preblock),
> +				xfs_highbit64(start - preblock),
>  				xfs_rtx_to_rbmblock(mp, preblock), -1);
>  		if (error) {
>  			return error;
> @@ -720,7 +720,7 @@ xfs_rtfree_range(
>  	 */
>  	if (postblock > end) {
>  		error = xfs_rtmodify_summary(args,
> -				XFS_RTBLOCKLOG(postblock - end),
> +				xfs_highbit64(postblock - end),
>  				xfs_rtx_to_rbmblock(mp, end + 1), -1);
>  		if (error) {
>  			return error;
> @@ -731,7 +731,7 @@ xfs_rtfree_range(
>  	 * (new) free extent.
>  	 */
>  	return xfs_rtmodify_summary(args,
> -			XFS_RTBLOCKLOG(postblock + 1 - preblock),
> +			xfs_highbit64(postblock + 1 - preblock),
>  			xfs_rtx_to_rbmblock(mp, preblock), 1);
>  }
>  
> @@ -800,7 +800,7 @@ xfs_rtcheck_range(
>  			/*
>  			 * Different, compute first wrong bit and return.
>  			 */
> -			i = XFS_RTLOBIT(wdiff) - bit;
> +			i = xfs_lowbit32(wdiff) - bit;
>  			*new = start + i;
>  			*stat = 0;
>  			return 0;
> @@ -839,7 +839,7 @@ xfs_rtcheck_range(
>  			/*
>  			 * Different, compute first wrong bit and return.
>  			 */
> -			i += XFS_RTLOBIT(wdiff);
> +			i += xfs_lowbit32(wdiff);
>  			*new = start + i;
>  			*stat = 0;
>  			return 0;
> @@ -877,7 +877,7 @@ xfs_rtcheck_range(
>  			/*
>  			 * Different, compute first wrong bit and return.
>  			 */
> -			i += XFS_RTLOBIT(wdiff);
> +			i += xfs_lowbit32(wdiff);
>  			*new = start + i;
>  			*stat = 0;
>  			return 0;
> diff --git a/fs/xfs/scrub/rtsummary.c b/fs/xfs/scrub/rtsummary.c
> index 8b15c47408d031..0689025aa4849d 100644
> --- a/fs/xfs/scrub/rtsummary.c
> +++ b/fs/xfs/scrub/rtsummary.c
> @@ -143,7 +143,7 @@ xchk_rtsum_record_free(
>  
>  	/* Compute the relevant location in the rtsum file. */
>  	rbmoff = xfs_rtx_to_rbmblock(mp, rec->ar_startext);
> -	lenlog = XFS_RTBLOCKLOG(rec->ar_extcount);
> +	lenlog = xfs_highbit64(rec->ar_extcount);
>  	offs = xfs_rtsumoffs(mp, lenlog, rbmoff);
>  
>  	rtbno = xfs_rtx_to_rtb(mp, rec->ar_startext);
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 2e578e726e9137..70b4eb840df4f3 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -156,7 +156,7 @@ xfs_rtallocate_range(
>  	 * (old) free extent.
>  	 */
>  	error = xfs_rtmodify_summary(args,
> -			XFS_RTBLOCKLOG(postblock + 1 - preblock),
> +			xfs_highbit64(postblock + 1 - preblock),
>  			xfs_rtx_to_rbmblock(mp, preblock), -1);
>  	if (error)
>  		return error;
> @@ -167,7 +167,7 @@ xfs_rtallocate_range(
>  	 */
>  	if (preblock < start) {
>  		error = xfs_rtmodify_summary(args,
> -				XFS_RTBLOCKLOG(start - preblock),
> +				xfs_highbit64(start - preblock),
>  				xfs_rtx_to_rbmblock(mp, preblock), 1);
>  		if (error)
>  			return error;
> @@ -179,7 +179,7 @@ xfs_rtallocate_range(
>  	 */
>  	if (postblock > end) {
>  		error = xfs_rtmodify_summary(args,
> -				XFS_RTBLOCKLOG(postblock - end),
> +				xfs_highbit64(postblock - end),
>  				xfs_rtx_to_rbmblock(mp, end + 1), 1);
>  		if (error)
>  			return error;
> -- 
> 2.39.2
> 
> 

[PATCH 16/19] xfs: remove XFS_RTMIN/XFS_RTMAX

[Christoph Hellwig]

Use the kernel min/max helpers instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_format.h   | 6 ------
 fs/xfs/libxfs/xfs_rtbitmap.c | 8 ++++----
 fs/xfs/xfs_rtalloc.c         | 7 ++++---
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 82a4ab2d89e9f0..f11e7c8d336993 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1156,12 +1156,6 @@ static inline bool xfs_dinode_has_large_extent_counts(
 #define	XFS_DFL_RTEXTSIZE	(64 * 1024)	        /* 64kB */
 #define	XFS_MIN_RTEXTSIZE	(4 * 1024)		/* 4kB */
 
-/*
- * RT bit manipulation macros.
- */
-#define	XFS_RTMIN(a,b)	((a) < (b) ? (a) : (b))
-#define	XFS_RTMAX(a,b)	((a) > (b) ? (a) : (b))
-
 /*
  * Dquot and dquot block format definitions
  */
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 4185ccf83bab68..31100120b2c586 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -184,7 +184,7 @@ xfs_rtfind_back(
 		 * Calculate first (leftmost) bit number to look at,
 		 * and mask for all the relevant bits in this word.
 		 */
-		firstbit = XFS_RTMAX((xfs_srtblock_t)(bit - len + 1), 0);
+		firstbit = max_t(xfs_srtblock_t, bit - len + 1, 0);
 		mask = (((xfs_rtword_t)1 << (bit - firstbit + 1)) - 1) <<
 			firstbit;
 		/*
@@ -338,7 +338,7 @@ xfs_rtfind_forw(
 		 * Calculate last (rightmost) bit number to look at,
 		 * and mask for all the relevant bits in this word.
 		 */
-		lastbit = XFS_RTMIN(bit + len, XFS_NBWORD);
+		lastbit = min(bit + len, XFS_NBWORD);
 		mask = (((xfs_rtword_t)1 << (lastbit - bit)) - 1) << bit;
 		/*
 		 * Calculate the difference between the value there
@@ -573,7 +573,7 @@ xfs_rtmodify_range(
 		/*
 		 * Compute first bit not changed and mask of relevant bits.
 		 */
-		lastbit = XFS_RTMIN(bit + len, XFS_NBWORD);
+		lastbit = min(bit + len, XFS_NBWORD);
 		mask = (((xfs_rtword_t)1 << (lastbit - bit)) - 1) << bit;
 		/*
 		 * Set/clear the active bits.
@@ -787,7 +787,7 @@ xfs_rtcheck_range(
 		/*
 		 * Compute first bit not examined.
 		 */
-		lastbit = XFS_RTMIN(bit + len, XFS_NBWORD);
+		lastbit = min(bit + len, XFS_NBWORD);
 		/*
 		 * Mask of relevant bits.
 		 */
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 70b4eb840df4f3..24d74c8b532e5f 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -637,9 +637,10 @@ xfs_rtallocate_extent_size(
 	 * for this summary level.
 	 */
 	for (l = xfs_highbit32(maxlen); l >= xfs_highbit32(minlen); l--) {
-		error = xfs_rtalloc_sumlevel(args, l, XFS_RTMAX(minlen, 1 << l),
-				XFS_RTMIN(maxlen, (1 << (l + 1)) - 1), prod,
-				len, rtx);
+		error = xfs_rtalloc_sumlevel(args, l,
+				max_t(xfs_rtxlen_t, minlen, 1 << l),
+				min_t(xfs_rtxlen_t, maxlen, (1 << (l + 1)) - 1),
+				prod, len, rtx);
 		if (error != -ENOSPC)
 			return error;
 	}
-- 
2.39.2

Re: [PATCH 16/19] xfs: remove XFS_RTMIN/XFS_RTMAX

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:35AM +0100, Christoph Hellwig wrote:
> Use the kernel min/max helpers instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_format.h   | 6 ------
>  fs/xfs/libxfs/xfs_rtbitmap.c | 8 ++++----
>  fs/xfs/xfs_rtalloc.c         | 7 ++++---
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 82a4ab2d89e9f0..f11e7c8d336993 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1156,12 +1156,6 @@ static inline bool xfs_dinode_has_large_extent_counts(
>  #define	XFS_DFL_RTEXTSIZE	(64 * 1024)	        /* 64kB */
>  #define	XFS_MIN_RTEXTSIZE	(4 * 1024)		/* 4kB */
>  
> -/*
> - * RT bit manipulation macros.
> - */
> -#define	XFS_RTMIN(a,b)	((a) < (b) ? (a) : (b))
> -#define	XFS_RTMAX(a,b)	((a) > (b) ? (a) : (b))
> -
>  /*
>   * Dquot and dquot block format definitions
>   */
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 4185ccf83bab68..31100120b2c586 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -184,7 +184,7 @@ xfs_rtfind_back(
>  		 * Calculate first (leftmost) bit number to look at,
>  		 * and mask for all the relevant bits in this word.
>  		 */
> -		firstbit = XFS_RTMAX((xfs_srtblock_t)(bit - len + 1), 0);
> +		firstbit = max_t(xfs_srtblock_t, bit - len + 1, 0);
>  		mask = (((xfs_rtword_t)1 << (bit - firstbit + 1)) - 1) <<
>  			firstbit;
>  		/*
> @@ -338,7 +338,7 @@ xfs_rtfind_forw(
>  		 * Calculate last (rightmost) bit number to look at,
>  		 * and mask for all the relevant bits in this word.
>  		 */
> -		lastbit = XFS_RTMIN(bit + len, XFS_NBWORD);
> +		lastbit = min(bit + len, XFS_NBWORD);
>  		mask = (((xfs_rtword_t)1 << (lastbit - bit)) - 1) << bit;
>  		/*
>  		 * Calculate the difference between the value there
> @@ -573,7 +573,7 @@ xfs_rtmodify_range(
>  		/*
>  		 * Compute first bit not changed and mask of relevant bits.
>  		 */
> -		lastbit = XFS_RTMIN(bit + len, XFS_NBWORD);
> +		lastbit = min(bit + len, XFS_NBWORD);
>  		mask = (((xfs_rtword_t)1 << (lastbit - bit)) - 1) << bit;
>  		/*
>  		 * Set/clear the active bits.
> @@ -787,7 +787,7 @@ xfs_rtcheck_range(
>  		/*
>  		 * Compute first bit not examined.
>  		 */
> -		lastbit = XFS_RTMIN(bit + len, XFS_NBWORD);
> +		lastbit = min(bit + len, XFS_NBWORD);
>  		/*
>  		 * Mask of relevant bits.
>  		 */
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 70b4eb840df4f3..24d74c8b532e5f 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -637,9 +637,10 @@ xfs_rtallocate_extent_size(
>  	 * for this summary level.
>  	 */
>  	for (l = xfs_highbit32(maxlen); l >= xfs_highbit32(minlen); l--) {
> -		error = xfs_rtalloc_sumlevel(args, l, XFS_RTMAX(minlen, 1 << l),
> -				XFS_RTMIN(maxlen, (1 << (l + 1)) - 1), prod,
> -				len, rtx);
> +		error = xfs_rtalloc_sumlevel(args, l,
> +				max_t(xfs_rtxlen_t, minlen, 1 << l),
> +				min_t(xfs_rtxlen_t, maxlen, (1 << (l + 1)) - 1),

Oof, the minlen/maxlen arguments are not pretty.  OTOH I can't come up
with a better name for the loop body versions of minlen/maxlen.

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

--D

> +				prod, len, rtx);
>  		if (error != -ENOSPC)
>  			return error;
>  	}
> -- 
> 2.39.2
> 
> 

[PATCH 17/19] xfs: reorder the minlen and prod calculations in xfs_bmap_rtalloc

[Christoph Hellwig]

xfs_bmap_rtalloc is a bit of a mess in terms of calculating the locally
need variables.  Reorder them a bit so that related code is located
next to each other - the raminlen calculation moves up next to where
the maximum len is calculated, and all the prod calculation is move
into a single place and rearranged so that the real prod calculation
only happens when it actually is needed.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 24d74c8b532e5f..595740d18dc4c3 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1369,7 +1369,6 @@ xfs_bmap_rtalloc(
 
 	align = xfs_get_extsz_hint(ap->ip);
 retry:
-	prod = xfs_extlen_to_rtxlen(mp, align);
 	error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
 					align, 1, ap->eof, 0,
 					ap->conv, &ap->offset, &ap->length);
@@ -1387,13 +1386,6 @@ xfs_bmap_rtalloc(
 	if (ap->offset != orig_offset)
 		minlen += orig_offset - ap->offset;
 
-	/*
-	 * If the offset & length are not perfectly aligned
-	 * then kill prod, it will just get us in trouble.
-	 */
-	div_u64_rem(ap->offset, align, &mod);
-	if (mod || ap->length % align)
-		prod = 1;
 	/*
 	 * Set ralen to be the actual requested length in rtextents.
 	 *
@@ -1404,6 +1396,7 @@ xfs_bmap_rtalloc(
 	 * adjust the starting point to match it.
 	 */
 	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
+	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
 
 	/*
 	 * Lock out modifications to both the RT bitmap and summary inodes
@@ -1432,7 +1425,16 @@ xfs_bmap_rtalloc(
 		start = 0;
 	}
 
-	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
+	/*
+	 * Only bother calculating a real prod factor if offset & length are
+	 * perfectly aligned, otherwise it will just get us in trouble.
+	 */
+	div_u64_rem(ap->offset, align, &mod);
+	if (mod || ap->length % align)
+		prod = 1;
+	else
+		prod = xfs_extlen_to_rtxlen(mp, align);
+
 	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
 			ap->wasdel, prod, &rtx);
 	if (error == -ENOSPC) {
-- 
2.39.2

Re: [PATCH 17/19] xfs: reorder the minlen and prod calculations in xfs_bmap_rtalloc

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:36AM +0100, Christoph Hellwig wrote:
> xfs_bmap_rtalloc is a bit of a mess in terms of calculating the locally
> need variables.  Reorder them a bit so that related code is located
> next to each other - the raminlen calculation moves up next to where
> the maximum len is calculated, and all the prod calculation is move
> into a single place and rearranged so that the real prod calculation
> only happens when it actually is needed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This looks like a simple enough change,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_rtalloc.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 24d74c8b532e5f..595740d18dc4c3 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1369,7 +1369,6 @@ xfs_bmap_rtalloc(
>  
>  	align = xfs_get_extsz_hint(ap->ip);
>  retry:
> -	prod = xfs_extlen_to_rtxlen(mp, align);
>  	error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
>  					align, 1, ap->eof, 0,
>  					ap->conv, &ap->offset, &ap->length);
> @@ -1387,13 +1386,6 @@ xfs_bmap_rtalloc(
>  	if (ap->offset != orig_offset)
>  		minlen += orig_offset - ap->offset;
>  
> -	/*
> -	 * If the offset & length are not perfectly aligned
> -	 * then kill prod, it will just get us in trouble.
> -	 */
> -	div_u64_rem(ap->offset, align, &mod);
> -	if (mod || ap->length % align)
> -		prod = 1;
>  	/*
>  	 * Set ralen to be the actual requested length in rtextents.
>  	 *
> @@ -1404,6 +1396,7 @@ xfs_bmap_rtalloc(
>  	 * adjust the starting point to match it.
>  	 */
>  	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
> +	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
>  
>  	/*
>  	 * Lock out modifications to both the RT bitmap and summary inodes
> @@ -1432,7 +1425,16 @@ xfs_bmap_rtalloc(
>  		start = 0;
>  	}
>  
> -	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
> +	/*
> +	 * Only bother calculating a real prod factor if offset & length are
> +	 * perfectly aligned, otherwise it will just get us in trouble.
> +	 */
> +	div_u64_rem(ap->offset, align, &mod);
> +	if (mod || ap->length % align)
> +		prod = 1;
> +	else
> +		prod = xfs_extlen_to_rtxlen(mp, align);
> +
>  	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
>  			ap->wasdel, prod, &rtx);
>  	if (error == -ENOSPC) {
> -- 
> 2.39.2
> 
> 

[PATCH 18/19] xfs: simplify and optimize the RT allocation fallback cascade

[Christoph Hellwig]

There are currently multiple levels of fall back if an RT allocation
can not be satisfied:

 1) xfs_rtallocate_extent extents the minlen and reduces the maxlen due
    to the extent size hint.  If that can't be done, it return -ENOSPC
    and let's xfs_bmap_rtalloc retry, which then not only drops the
    extent size hint based alignment, but also the minlen adjustment
 2) if xfs_rtallocate_extent gets -ENOSPC from the underlying functions,
    it only drops the extent size hint based alignment and retries
 3) if that still does not succeed, xfs_rtallocate_extent drops the
    extent size hint (which is a complex no-op at this point) and the
    minlen using the same code as (1) above
 4) if that still doesn't success and the caller wanted an allocation
    near a blkno, drop that blkno hint.

The handling in 1 is rather inefficient as we could just drop the
alignment and continue, and 2/3 interact in really weird ways due to
the duplicate policy.

Move aligning the min and maxlen out of xfs_rtallocate_extent and into
a helper called directly by xfs_bmap_rtalloc.  This allows just
continuing with the allocation if we have to drop the alignment instead
of going through the retry loop and also dropping the perfectly the
minlen adjustment that didn't cause the problem, and then just use
a single retry that drops both the minlen and alignment requirement
when we really are out of space, thus consolidating cases (2) and (3)
above.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 595740d18dc4c3..16255617629ef5 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1088,21 +1088,6 @@ xfs_rtallocate_extent(
 	ASSERT(xfs_isilocked(args.mp->m_rbmip, XFS_ILOCK_EXCL));
 	ASSERT(minlen > 0 && minlen <= maxlen);
 
-	/*
-	 * If prod is set then figure out what to do to minlen and maxlen.
-	 */
-	if (prod > 1) {
-		xfs_rtxlen_t	i;
-
-		if ((i = maxlen % prod))
-			maxlen -= i;
-		if ((i = minlen % prod))
-			minlen += prod - i;
-		if (maxlen < minlen)
-			return -ENOSPC;
-	}
-
-retry:
 	if (start == 0) {
 		error = xfs_rtallocate_extent_size(&args, minlen,
 				maxlen, len, prod, rtx);
@@ -1111,13 +1096,8 @@ xfs_rtallocate_extent(
 				maxlen, len, prod, rtx);
 	}
 	xfs_rtbuf_cache_relse(&args);
-	if (error) {
-		if (error == -ENOSPC && prod > 1) {
-			prod = 1;
-			goto retry;
-		}
+	if (error)
 		return error;
-	}
 
 	/*
 	 * If it worked, update the superblock.
@@ -1348,6 +1328,35 @@ xfs_rtpick_extent(
 	return 0;
 }
 
+static void
+xfs_rtalloc_align_minmax(
+	xfs_rtxlen_t		*raminlen,
+	xfs_rtxlen_t		*ramaxlen,
+	xfs_rtxlen_t		*prod)
+{
+	xfs_rtxlen_t		newmaxlen = *ramaxlen;
+	xfs_rtxlen_t		newminlen = *raminlen;
+	xfs_rtxlen_t		slack;
+
+	slack = newmaxlen % *prod;
+	if (slack)
+		newmaxlen -= slack;
+	slack = newminlen % *prod;
+	if (slack)
+		newminlen += *prod - slack;
+
+	/*
+	 * If adjusting for extent size hint alignment produces an invalid
+	 * min/max len combination, go ahead without it.
+	 */
+	if (newmaxlen < newminlen) {
+		*prod = 1;
+		return;
+	}
+	*ramaxlen = newmaxlen;
+	*raminlen = newminlen;
+}
+
 int
 xfs_bmap_rtalloc(
 	struct xfs_bmalloca	*ap)
@@ -1430,10 +1439,13 @@ xfs_bmap_rtalloc(
 	 * perfectly aligned, otherwise it will just get us in trouble.
 	 */
 	div_u64_rem(ap->offset, align, &mod);
-	if (mod || ap->length % align)
+	if (mod || ap->length % align) {
 		prod = 1;
-	else
+	} else {
 		prod = xfs_extlen_to_rtxlen(mp, align);
+		if (prod > 1)
+			xfs_rtalloc_align_minmax(&raminlen, &ralen, &prod);
+	}
 
 	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
 			ap->wasdel, prod, &rtx);
-- 
2.39.2

Re: [PATCH 18/19] xfs: simplify and optimize the RT allocation fallback cascade

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:37AM +0100, Christoph Hellwig wrote:
> There are currently multiple levels of fall back if an RT allocation
> can not be satisfied:
> 
>  1) xfs_rtallocate_extent extents the minlen and reduces the maxlen due

                            ^^^^^^^ extends?

>     to the extent size hint.  If that can't be done, it return -ENOSPC
>     and let's xfs_bmap_rtalloc retry, which then not only drops the
>     extent size hint based alignment, but also the minlen adjustment
>  2) if xfs_rtallocate_extent gets -ENOSPC from the underlying functions,
>     it only drops the extent size hint based alignment and retries
>  3) if that still does not succeed, xfs_rtallocate_extent drops the
>     extent size hint (which is a complex no-op at this point) and the
>     minlen using the same code as (1) above
>  4) if that still doesn't success and the caller wanted an allocation
>     near a blkno, drop that blkno hint.
> 
> The handling in 1 is rather inefficient as we could just drop the
> alignment and continue, and 2/3 interact in really weird ways due to
> the duplicate policy.
> 
> Move aligning the min and maxlen out of xfs_rtallocate_extent and into
> a helper called directly by xfs_bmap_rtalloc.  This allows just
> continuing with the allocation if we have to drop the alignment instead
> of going through the retry loop and also dropping the perfectly the
> minlen adjustment that didn't cause the problem, and then just use

"...dropping the perfectly *usable* minlen adjustment..." ?

> a single retry that drops both the minlen and alignment requirement
> when we really are out of space, thus consolidating cases (2) and (3)
> above.

How can we drop the minlen requirement, won't that result in undersize
mapping allocations?  Or is the subtlety here that for realtime files,
that's ok because we never have forced multi-rtx allocations like we do
for the data device?

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_rtalloc.c | 58 ++++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 595740d18dc4c3..16255617629ef5 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1088,21 +1088,6 @@ xfs_rtallocate_extent(
>  	ASSERT(xfs_isilocked(args.mp->m_rbmip, XFS_ILOCK_EXCL));
>  	ASSERT(minlen > 0 && minlen <= maxlen);
>  
> -	/*
> -	 * If prod is set then figure out what to do to minlen and maxlen.
> -	 */
> -	if (prod > 1) {
> -		xfs_rtxlen_t	i;
> -
> -		if ((i = maxlen % prod))
> -			maxlen -= i;
> -		if ((i = minlen % prod))
> -			minlen += prod - i;
> -		if (maxlen < minlen)
> -			return -ENOSPC;
> -	}
> -
> -retry:
>  	if (start == 0) {
>  		error = xfs_rtallocate_extent_size(&args, minlen,
>  				maxlen, len, prod, rtx);
> @@ -1111,13 +1096,8 @@ xfs_rtallocate_extent(
>  				maxlen, len, prod, rtx);
>  	}
>  	xfs_rtbuf_cache_relse(&args);
> -	if (error) {
> -		if (error == -ENOSPC && prod > 1) {
> -			prod = 1;
> -			goto retry;
> -		}
> +	if (error)
>  		return error;
> -	}
>  
>  	/*
>  	 * If it worked, update the superblock.
> @@ -1348,6 +1328,35 @@ xfs_rtpick_extent(
>  	return 0;
>  }
>  
> +static void
> +xfs_rtalloc_align_minmax(
> +	xfs_rtxlen_t		*raminlen,
> +	xfs_rtxlen_t		*ramaxlen,
> +	xfs_rtxlen_t		*prod)
> +{
> +	xfs_rtxlen_t		newmaxlen = *ramaxlen;
> +	xfs_rtxlen_t		newminlen = *raminlen;
> +	xfs_rtxlen_t		slack;
> +
> +	slack = newmaxlen % *prod;
> +	if (slack)
> +		newmaxlen -= slack;
> +	slack = newminlen % *prod;
> +	if (slack)
> +		newminlen += *prod - slack;
> +
> +	/*
> +	 * If adjusting for extent size hint alignment produces an invalid
> +	 * min/max len combination, go ahead without it.
> +	 */
> +	if (newmaxlen < newminlen) {
> +		*prod = 1;
> +		return;
> +	}
> +	*ramaxlen = newmaxlen;
> +	*raminlen = newminlen;
> +}
> +
>  int
>  xfs_bmap_rtalloc(
>  	struct xfs_bmalloca	*ap)
> @@ -1430,10 +1439,13 @@ xfs_bmap_rtalloc(
>  	 * perfectly aligned, otherwise it will just get us in trouble.
>  	 */
>  	div_u64_rem(ap->offset, align, &mod);
> -	if (mod || ap->length % align)
> +	if (mod || ap->length % align) {
>  		prod = 1;
> -	else
> +	} else {
>  		prod = xfs_extlen_to_rtxlen(mp, align);
> +		if (prod > 1)
> +			xfs_rtalloc_align_minmax(&raminlen, &ralen, &prod);
> +	}
>  
>  	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
>  			ap->wasdel, prod, &rtx);
> -- 
> 2.39.2
> 
> 

Re: [PATCH 18/19] xfs: simplify and optimize the RT allocation fallback cascade

[Christoph Hellwig]

On Thu, Dec 14, 2023 at 01:32:21PM -0800, Darrick J. Wong wrote:
> >  1) xfs_rtallocate_extent extents the minlen and reduces the maxlen due
> 
>                             ^^^^^^^ extends?

Yes.  I'm definitively talking about extents too much in my life :)

> > Move aligning the min and maxlen out of xfs_rtallocate_extent and into
> > a helper called directly by xfs_bmap_rtalloc.  This allows just
> > continuing with the allocation if we have to drop the alignment instead
> > of going through the retry loop and also dropping the perfectly the
> > minlen adjustment that didn't cause the problem, and then just use
> 
> "...dropping the perfectly *usable* minlen adjustment..." ?
> 
> > a single retry that drops both the minlen and alignment requirement
> > when we really are out of space, thus consolidating cases (2) and (3)
> > above.
> 
> How can we drop the minlen requirement, won't that result in undersize
> mapping allocations?  Or is the subtlety here that for realtime files,
> that's ok because we never have forced multi-rtx allocations like we do
> for the data device?

The rtalloc minlen is different from the bmap minlen.  The bmap minlen
is always 1 except for metadata XFS_BMAPI_CONTIG allocations, which
obviosuly can't happen for RT allocations.  The rtalloc minlen starts
out as a single rtextent and is increases when we adjust the physical
allocation location to better align with the previous extent.

[PATCH 19/19] xfs: fold xfs_rtallocate_extent into xfs_bmap_rtalloc

[Christoph Hellwig]

There isn't really much left in xfs_rtallocate_extent now, fold it into
the only caller.

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

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 16255617629ef5..cbcdf604756fd3 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1063,53 +1063,6 @@ xfs_growfs_rt(
 	return error;
 }
 
-/*
- * Allocate an extent in the realtime subvolume, with the usual allocation
- * parameters.  The length units are all in realtime extents, as is the
- * result block number.
- */
-static int
-xfs_rtallocate_extent(
-	struct xfs_trans	*tp,
-	xfs_rtxnum_t		start,	/* starting rtext number to allocate */
-	xfs_rtxlen_t		minlen,	/* minimum length to allocate */
-	xfs_rtxlen_t		maxlen,	/* maximum length to allocate */
-	xfs_rtxlen_t		*len,	/* out: actual length allocated */
-	int			wasdel,	/* was a delayed allocation extent */
-	xfs_rtxlen_t		prod,	/* extent product factor */
-	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
-{
-	struct xfs_rtalloc_args	args = {
-		.mp		= tp->t_mountp,
-		.tp		= tp,
-	};
-	int			error;	/* error value */
-
-	ASSERT(xfs_isilocked(args.mp->m_rbmip, XFS_ILOCK_EXCL));
-	ASSERT(minlen > 0 && minlen <= maxlen);
-
-	if (start == 0) {
-		error = xfs_rtallocate_extent_size(&args, minlen,
-				maxlen, len, prod, rtx);
-	} else {
-		error = xfs_rtallocate_extent_near(&args, start, minlen,
-				maxlen, len, prod, rtx);
-	}
-	xfs_rtbuf_cache_relse(&args);
-	if (error)
-		return error;
-
-	/*
-	 * If it worked, update the superblock.
-	 */
-	ASSERT(*len >= minlen && *len <= maxlen);
-	if (wasdel)
-		xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FREXTENTS, -(long)*len);
-	else
-		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, -(long)*len);
-	return 0;
-}
-
 /*
  * Initialize realtime fields in the mount structure.
  */
@@ -1374,6 +1327,10 @@ xfs_bmap_rtalloc(
 	xfs_rtxlen_t		raminlen;
 	bool			rtlocked = false;
 	bool			ignore_locality = false;
+	struct xfs_rtalloc_args	args = {
+		.mp		= mp,
+		.tp		= ap->tp,
+	};
 	int			error;
 
 	align = xfs_get_extsz_hint(ap->ip);
@@ -1406,6 +1363,8 @@ xfs_bmap_rtalloc(
 	 */
 	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
 	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
+	ASSERT(raminlen > 0);
+	ASSERT(raminlen <= ralen);
 
 	/*
 	 * Lock out modifications to both the RT bitmap and summary inodes
@@ -1447,8 +1406,15 @@ xfs_bmap_rtalloc(
 			xfs_rtalloc_align_minmax(&raminlen, &ralen, &prod);
 	}
 
-	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
-			ap->wasdel, prod, &rtx);
+	if (start) {
+		error = xfs_rtallocate_extent_near(&args, start, raminlen,
+				ralen, &ralen, prod, &rtx);
+	} else {
+		error = xfs_rtallocate_extent_size(&args, raminlen,
+				ralen, &ralen, prod, &rtx);
+	}
+	xfs_rtbuf_cache_relse(&args);
+
 	if (error == -ENOSPC) {
 		if (align > mp->m_sb.sb_rextsize) {
 			/*
@@ -1480,6 +1446,9 @@ xfs_bmap_rtalloc(
 	if (error)
 		return error;
 
+	xfs_trans_mod_sb(ap->tp, ap->wasdel ?
+			XFS_TRANS_SB_RES_FREXTENTS : XFS_TRANS_SB_FREXTENTS,
+			-(long)ralen);
 	ap->blkno = xfs_rtx_to_rtb(mp, rtx);
 	ap->length = xfs_rtxlen_to_extlen(mp, ralen);
 	xfs_bmap_alloc_account(ap);
-- 
2.39.2

Re: [PATCH 19/19] xfs: fold xfs_rtallocate_extent into xfs_bmap_rtalloc

[Darrick J. Wong]

On Thu, Dec 14, 2023 at 07:34:38AM +0100, Christoph Hellwig wrote:
> There isn't really much left in xfs_rtallocate_extent now, fold it into
> the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good, what a nice cleanup!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_rtalloc.c | 67 ++++++++++++--------------------------------
>  1 file changed, 18 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 16255617629ef5..cbcdf604756fd3 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1063,53 +1063,6 @@ xfs_growfs_rt(
>  	return error;
>  }
>  
> -/*
> - * Allocate an extent in the realtime subvolume, with the usual allocation
> - * parameters.  The length units are all in realtime extents, as is the
> - * result block number.
> - */
> -static int
> -xfs_rtallocate_extent(
> -	struct xfs_trans	*tp,
> -	xfs_rtxnum_t		start,	/* starting rtext number to allocate */
> -	xfs_rtxlen_t		minlen,	/* minimum length to allocate */
> -	xfs_rtxlen_t		maxlen,	/* maximum length to allocate */
> -	xfs_rtxlen_t		*len,	/* out: actual length allocated */
> -	int			wasdel,	/* was a delayed allocation extent */
> -	xfs_rtxlen_t		prod,	/* extent product factor */
> -	xfs_rtxnum_t		*rtx)	/* out: start rtext allocated */
> -{
> -	struct xfs_rtalloc_args	args = {
> -		.mp		= tp->t_mountp,
> -		.tp		= tp,
> -	};
> -	int			error;	/* error value */
> -
> -	ASSERT(xfs_isilocked(args.mp->m_rbmip, XFS_ILOCK_EXCL));
> -	ASSERT(minlen > 0 && minlen <= maxlen);
> -
> -	if (start == 0) {
> -		error = xfs_rtallocate_extent_size(&args, minlen,
> -				maxlen, len, prod, rtx);
> -	} else {
> -		error = xfs_rtallocate_extent_near(&args, start, minlen,
> -				maxlen, len, prod, rtx);
> -	}
> -	xfs_rtbuf_cache_relse(&args);
> -	if (error)
> -		return error;
> -
> -	/*
> -	 * If it worked, update the superblock.
> -	 */
> -	ASSERT(*len >= minlen && *len <= maxlen);
> -	if (wasdel)
> -		xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FREXTENTS, -(long)*len);
> -	else
> -		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, -(long)*len);
> -	return 0;
> -}
> -
>  /*
>   * Initialize realtime fields in the mount structure.
>   */
> @@ -1374,6 +1327,10 @@ xfs_bmap_rtalloc(
>  	xfs_rtxlen_t		raminlen;
>  	bool			rtlocked = false;
>  	bool			ignore_locality = false;
> +	struct xfs_rtalloc_args	args = {
> +		.mp		= mp,
> +		.tp		= ap->tp,
> +	};
>  	int			error;
>  
>  	align = xfs_get_extsz_hint(ap->ip);
> @@ -1406,6 +1363,8 @@ xfs_bmap_rtalloc(
>  	 */
>  	ralen = xfs_extlen_to_rtxlen(mp, min(ap->length, XFS_MAX_BMBT_EXTLEN));
>  	raminlen = max_t(xfs_rtxlen_t, 1, xfs_extlen_to_rtxlen(mp, minlen));
> +	ASSERT(raminlen > 0);
> +	ASSERT(raminlen <= ralen);
>  
>  	/*
>  	 * Lock out modifications to both the RT bitmap and summary inodes
> @@ -1447,8 +1406,15 @@ xfs_bmap_rtalloc(
>  			xfs_rtalloc_align_minmax(&raminlen, &ralen, &prod);
>  	}
>  
> -	error = xfs_rtallocate_extent(ap->tp, start, raminlen, ralen, &ralen,
> -			ap->wasdel, prod, &rtx);
> +	if (start) {
> +		error = xfs_rtallocate_extent_near(&args, start, raminlen,
> +				ralen, &ralen, prod, &rtx);
> +	} else {
> +		error = xfs_rtallocate_extent_size(&args, raminlen,
> +				ralen, &ralen, prod, &rtx);
> +	}
> +	xfs_rtbuf_cache_relse(&args);
> +
>  	if (error == -ENOSPC) {
>  		if (align > mp->m_sb.sb_rextsize) {
>  			/*
> @@ -1480,6 +1446,9 @@ xfs_bmap_rtalloc(
>  	if (error)
>  		return error;
>  
> +	xfs_trans_mod_sb(ap->tp, ap->wasdel ?
> +			XFS_TRANS_SB_RES_FREXTENTS : XFS_TRANS_SB_FREXTENTS,
> +			-(long)ralen);
>  	ap->blkno = xfs_rtx_to_rtb(mp, rtx);
>  	ap->length = xfs_rtxlen_to_extlen(mp, ralen);
>  	xfs_bmap_alloc_account(ap);
> -- 
> 2.39.2
> 
>