| Message ID | 20231004001943.349265-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: push perags further into allocation routines | expand |
On Wed, Oct 04, 2023 at 11:19:35AM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > This function is really implementing two policies. The first is > trying to create contiguous extents at file extension. Failing that, > it attempts to perform aligned allocation. These are really two > separate policies, and it is further complicated by filestream > allocation having different aligned allocation constraints than > the normal bmap allocation. > > Split xfs_bmap_btalloc_at_eof() into two parts so we can start to > align the two different allocator policies more closely. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 147 +++++++++++++++++++++------------------ > 1 file changed, 80 insertions(+), 67 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 30c931b38853..e14671414afb 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -3451,81 +3451,81 @@ xfs_bmap_exact_minlen_extent_alloc( > > #endif > > -/* > - * If we are not low on available data blocks and we are allocating at > - * EOF, optimise allocation for contiguous file extension and/or stripe > - * alignment of the new extent. > - * > - * NOTE: ap->aeof is only set if the allocation length is >= the > - * stripe unit and the allocation offset is at the end of file. > - */ > + /* > + * Attempt contiguous allocation for file extension. > + */ > + static int > + xfs_bmap_btalloc_at_eof( > + struct xfs_bmalloca *ap, > + struct xfs_alloc_arg *args, > + xfs_extlen_t blen, > + int stripe_align) > +{ > + struct xfs_mount *mp = args->mp; > + struct xfs_perag *caller_pag = args->pag; > + xfs_extlen_t nextminlen = 0; > + int error; > + > + /* > + * Compute the minlen+alignment for the next case. Set slop so > + * that the value of minlen+alignment+slop doesn't go up between > + * the calls. > + */ > + args->alignment = 1; > + if (blen > stripe_align && blen <= args->maxlen) > + nextminlen = blen - stripe_align; > + else > + nextminlen = args->minlen; > + if (nextminlen + stripe_align > args->minlen + 1) > + args->minalignslop = nextminlen + stripe_align - > + args->minlen - 1; > + else > + args->minalignslop = 0; > + > + if (!caller_pag) > + args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno)); > + error = xfs_alloc_vextent_exact_bno(args, ap->blkno); > + if (!caller_pag) { > + xfs_perag_put(args->pag); > + args->pag = NULL; Splitting these two if cases into two less ambitious functions makes it easier for me to regrok what this code was doing. Thank you, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + } > + if (error) > + return error; > + > + if (args->fsbno != NULLFSBLOCK) > + return 0; > + /* > + * Exact allocation failed. Reset to try an aligned allocation > + * according to the original allocation specification. > + */ > + args->minlen = nextminlen; > + return 0; > +} > + > static int > -xfs_bmap_btalloc_at_eof( > +xfs_bmap_btalloc_aligned( > struct xfs_bmalloca *ap, > struct xfs_alloc_arg *args, > xfs_extlen_t blen, > int stripe_align, > bool ag_only) > { > - struct xfs_mount *mp = args->mp; > - struct xfs_perag *caller_pag = args->pag; > + struct xfs_perag *caller_pag = args->pag; > int error; > > /* > - * If there are already extents in the file, try an exact EOF block > - * allocation to extend the file as a contiguous extent. If that fails, > - * or it's the first allocation in a file, just try for a stripe aligned > - * allocation. > + * If we failed an exact EOF allocation already, stripe > + * alignment will have already been taken into account in > + * args->minlen. Hence we only adjust minlen to try to preserve > + * alignment if no slop has been reserved for alignment > */ > - if (ap->offset) { > - xfs_extlen_t nextminlen = 0; > - > - /* > - * Compute the minlen+alignment for the next case. Set slop so > - * that the value of minlen+alignment+slop doesn't go up between > - * the calls. > - */ > - args->alignment = 1; > - if (blen > stripe_align && blen <= args->maxlen) > - nextminlen = blen - stripe_align; > - else > - nextminlen = args->minlen; > - if (nextminlen + stripe_align > args->minlen + 1) > - args->minalignslop = nextminlen + stripe_align - > - args->minlen - 1; > - else > - args->minalignslop = 0; > - > - if (!caller_pag) > - args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno)); > - error = xfs_alloc_vextent_exact_bno(args, ap->blkno); > - if (!caller_pag) { > - xfs_perag_put(args->pag); > - args->pag = NULL; > - } > - if (error) > - return error; > - > - if (args->fsbno != NULLFSBLOCK) > - return 0; > - /* > - * Exact allocation failed. Reset to try an aligned allocation > - * according to the original allocation specification. > - */ > - args->alignment = stripe_align; > - args->minlen = nextminlen; > - args->minalignslop = 0; > - } else { > - /* > - * Adjust minlen to try and preserve alignment if we > - * can't guarantee an aligned maxlen extent. > - */ > - args->alignment = stripe_align; > - if (blen > args->alignment && > - blen <= args->maxlen + args->alignment) > - args->minlen = blen - args->alignment; > - args->minalignslop = 0; > + if (args->minalignslop == 0) { > + if (blen > stripe_align && > + blen <= args->maxlen + stripe_align) > + args->minlen = blen - stripe_align; > } > + args->alignment = stripe_align; > + args->minalignslop = 0; > > if (ag_only) { > error = xfs_alloc_vextent_near_bno(args, ap->blkno); > @@ -3612,8 +3612,14 @@ xfs_bmap_btalloc_filestreams( > } > > args->minlen = xfs_bmap_select_minlen(ap, args, blen); > + if (ap->aeof && ap->offset) > + error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align); > + > + if (error || args->fsbno != NULLFSBLOCK) > + goto out_low_space; > + > if (ap->aeof) > - error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align, > + error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align, > true); > > if (!error && args->fsbno == NULLFSBLOCK) > @@ -3662,13 +3668,20 @@ xfs_bmap_btalloc_best_length( > * optimal or even aligned allocations in this case, so don't waste time > * trying. > */ > - if (ap->aeof && !(ap->tp->t_flags & XFS_TRANS_LOWMODE)) { > - error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align, > - false); > + if (ap->aeof && ap->offset && !(ap->tp->t_flags & XFS_TRANS_LOWMODE)) { > + error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align); > if (error || args->fsbno != NULLFSBLOCK) > return error; > } > > + /* attempt aligned allocation for new EOF extents */ > + if (ap->aeof) > + error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align, > + false); > + if (error || args->fsbno != NULLFSBLOCK) > + return error; > + > + /* attempt unaligned allocation */ > error = xfs_alloc_vextent_start_ag(args, ap->blkno); > if (error || args->fsbno != NULLFSBLOCK) > return error; > -- > 2.40.1 >
The split looks good, and much easier to understand than before. I have a minor nitpick on the callsites below: > @@ -3612,8 +3612,14 @@ xfs_bmap_btalloc_filestreams( > } > > args->minlen = xfs_bmap_select_minlen(ap, args, blen); > + if (ap->aeof && ap->offset) > + error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align); > + > + if (error || args->fsbno != NULLFSBLOCK) > + goto out_low_space; > + > if (ap->aeof) > - error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align, > + error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align, > true); > > if (!error && args->fsbno == NULLFSBLOCK) I find the way how this is structured not very helpful to the read, although most of the blame lies with the pre-existing code. If we'd check the error where it happens I think it would be way easier to read: args->minlen = xfs_bmap_select_minlen(ap, args, blen); if (ap->aeof) { if (ap->offset) { error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align); if (error || args->fsbno != NULLFSBLOCK) goto out_low_space; } error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align, true); if (error || args->fsbno != NULLFSBLOCK) goto out_low_space; } error = xfs_alloc_vextent_near_bno(args, ap->blkno); The same applies to xfs_bmap_btalloc_best_length.
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 30c931b38853..e14671414afb 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3451,81 +3451,81 @@ xfs_bmap_exact_minlen_extent_alloc( #endif -/* - * If we are not low on available data blocks and we are allocating at - * EOF, optimise allocation for contiguous file extension and/or stripe - * alignment of the new extent. - * - * NOTE: ap->aeof is only set if the allocation length is >= the - * stripe unit and the allocation offset is at the end of file. - */ + /* + * Attempt contiguous allocation for file extension. + */ + static int + xfs_bmap_btalloc_at_eof( + struct xfs_bmalloca *ap, + struct xfs_alloc_arg *args, + xfs_extlen_t blen, + int stripe_align) +{ + struct xfs_mount *mp = args->mp; + struct xfs_perag *caller_pag = args->pag; + xfs_extlen_t nextminlen = 0; + int error; + + /* + * Compute the minlen+alignment for the next case. Set slop so + * that the value of minlen+alignment+slop doesn't go up between + * the calls. + */ + args->alignment = 1; + if (blen > stripe_align && blen <= args->maxlen) + nextminlen = blen - stripe_align; + else + nextminlen = args->minlen; + if (nextminlen + stripe_align > args->minlen + 1) + args->minalignslop = nextminlen + stripe_align - + args->minlen - 1; + else + args->minalignslop = 0; + + if (!caller_pag) + args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno)); + error = xfs_alloc_vextent_exact_bno(args, ap->blkno); + if (!caller_pag) { + xfs_perag_put(args->pag); + args->pag = NULL; + } + if (error) + return error; + + if (args->fsbno != NULLFSBLOCK) + return 0; + /* + * Exact allocation failed. Reset to try an aligned allocation + * according to the original allocation specification. + */ + args->minlen = nextminlen; + return 0; +} + static int -xfs_bmap_btalloc_at_eof( +xfs_bmap_btalloc_aligned( struct xfs_bmalloca *ap, struct xfs_alloc_arg *args, xfs_extlen_t blen, int stripe_align, bool ag_only) { - struct xfs_mount *mp = args->mp; - struct xfs_perag *caller_pag = args->pag; + struct xfs_perag *caller_pag = args->pag; int error; /* - * If there are already extents in the file, try an exact EOF block - * allocation to extend the file as a contiguous extent. If that fails, - * or it's the first allocation in a file, just try for a stripe aligned - * allocation. + * If we failed an exact EOF allocation already, stripe + * alignment will have already been taken into account in + * args->minlen. Hence we only adjust minlen to try to preserve + * alignment if no slop has been reserved for alignment */ - if (ap->offset) { - xfs_extlen_t nextminlen = 0; - - /* - * Compute the minlen+alignment for the next case. Set slop so - * that the value of minlen+alignment+slop doesn't go up between - * the calls. - */ - args->alignment = 1; - if (blen > stripe_align && blen <= args->maxlen) - nextminlen = blen - stripe_align; - else - nextminlen = args->minlen; - if (nextminlen + stripe_align > args->minlen + 1) - args->minalignslop = nextminlen + stripe_align - - args->minlen - 1; - else - args->minalignslop = 0; - - if (!caller_pag) - args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno)); - error = xfs_alloc_vextent_exact_bno(args, ap->blkno); - if (!caller_pag) { - xfs_perag_put(args->pag); - args->pag = NULL; - } - if (error) - return error; - - if (args->fsbno != NULLFSBLOCK) - return 0; - /* - * Exact allocation failed. Reset to try an aligned allocation - * according to the original allocation specification. - */ - args->alignment = stripe_align; - args->minlen = nextminlen; - args->minalignslop = 0; - } else { - /* - * Adjust minlen to try and preserve alignment if we - * can't guarantee an aligned maxlen extent. - */ - args->alignment = stripe_align; - if (blen > args->alignment && - blen <= args->maxlen + args->alignment) - args->minlen = blen - args->alignment; - args->minalignslop = 0; + if (args->minalignslop == 0) { + if (blen > stripe_align && + blen <= args->maxlen + stripe_align) + args->minlen = blen - stripe_align; } + args->alignment = stripe_align; + args->minalignslop = 0; if (ag_only) { error = xfs_alloc_vextent_near_bno(args, ap->blkno); @@ -3612,8 +3612,14 @@ xfs_bmap_btalloc_filestreams( } args->minlen = xfs_bmap_select_minlen(ap, args, blen); + if (ap->aeof && ap->offset) + error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align); + + if (error || args->fsbno != NULLFSBLOCK) + goto out_low_space; + if (ap->aeof) - error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align, + error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align, true); if (!error && args->fsbno == NULLFSBLOCK) @@ -3662,13 +3668,20 @@ xfs_bmap_btalloc_best_length( * optimal or even aligned allocations in this case, so don't waste time * trying. */ - if (ap->aeof && !(ap->tp->t_flags & XFS_TRANS_LOWMODE)) { - error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align, - false); + if (ap->aeof && ap->offset && !(ap->tp->t_flags & XFS_TRANS_LOWMODE)) { + error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align); if (error || args->fsbno != NULLFSBLOCK) return error; } + /* attempt aligned allocation for new EOF extents */ + if (ap->aeof) + error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align, + false); + if (error || args->fsbno != NULLFSBLOCK) + return error; + + /* attempt unaligned allocation */ error = xfs_alloc_vextent_start_ag(args, ap->blkno); if (error || args->fsbno != NULLFSBLOCK) return error;