On Sun, Sep 07, 2014 at 08:25:58AM -0400, Brian Foster wrote:
> The extent shift mechanism in xfs_bmap_shift_extents() is complicated
> and handles several different, non-deterministic scenarios. These
> include extent shifts, extent merges and potential btree updates in
> either of the former scenarios.
>
> Refactor the code to be more linear and readable. The loop logic in
> xfs_bmap_shift_extents() and some initial error checking is adjusted
> slightly. The associated btree lookup and update/delete operations are
> condensed into single blocks of code. This reduces the number of
> btree-specific blocks and facilitates the separation of the merge
> operation into a new xfs_bmap_shift_extents_merge() helper. The merge
> check is also separated into an inline.
>
> This is a code refactor only. The behavior of extent shift and collapse
> range is not modified.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 243 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 168 insertions(+), 75 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 4b3f1b9..449a016 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5404,6 +5404,120 @@ error0:
> }
>
> /*
> + * Determine whether an extent shift can be accomplished by a merge with the
> + * extent that precedes the target hole of the shift.
> + */
> +static inline bool
> +xfs_bmap_shift_extents_can_merge(
No need for "inline" for static functions. The compiler will do that
as an optimisation if appropriate.
> + struct xfs_bmbt_irec *left, /* preceding extent */
> + struct xfs_bmbt_irec *got, /* current extent to shift */
> + xfs_fileoff_t shift) /* shift fsb */
> +{
> + xfs_fileoff_t startoff;
> +
> + startoff = got->br_startoff - shift;
> +
> + /*
> + * The extent, once shifted, must be adjacent in-file and on-disk with
> + * the preceding extent.
> + */
> + if ((left->br_startoff + left->br_blockcount != startoff) ||
> + (left->br_startblock + left->br_blockcount != got->br_startblock) ||
> + (left->br_state != got->br_state) ||
> + (left->br_blockcount + got->br_blockcount > MAXEXTLEN))
> + return false;
> +
> + return true;
> +}
> +
> +/*
> + * An extent shift adjusts the file offset of an extent to fill a preceding hole
> + * in the file. If an extent shift would result in the extent being fully
> + * adjacent to the extent that currently precedes the hole, we can merge with
> + * the preceding extent rather than do the shift.
> + *
> + * This function assumes the caller has verified a shift-by-merge is possible
> + * with the provided extents via xfs_bmap_shift_extents_can_merge().
> + */
> +static int
> +xfs_bmap_shift_extents_merge(
> + struct xfs_inode *ip,
> + int whichfork,
> + xfs_fileoff_t shift, /* shift fsb */
> + int current_ext, /* idx of gotp */
> + struct xfs_bmbt_rec_host *gotp, /* extent to shift */
> + struct xfs_bmbt_rec_host *leftp, /* preceding extent */
> + struct xfs_btree_cur *cur,
> + int *logflags) /* output */
> +{
> + struct xfs_ifork *ifp;
> + struct xfs_bmbt_irec got;
> + struct xfs_bmbt_irec left;
> + xfs_filblks_t blockcount;
> + int error, i;
> +
> + ifp = XFS_IFORK_PTR(ip, whichfork);
> + xfs_bmbt_get_all(gotp, &got);
> + xfs_bmbt_get_all(leftp, &left);
> + blockcount = left.br_blockcount + got.br_blockcount;
> +
> + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + ASSERT(xfs_bmap_shift_extents_can_merge(&left, &got, shift));
> +
> + /*
> + * Merge the in-core extents. Note that the host record pointers and
> + * current_ext index are invalid once the extent has been removed via
> + * xfs_iext_remove().
> + */
> + xfs_bmbt_set_blockcount(leftp, blockcount);
> + xfs_iext_remove(ip, current_ext, 1, 0);
> +
> + /*
> + * Update the on-disk extent count, the btree if necessary and log the
> + * inode.
> + */
> + XFS_IFORK_NEXT_SET(ip, whichfork,
> + XFS_IFORK_NEXTENTS(ip, whichfork) - 1);
> + *logflags |= XFS_ILOG_CORE;
> + if (cur) {
> + /* lookup and remove the extent to merge */
> + error = xfs_bmbt_lookup_eq(cur, got.br_startoff,
> + got.br_startblock, got.br_blockcount, &i);
> + if (error)
> + goto error;
Probably shouldn't give the jump label the same name as a variable
in the function. "out_error" is commonly used here.
> + XFS_WANT_CORRUPTED_GOTO(i == 1, error);
> +
> + error = xfs_btree_delete(cur, &i);
> + if (error)
> + goto error;
> + XFS_WANT_CORRUPTED_GOTO(i == 1, error);
> +
> + /* lookup and update size of the previous extent */
> + error = xfs_bmbt_lookup_eq(cur, left.br_startoff,
> + left.br_startblock, left.br_blockcount, &i);
> + if (error)
> + goto error;
> + XFS_WANT_CORRUPTED_GOTO(i == 1, error);
> +
> + left.br_blockcount = blockcount;
> +
> + error = xfs_bmbt_update(cur, left.br_startoff,
> + left.br_startblock, left.br_blockcount,
> + left.br_state);
> + if (error)
> + goto error;
> + } else {
> + *logflags |= XFS_ILOG_DEXT;
> + }
> +
> + return 0;
> +
> +error:
> + return error;
This code is much clearer, though I'd get rid of further
indents by changing the logic around like so:
....
*logflags |= XFS_ILOG_CORE;
if (!cur) {
*logflags |= XFS_ILOG_DEXT;
return 0;
}
/* lookup and remove the extent to merge */
error = xfs_bmbt_lookup_eq(cur, got.br_startoff,
got.br_startblock, got.br_blockcount, &i);
if (error)
goto out_error;
.....
error = xfs_bmbt_update(cur, left.br_startoff,
left.br_startblock, left.br_blockcount,
left.br_state);
out_error:
return error;
}
> @@ -5493,30 +5617,42 @@ xfs_bmap_shift_extents(
> */
> total_extents = ifp->if_bytes / sizeof(xfs_bmbt_rec_t);
> while (nexts++ < num_exts && current_ext < total_extents) {
> -
> - gotp = xfs_iext_get_ext(ifp, current_ext);
> - xfs_bmbt_get_all(gotp, &got);
> startoff = got.br_startoff - offset_shift_fsb;
>
> - /*
> - * Before shifting extent into hole, make sure that the hole is
> - * large enough to accommodate the shift.
> - */
> + /* grab the left extent and check for a potential merge */
> if (current_ext > 0) {
> - xfs_bmbt_get_all(xfs_iext_get_ext(ifp, current_ext - 1),
> - &left);
> - if (startoff < left.br_startoff + left.br_blockcount)
> + leftp = xfs_iext_get_ext(ifp, current_ext - 1);
> + xfs_bmbt_get_all(leftp, &left);
> +
> + /* make sure hole is large enough for shift */
> + if (startoff < left.br_startoff + left.br_blockcount) {
> error = -EINVAL;
> - } else if (offset_shift_fsb > got.br_startoff) {
> - /*
> - * When first extent is shifted, offset_shift_fsb should
> - * be less than the stating offset of the first extent.
> - */
> - error = -EINVAL;
> + goto del_cursor;
> + }
> +
> + if (xfs_bmap_shift_extents_can_merge(&left, &got,
> + offset_shift_fsb)) {
> + error = xfs_bmap_shift_extents_merge(ip, whichfork,
> + offset_shift_fsb, current_ext, gotp,
> + leftp, cur, &logflags);
> + if (error)
> + goto del_cursor;
> +
> + /*
> + * The extent was merged so adjust the extent
> + * index and move onto the next.
> + */
> + current_ext--;
> + goto next;
> + }
> }
> - if (error)
> - goto del_cursor;
>
> + /*
> + * We didn't merge the extent so do the shift. Update the start
> + * offset in the in-core extent and btree, if necessary.
> + */
> + xfs_bmbt_set_startoff(gotp, startoff);
> + logflags |= XFS_ILOG_CORE;
> if (cur) {
> error = xfs_bmbt_lookup_eq(cur, got.br_startoff,
> got.br_startblock,
This doesn't do a lot to improve the code. It increases the level of
indent and, IMO, makes it harder to read. It's a classic case of
function names getting so long there's no space left for the code...
So, how about s/xfs_bmap_shift_extents/xfs_bmse/ as a means of
reducing the namespace verbosity? And, really, that whole loop body
could be pushed into a separate function. i.e
while (nexts++ < num_exts) {
error = xfs_bmse_collapse_one(ip, ifp, cur, ¤t_ext, gotp,
offset_shift_fsb, &logflags);
if (error)
goto del_cursor;
/* check against total extent count, grab the next record */
total_extents = ifp->if_bytes / sizeof(xfs_bmbt_rec_t);
if (current_ext >= total_extents)
break;
gotp = xfs_iext_get_ext(ifp, current_ext);
xfs_bmbt_get_all(gotp, &got);
}
That reduces all the jumps/error cases simply to return statements,
allowing them to be ordered clearly to minimise the indent of
the code. It also means that way the value of current_ext changes is
obvious, rather than having the decrement/increment because of the
"next iteration" handling in the loop always incrementing the value.
Even the initial check outside the loop for:
if (current_ext == 0 && got.br_startoff < offset_shift_fsb) {
error = -EINVAL;
goto del_cursor
}
could be driven inside xfs_bmse_collapse_one() as the first check that
is done, and that would further simplify xfs_bmap_shift_extents().
i.e.
xfs_bmse_collapse_one()
{
if (*current_ext == 0) {
if (got.br_startoff < offset_shift_fsb)
return -EINVAL;
goto shift_extent;
}
/* get left, do merge checks */
....
if (!xfs_bmse_can_merge(&left, &got, offset_shift_fsb))
goto shift_extent;
return xfs_bmse_merge(ip, whichfork, ...)
shift_extent:
(*current_ext)++;
xfs_bmbt_set_startoff(gotp, startoff);
logflags |= XFS_ILOG_CORE;
if (!cur)
*logflags |= XFS_ILOG_DEXT;
return 0;
}
/* do btree shift */
....
return error;
}
This way we end up with a set of well defined operations, along
with clear logic on how they are iterated to make do the overall
shift operation.
What do you think?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
