On Sun, Sep 03, 2017 at 09:29:50AM +0200, Christoph Hellwig wrote:
> Use xfs_iext_get_extent to find, and xfs_iext_update_extent to update
> entries in the in-core extent list. This isolates the function from
> the detailed layout of the extent list, and generally makes the code
> a lot more readable.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 138 +++++++++++++++++++++++++----------------------
> 1 file changed, 74 insertions(+), 64 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 05d4778ce528..edea670a3f74 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
...
> @@ -1779,9 +1775,9 @@ xfs_bmap_add_extent_delay_real(
> * The right neighbor is contiguous, the left is not.
> */
> trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> - xfs_bmbt_set_startblock(ep, new->br_startblock);
> - xfs_bmbt_set_blockcount(ep,
> - PREV.br_blockcount + RIGHT.br_blockcount);
> + PREV.br_startblock = new->br_startblock;
> + PREV.br_blockcount += RIGHT.br_blockcount;
> + xfs_iext_update_extent(ifp, bma->idx, &PREV);
I think using PREV without setting the state here is a bit of a landmine
because the contiguity checking logic above only verifies that the new
extent state matches the associated neighbors. This is not really a
problem introduced by the patch, nor something that is likely to result
in a real problem due to context, but it would be nice to fix up since
the overall pattern of this function seems to be copied around a bit
more nowadays.
> trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
>
> xfs_iext_remove(bma->ip, bma->idx + 1, 1, state);
...
> @@ -1839,37 +1835,39 @@ xfs_bmap_add_extent_delay_real(
> * Filling in the first part of a previous delayed allocation.
> * The left neighbor is contiguous.
> */
> + old = LEFT;
> +
> trace_xfs_bmap_pre_update(bma->ip, bma->idx - 1, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, bma->idx - 1),
> - LEFT.br_blockcount + new->br_blockcount);
> - xfs_bmbt_set_startoff(ep,
> - PREV.br_startoff + new->br_blockcount);
> + LEFT.br_blockcount += new->br_blockcount;
> + xfs_iext_update_extent(ifp, bma->idx - 1, &LEFT);
> trace_xfs_bmap_post_update(bma->ip, bma->idx - 1, state, _THIS_IP_);
>
> - temp = PREV.br_blockcount - new->br_blockcount;
> - trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> if (bma->cur == NULL)
> rval = XFS_ILOG_DEXT;
> else {
> rval = 0;
> - error = xfs_bmbt_lookup_eq(bma->cur, LEFT.br_startoff,
> - LEFT.br_startblock, LEFT.br_blockcount,
> + error = xfs_bmbt_lookup_eq(bma->cur, old.br_startoff,
> + old.br_startblock, old.br_blockcount,
> &i);
> if (error)
> goto done;
> XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> error = xfs_bmbt_update(bma->cur, LEFT.br_startoff,
> - LEFT.br_startblock,
> - LEFT.br_blockcount +
> - new->br_blockcount,
> + LEFT.br_startblock, LEFT.br_blockcount,
> LEFT.br_state);
> if (error)
> goto done;
> }
> +
> + temp = PREV.br_blockcount - new->br_blockcount;
> da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp),
> startblockval(PREV.br_startblock));
> - xfs_bmbt_set_startblock(ep, nullstartblock(da_new));
> +
> + trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> + PREV.br_startoff += new->br_blockcount;
> + PREV.br_blockcount = temp;
> + PREV.br_startblock = nullstartblock(da_new);
> + xfs_iext_update_extent(ifp, bma->idx, &PREV);
It looks like this was shuffled around to do the updates within the
appropriate pre/post tracepoints. Any reason not to continue to keep the
in-core extent list update before the on-disk update as before, however?
(Same question for some of the hunks below as well).
> trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
>
> bma->idx--;
...
> @@ -1924,14 +1925,13 @@ xfs_bmap_add_extent_delay_real(
> * Filling in the last part of a previous delayed allocation.
> * The right neighbor is contiguous with the new allocation.
> */
> - temp = PREV.br_blockcount - new->br_blockcount;
> trace_xfs_bmap_pre_update(bma->ip, bma->idx + 1, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, bma->idx + 1),
> - new->br_startoff, new->br_startblock,
> - new->br_blockcount + RIGHT.br_blockcount,
> - RIGHT.br_state);
> + RIGHT.br_startoff = new->br_startoff;
> + RIGHT.br_startblock = new->br_startblock;
> + RIGHT.br_blockcount += new->br_blockcount;
> + xfs_iext_update_extent(ifp, bma->idx + 1, &RIGHT);
> trace_xfs_bmap_post_update(bma->ip, bma->idx + 1, state, _THIS_IP_);
> +
> if (bma->cur == NULL)
> rval = XFS_ILOG_DEXT;
> else {
> @@ -1943,18 +1943,20 @@ xfs_bmap_add_extent_delay_real(
> goto done;
> XFS_WANT_CORRUPTED_GOTO(mp, i == 1, done);
> error = xfs_bmbt_update(bma->cur, new->br_startoff,
> - new->br_startblock,
> - new->br_blockcount +
> - RIGHT.br_blockcount,
> - RIGHT.br_state);
> + new->br_startblock, new->br_blockcount,
> + new->br_state);
The immediately previous code (not shown) looks up RIGHT, which has
already been updated above to cover (new + RIGHT) and so does not exist
in the bmapbt. This code now attempts to update that extent to new,
which hasn't been updated to cover RIGHT.
> if (error)
> goto done;
> }
>
> + temp = PREV.br_blockcount - new->br_blockcount;
The use of temp seems unnecessary (perhaps not just here), particularly
since you could modify PREV.br_blockcount as this patch has been doing
elsewhere.
> da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(bma->ip, temp),
> startblockval(PREV.br_startblock));
> +
> trace_xfs_bmap_pre_update(bma->ip, bma->idx, state, _THIS_IP_);
> - xfs_bmbt_set_startblock(ep, nullstartblock(da_new));
> + PREV.br_blockcount = temp;
> + PREV.br_startblock = nullstartblock(da_new);
> + xfs_iext_update_extent(ifp, bma->idx, &PREV);
> trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
>
> bma->idx++;
...
> @@ -2078,12 +2084,16 @@ xfs_bmap_add_extent_delay_real(
> goto done;
> }
>
> - ep = xfs_iext_get_ext(ifp, bma->idx);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> + trace_xfs_bmap_pre_update(bma->ip, bma->idx, 0, _THIS_IP_);
> + PREV.br_startblock = nullstartblock(temp);
> + PREV.br_blockcount = temp; /* truncate PREV */
> + xfs_iext_update_extent(ifp, bma->idx, &PREV);
> trace_xfs_bmap_post_update(bma->ip, bma->idx, state, _THIS_IP_);
> +
> + /* update to the final reservation: */
> trace_xfs_bmap_pre_update(bma->ip, bma->idx + 2, state, _THIS_IP_);
> - xfs_bmbt_set_startblock(xfs_iext_get_ext(ifp, bma->idx + 2),
> - nullstartblock((int)temp2));
> + RIGHT.br_startblock = nullstartblock(temp2);
> + xfs_iext_update_extent(ifp, bma->idx + 2, &RIGHT);
I'm wondering if this last hunk can be removed. Has RIGHT.br_startblock
actually changed since set above?
Brian
> trace_xfs_bmap_post_update(bma->ip, bma->idx + 2, state, _THIS_IP_);
>
> bma->idx++;
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
