On Fri, Oct 26, 2018 at 04:56:23AM -0700, Christoph Hellwig wrote:
> I don't think this is a new logic change, as we start at fbno
> already (both in the existing code and with your patch), but we got
> here because that block did not contain a suitable free space.
>
> That being said with the reverse search in the next patch the + 1
> is pointless as that code changes again. But many of the other changes
> in this patch or your next patch (which I hadn't looked at yet when
> I did this) should probably be in this one, otherwise we just create
> churn.
Or in other words, we could apply something like this (entirely
based on your next patch) here:
diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index 6a6572c5602e..919d6597fb19 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -1750,7 +1750,7 @@ xfs_dir2_node_find_freeblk(
struct xfs_inode *dp = args->dp;
struct xfs_trans *tp = args->trans;
struct xfs_buf *fbp = NULL;
- int findex;
+ int findex = 0;
xfs_dir2_db_t lastfbno;
xfs_dir2_db_t ifbno = -1;
xfs_dir2_db_t dbno = -1;
@@ -1778,7 +1778,7 @@ xfs_dir2_node_find_freeblk(
ASSERT(be16_to_cpu(bests[findex]) != NULLDATAOFF);
ASSERT(be16_to_cpu(bests[findex]) >= length);
dbno = freehdr.firstdb + findex;
- goto out;
+ goto found_block;
}
/*
@@ -1787,9 +1787,9 @@ xfs_dir2_node_find_freeblk(
*/
ifbno = fblk->blkno;
fbno = ifbno;
+ xfs_trans_brelse(tp, fbp);
+ fblk->bp = NULL;
}
- ASSERT(dbno == -1);
- findex = 0;
/*
* If we don't have a data block yet, we're going to scan the freespace
@@ -1810,48 +1810,42 @@ xfs_dir2_node_find_freeblk(
* data for a data block with enough free space in it.
*/
for ( ; fbno < lastfbno; fbno++) {
- /* If we don't have a freeblock in hand, get the next one. */
- if (fbp == NULL) {
- /* If it's ifbno we already looked at it. */
- if (fbno == ifbno)
- continue;
+ /* If it's ifbno we already looked at it. */
+ if (fbno == ifbno)
+ continue;
- /*
- * Read the block. There can be holes in the freespace
- * blocks, so this might not succeed. This should be
- * really rare, so there's no reason to avoid it.
- */
- error = xfs_dir2_free_try_read(tp, dp,
- xfs_dir2_db_to_da(args->geo, fbno),
- &fbp);
- if (error)
- return error;
- if (!fbp)
- continue;
+ /*
+ * Read the block. There can be holes in the freespace
+ * blocks, so this might not succeed. This should be
+ * really rare, so there's no reason to avoid it.
+ */
+ error = xfs_dir2_free_try_read(tp, dp,
+ xfs_dir2_db_to_da(args->geo, fbno),
+ &fbp);
+ if (error)
+ return error;
+ if (!fbp)
+ continue;
- findex = 0;
- free = fbp->b_addr;
- bests = dp->d_ops->free_bests_p(free);
- dp->d_ops->free_hdr_from_disk(&freehdr, free);
- }
+ findex = 0;
+ free = fbp->b_addr;
+ bests = dp->d_ops->free_bests_p(free);
+ dp->d_ops->free_hdr_from_disk(&freehdr, free);
/* Scan the free entry array for a large enough free space. */
- do {
+ for (findex = freehdr.nvalid - 1; findex >= 0; findex--) {
if (be16_to_cpu(bests[findex]) != NULLDATAOFF &&
be16_to_cpu(bests[findex]) >= length) {
dbno = freehdr.firstdb + findex;
- goto out;
+ goto found_block;
}
- } while (++findex < freehdr.nvalid);
+ }
/* Didn't find free space, go on to next free block */
xfs_trans_brelse(tp, fbp);
- fbp = NULL;
- if (fblk)
- fblk->bp = NULL;
}
-out:
+found_block:
*dbnop = dbno;
*fbpp = fbp;
*findexp = findex;
