xfs
[Top] [All Lists]

Re: [PATCH 2/9] xfs: remove if_lastex

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/9] xfs: remove if_lastex
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 20 May 2011 15:17:47 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110511150711.549194744@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110511150402.258164661@xxxxxxxxxxxxxxxxxxxxxx> <20110511150711.549194744@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-05-11 at 11:04 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-kill-if_lastex)
> The if_lastex field in struct xfs_ifork is only used as a temporary index
> during xfs_bmapi and xfs_bmapi.  Instead of using the inode fork to store

                      xfs_bunmapi

(I'll fix this for you when I commit the change.)

> it keep it local in the callchain.  Fortunately this is very easy as
> we already pass a stack copy of it down the whole chain which can simplify
> be changed to be passed by reference.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

This looks good to me.  I have a few comments below but
nothing looks incorrect.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

I will continue with patches 3-9 a little later on.

> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c        2011-05-10 13:20:21.182349200 +0200
> +++ xfs/fs/xfs/xfs_bmap.c     2011-05-10 13:29:40.618349159 +0200

. . .

> @@ -725,14 +709,13 @@ xfs_bmap_add_extent_delay_real(
>                * Filling in all of a previously delayed allocation extent.
>                * The left and right neighbors are both contiguous with new.
>                */

Couldn't you decrement *idx here, and use its value
(without subtracting 1) in almost all spots below...

> -             trace_xfs_bmap_pre_update(ip, idx - 1, state, _THIS_IP_);
> -             xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, idx - 1),
> +             trace_xfs_bmap_pre_update(ip, *idx - 1, state, _THIS_IP_);
> +             xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, *idx - 1),
>                       LEFT.br_blockcount + PREV.br_blockcount +
>                       RIGHT.br_blockcount);
> -             trace_xfs_bmap_post_update(ip, idx - 1, state, _THIS_IP_);
> +             trace_xfs_bmap_post_update(ip, *idx - 1, state, _THIS_IP_);
>  
> -             xfs_iext_remove(ip, idx, 2, state);
> -             ip->i_df.if_lastex = idx - 1;
> +             xfs_iext_remove(ip, *idx, 2, state);
...(but pass *idx + 1 here)...
>               ip->i_d.di_nextents--;
>               if (cur == NULL)
>                       rval = XFS_ILOG_CORE | XFS_ILOG_DEXT;
> @@ -756,6 +739,8 @@ xfs_bmap_add_extent_delay_real(
>                                       RIGHT.br_blockcount, LEFT.br_state)))
>                               goto done;
>               }
> +
> +             --*idx;

...rather than waiting until here to do the decrement?

>               *dnew = 0;
>               break;
>  
> @@ -764,13 +749,12 @@ xfs_bmap_add_extent_delay_real(
>                * Filling in all of a previously delayed allocation extent.
>                * The left neighbor is contiguous, the right is not.
>                */

Same general comment in this section (and in some others
that follow).

This is not a big deal at all, but I did notice that you
did the decrement (or increment) earlier in some spots
below, but not in these.  I see nothing incorrect,
just saw what seemed like inconsistency and wondered
if there was a reasaon.

> -             trace_xfs_bmap_pre_update(ip, idx - 1, state, _THIS_IP_);
> -             xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, idx - 1),
> +             trace_xfs_bmap_pre_update(ip, *idx - 1, state, _THIS_IP_);
> +             xfs_bmbt_set_blockcount(xfs_iext_get_ext(ifp, *idx - 1),
>                       LEFT.br_blockcount + PREV.br_blockcount);
> -             trace_xfs_bmap_post_update(ip, idx - 1, state, _THIS_IP_);
> +             trace_xfs_bmap_post_update(ip, *idx - 1, state, _THIS_IP_);
>  
> -             ip->i_df.if_lastex = idx - 1;
> -             xfs_iext_remove(ip, idx, 1, state);
> +             xfs_iext_remove(ip, *idx, 1, state);
>               if (cur == NULL)
>                       rval = XFS_ILOG_DEXT;
>               else {
. . .

> @@ -1468,17 +1458,19 @@ xfs_bmap_add_extent_unwritten_real(
>                * Setting the last part of a previous oldext extent to newext.
>                * The right neighbor is contiguous with the new allocation.
>                */
> -             trace_xfs_bmap_pre_update(ip, idx, state, _THIS_IP_);
> -             trace_xfs_bmap_pre_update(ip, idx + 1, state, _THIS_IP_);

This one was a bit weird (second trace call being out of place).

> +             trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
>               xfs_bmbt_set_blockcount(ep,
>                       PREV.br_blockcount - new->br_blockcount);
> -             trace_xfs_bmap_post_update(ip, idx, state, _THIS_IP_);
> -             xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, idx + 1),
> +             trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +
> +             ++*idx;
> +
> +             trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> +             xfs_bmbt_set_allf(xfs_iext_get_ext(ifp, *idx),
>                       new->br_startoff, new->br_startblock,
>                       new->br_blockcount + RIGHT.br_blockcount, newext);
> -             trace_xfs_bmap_post_update(ip, idx + 1, state, _THIS_IP_);
> +             trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
>  
> -             ip->i_df.if_lastex = idx + 1;
>               if (cur == NULL)
>                       rval = XFS_ILOG_DEXT;
>               else {

. . .



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