| Message ID | 20190509165839.44329-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs: rework extent allocation | expand |
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>
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>
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.
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
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. */
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(-)