[2/3] xfs: clean up weird while loop in xfs_alloc_ag_vextent_near
diff mbox series

Message ID 157319669798.834585.10287243947050889974.stgit@magnolia
State Accepted
Headers show
Series
  • xfs: various coverity fixes
Related show

Commit Message

Darrick J. Wong Nov. 8, 2019, 7:04 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the weird while loop out of existence.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |  119 +++++++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 53 deletions(-)

Comments

Christoph Hellwig Nov. 8, 2019, 7:11 a.m. UTC | #1
What tree is this supposed to apply to?  It fails against
xfs/for-next here.  Otherwise this looks fine to me.
Darrick J. Wong Nov. 8, 2019, 7:20 a.m. UTC | #2
On Thu, Nov 07, 2019 at 11:11:51PM -0800, Christoph Hellwig wrote:
> What tree is this supposed to apply to?  It fails against

Sorry, I've been reworking these patches against a branch that I hadn't
yet pushed to xfs-linux.git (and won't until I can take one more look
tomorrow with a fresh brain).

In the meantime, you can see my development branch on my personal git
repo:

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-5.5-dev

> xfs/for-next here.  Otherwise this looks fine to me.

Fine enough for an RVB provided the above? :D

--D
Christoph Hellwig Nov. 8, 2019, 7:22 a.m. UTC | #3
On Thu, Nov 07, 2019 at 11:20:02PM -0800, Darrick J. Wong wrote:
> On Thu, Nov 07, 2019 at 11:11:51PM -0800, Christoph Hellwig wrote:
> > What tree is this supposed to apply to?  It fails against
> 
> Sorry, I've been reworking these patches against a branch that I hadn't
> yet pushed to xfs-linux.git (and won't until I can take one more look
> tomorrow with a fresh brain).
> 
> In the meantime, you can see my development branch on my personal git
> repo:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-5.5-dev
> 
> > xfs/for-next here.  Otherwise this looks fine to me.
> 
> Fine enough for an RVB provided the above? :D

Sure, it _looks_ good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although I'd love to be able to at least do a quick xfstests run on
it..

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 60ca09bedc23..3a724d80783a 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1486,6 +1486,65 @@  xfs_alloc_ag_vextent_locality(
 	return 0;
 }
 
+/* Check the last block of the cnt btree for allocations. */
+static int
+xfs_alloc_ag_vextent_lastblock(
+	struct xfs_alloc_arg	*args,
+	struct xfs_alloc_cur	*acur,
+	xfs_agblock_t		*bno,
+	xfs_extlen_t		*len,
+	bool			*allocated)
+{
+	int			error;
+	int			i;
+
+#ifdef DEBUG
+	/* Randomly don't execute the first algorithm. */
+	if (prandom_u32() & 1)
+		return 0;
+#endif
+
+	/*
+	 * Start from the entry that lookup found, sequence through all larger
+	 * free blocks.  If we're actually pointing at a record smaller than
+	 * maxlen, go to the start of this block, and skip all those smaller
+	 * than minlen.
+	 */
+	if (len || args->alignment > 1) {
+		acur->cnt->bc_ptrs[0] = 1;
+		do {
+			error = xfs_alloc_get_rec(acur->cnt, bno, len, &i);
+			if (error)
+				return error;
+			if (XFS_IS_CORRUPT(args->mp, i != 1))
+				return -EFSCORRUPTED;
+			if (*len >= args->minlen)
+				break;
+			error = xfs_btree_increment(acur->cnt, 0, &i);
+			if (error)
+				return error;
+		} while (i);
+		ASSERT(*len >= args->minlen);
+		if (!i)
+			return 0;
+	}
+
+	error = xfs_alloc_walk_iter(args, acur, acur->cnt, true, false, -1, &i);
+	if (error)
+		return error;
+
+	/*
+	 * It didn't work.  We COULD be in a case where there's a good record
+	 * somewhere, so try again.
+	 */
+	if (acur->len == 0)
+		return 0;
+
+	trace_xfs_alloc_near_first(args);
+	*allocated = true;
+	return 0;
+}
+
 /*
  * Allocate a variable extent near bno in the allocation group agno.
  * Extent's length (returned in len) will be between minlen and maxlen,
@@ -1501,14 +1560,6 @@  xfs_alloc_ag_vextent_near(
 	int			i;		/* result code, temporary */
 	xfs_agblock_t		bno;
 	xfs_extlen_t		len;
-#ifdef DEBUG
-	/*
-	 * Randomly don't execute the first algorithm.
-	 */
-	int		dofirst;	/* set to do first algorithm */
-
-	dofirst = prandom_u32() & 1;
-#endif
 
 	/* handle uninitialized agbno range so caller doesn't have to */
 	if (!args->min_agbno && !args->max_agbno)
@@ -1551,54 +1602,16 @@  xfs_alloc_ag_vextent_near(
 	 * near the right edge of the tree.  If it's in the last btree leaf
 	 * block, then we just examine all the entries in that block
 	 * that are big enough, and pick the best one.
-	 * This is written as a while loop so we can break out of it,
-	 * but we never loop back to the top.
 	 */
-	while (xfs_btree_islastblock(acur.cnt, 0)) {
-#ifdef DEBUG
-		if (dofirst)
-			break;
-#endif
-		/*
-		 * Start from the entry that lookup found, sequence through
-		 * all larger free blocks.  If we're actually pointing at a
-		 * record smaller than maxlen, go to the start of this block,
-		 * and skip all those smaller than minlen.
-		 */
-		if (len || args->alignment > 1) {
-			acur.cnt->bc_ptrs[0] = 1;
-			do {
-				error = xfs_alloc_get_rec(acur.cnt, &bno, &len,
-						&i);
-				if (error)
-					goto out;
-				if (XFS_IS_CORRUPT(args->mp, i != 1))
-					goto out;
-				if (len >= args->minlen)
-					break;
-				error = xfs_btree_increment(acur.cnt, 0, &i);
-				if (error)
-					goto out;
-			} while (i);
-			ASSERT(len >= args->minlen);
-			if (!i)
-				break;
-		}
+	if (xfs_btree_islastblock(acur.cnt, 0)) {
+		bool		allocated = false;
 
-		error = xfs_alloc_walk_iter(args, &acur, acur.cnt, true, false,
-					    -1, &i);
+		error = xfs_alloc_ag_vextent_lastblock(args, &acur, &bno, &len,
+				&allocated);
 		if (error)
 			goto out;
-
-		/*
-		 * It didn't work.  We COULD be in a case where
-		 * there's a good record somewhere, so try again.
-		 */
-		if (acur.len == 0)
-			break;
-
-		trace_xfs_alloc_near_first(args);
-		goto alloc;
+		if (allocated)
+			goto alloc_finish;
 	}
 
 	/*
@@ -1624,7 +1637,7 @@  xfs_alloc_ag_vextent_near(
 		goto out;
 	}
 
-alloc:
+alloc_finish:
 	/* fix up btrees on a successful allocation */
 	error = xfs_alloc_cur_finish(args, &acur);