[1/6] xfs: refactor small allocation helper to skip cntbt attempt
diff mbox series

Message ID 20190509165839.44329-2-bfoster@redhat.com
State New
Headers show
Series
  • xfs: rework extent allocation
Related show

Commit Message

Brian Foster May 9, 2019, 4:58 p.m. UTC
The small allocation helper is implemented in a way that is fairly
tightly integrated to the existing allocation algorithms. It expects
a cntbt cursor beyond the end of the tree, attempts to locate the
last record in the tree and only attempts an AGFL allocation if the
cntbt is empty.

The upcoming generic algorithm doesn't rely on the cntbt processing
of this function. It will only call this function when the cntbt
doesn't have a big enough extent or is empty and thus AGFL
allocation is the only remaining option. Tweak
xfs_alloc_ag_vextent_small() to handle a NULL cntbt cursor and skip
the cntbt logic. This facilitates use by the existing allocation
code and new code that only requires an AGFL allocation attempt.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c | 52 +++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

Comments

Christoph Hellwig May 10, 2019, 5:24 p.m. UTC | #1
This looks pretty sensible to me.  What confuses me a bit is that
the patch is much more (good!) refactoring than the actual change.

If you have to respin it maybe split it up, making the actual
behavior change even more obvious.

Otherwise:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Brian Foster May 13, 2019, 3:44 p.m. UTC | #2
On Fri, May 10, 2019 at 10:24:46AM -0700, Christoph Hellwig wrote:
> This looks pretty sensible to me.  What confuses me a bit is that
> the patch is much more (good!) refactoring than the actual change.
> 
> If you have to respin it maybe split it up, making the actual
> behavior change even more obvious.
> 

The only functional change was basically to check for ccur before using
it and initializing i to zero. It just seemed to make sense to clean up
the surrounding code while there, but I can either split out the
aesthetic cleanup or defer that stuff to the broader rework at the end
of the series (where the cursor stuff just gets ripped out anyways) if
either of those is cleaner..

Brian

> Otherwise:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Christoph Hellwig May 15, 2019, 7:52 a.m. UTC | #3
On Mon, May 13, 2019 at 11:44:34AM -0400, Brian Foster wrote:
> The only functional change was basically to check for ccur before using
> it and initializing i to zero. It just seemed to make sense to clean up
> the surrounding code while there, but I can either split out the
> aesthetic cleanup or defer that stuff to the broader rework at the end
> of the series (where the cursor stuff just gets ripped out anyways) if
> either of those is cleaner..

Yeah, I noticed how trivial the actual change was.  That is why just
doing the cleanup in pass 1 and applying the change in pass 2 might
make it more readbale.  It might also be worth to throw in the use
a goto label instead of long conditionals from your last patch into
that cleanup prep patch.
Brian Foster May 15, 2019, 2:41 p.m. UTC | #4
On Wed, May 15, 2019 at 12:52:53AM -0700, Christoph Hellwig wrote:
> On Mon, May 13, 2019 at 11:44:34AM -0400, Brian Foster wrote:
> > The only functional change was basically to check for ccur before using
> > it and initializing i to zero. It just seemed to make sense to clean up
> > the surrounding code while there, but I can either split out the
> > aesthetic cleanup or defer that stuff to the broader rework at the end
> > of the series (where the cursor stuff just gets ripped out anyways) if
> > either of those is cleaner..
> 
> Yeah, I noticed how trivial the actual change was.  That is why just
> doing the cleanup in pass 1 and applying the change in pass 2 might
> make it more readbale.  It might also be worth to throw in the use
> a goto label instead of long conditionals from your last patch into
> that cleanup prep patch.

Ok, I've lifted all of the small mode refactoring (from both this patch
and the last one) into an initial patch. Unless you indicate otherwise,
I've also retained your review tag(s), though you might want to take a
cursory look once the next version lands.

Brian

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index a9ff3cf82cce..55eda416e18b 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1589,33 +1589,36 @@  xfs_alloc_ag_vextent_size(
  */
 STATIC int			/* error */
 xfs_alloc_ag_vextent_small(
-	xfs_alloc_arg_t	*args,	/* allocation argument structure */
-	xfs_btree_cur_t	*ccur,	/* by-size cursor */
-	xfs_agblock_t	*fbnop,	/* result block number */
-	xfs_extlen_t	*flenp,	/* result length */
-	int		*stat)	/* status: 0-freelist, 1-normal/none */
+	struct xfs_alloc_arg	*args,	/* allocation argument structure */
+	struct xfs_btree_cur	*ccur,	/* optional by-size cursor */
+	xfs_agblock_t		*fbnop,	/* result block number */
+	xfs_extlen_t		*flenp,	/* result length */
+	int			*stat)	/* status: 0-freelist, 1-normal/none */
 {
-	int		error;
-	xfs_agblock_t	fbno;
-	xfs_extlen_t	flen;
-	int		i;
+	int			error = 0;
+	xfs_agblock_t		fbno;
+	xfs_extlen_t		flen;
+	int			i = 0;
 
-	if ((error = xfs_btree_decrement(ccur, 0, &i)))
+	/*
+	 * If a cntbt cursor is provided, try to allocate the largest record in
+	 * the tree. Try the AGFL if the cntbt is empty, otherwise fail the
+	 * allocation. Make sure to respect minleft even when pulling from the
+	 * freelist.
+	 */
+	if (ccur)
+		error = xfs_btree_decrement(ccur, 0, &i);
+	if (error)
 		goto error0;
 	if (i) {
-		if ((error = xfs_alloc_get_rec(ccur, &fbno, &flen, &i)))
+		error = xfs_alloc_get_rec(ccur, &fbno, &flen, &i);
+		if (error)
 			goto error0;
 		XFS_WANT_CORRUPTED_GOTO(args->mp, i == 1, error0);
-	}
-	/*
-	 * Nothing in the btree, try the freelist.  Make sure
-	 * to respect minleft even when pulling from the
-	 * freelist.
-	 */
-	else if (args->minlen == 1 && args->alignment == 1 &&
-		 args->resv != XFS_AG_RESV_AGFL &&
-		 (be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_flcount)
-		  > args->minleft)) {
+	} else if (args->minlen == 1 && args->alignment == 1 &&
+		   args->resv != XFS_AG_RESV_AGFL &&
+		   (be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_flcount) >
+		    args->minleft)) {
 		error = xfs_alloc_get_freelist(args->tp, args->agbp, &fbno, 0);
 		if (error)
 			goto error0;
@@ -1661,14 +1664,11 @@  xfs_alloc_ag_vextent_small(
 		 */
 		else
 			flen = 0;
-	}
-	/*
-	 * Can't allocate from the freelist for some reason.
-	 */
-	else {
+	} else {
 		fbno = NULLAGBLOCK;
 		flen = 0;
 	}
+
 	/*
 	 * Can't do the allocation, give up.
 	 */