xfs
[Top] [All Lists]

Re: [PATCH 2/4] xfs: refactor xfs_bmap_shift_extents() into multiple fun

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: refactor xfs_bmap_shift_extents() into multiple functions
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 9 Sep 2014 15:08:00 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1410092760-3451-3-git-send-email-bfoster@xxxxxxxxxx>
References: <1410092760-3451-1-git-send-email-bfoster@xxxxxxxxxx> <1410092760-3451-3-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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, &current_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

<Prev in Thread] Current Thread [Next in Thread>