Re: [PATCH] fix up delta calculation for xfs_bmap_add_extent_unwritten_real

[Lachlan McIlroy <lachlan@sgi.com>]

Looks good to me.

I can't understand why this code is inconsistent with the other cases
- no other cases adjusts PREV or the cursor (other than the state).
To be consistent it would update PREV to become the new LHS by passing
'new->br_startoff - PREV.br_startoff' into xfs_bmbt_update() and then
inserting r[0] and r[1] although that way would require an extra call
to xfs_bmbt_increment().

tes@sgi.com wrote:
> pv#984030
> Patch provided by olaf@sgi.com and has been reviewed
> but putting out there for others to see the change.
> 
> A bug was found in xfs_bmap_add_extent_unwritten_real() which will
> not affect xfs in its normal use.
> In a particular case, the delta param which is supposed to describe
> the region where extents have changed was not updated appropriately.
> 
> The case of interest is:
>    1707         case 0:
>    1708                 /*
>    1709                  * Setting the middle part of a previous oldext extent to
>    1710                  * newext.  Contiguity is impossible here.
>    1711                  * One extent becomes three extents.
>    1712                  */
> 
> where we are not left-filling (on LHS) or right-filling (on RHS) and so
> are also not contiguous with neighbours. But we are in the middle.
> 
> PREV:------------------------------------------------------------>|
> -----------------------|--------------------|----------------------
> |  cur->bc_rec.b  LHS  |    r[0] = new      |    r[1] RHS         |
> -----------------------|--------------------|----------------------
> 
> So our new extent is in the middle and r[1] is on the RHS.
> The LHS, cur, is set to PREV after making PREV's count smaller
> to its new size. However, this means that our original PREV's count
> is no longer covering the whole range.
> So Olaf's change leave's PREV alone and just updates cur->bc_rec.b to have
> PREV's contents and decrements its new count.
> Mainly, because below we calculate the delta based on the PREV values
> (in particular expecting the original block count).
> We are not using PREV anywhere else after that AFAICS so I think the
> change is safe from causing a regression elsewhere.
> 
> --Tim
> 
>  xfs_bmap.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> Index: 2.6.x-xfs-quilt/fs/xfs/xfs_bmap.c
> ===================================================================
> --- 2.6.x-xfs-quilt.orig/fs/xfs/xfs_bmap.c	2008-07-04 15:34:38.000000000 +1000
> +++ 2.6.x-xfs-quilt/fs/xfs/xfs_bmap.c	2008-07-09 16:24:17.250532483 +1000
> @@ -1740,9 +1740,9 @@ xfs_bmap_add_extent_unwritten_real(
>  				r[1].br_state)))
>  				goto done;
>  			/* new left extent - oldext */
> -			PREV.br_blockcount =
> -				new->br_startoff - PREV.br_startoff;
>  			cur->bc_rec.b = PREV;
> +			cur->bc_rec.b.br_blockcount =
> +				new->br_startoff - PREV.br_startoff;
>  			if ((error = xfs_bmbt_insert(cur, &i)))
>  				goto done;
>  			XFS_WANT_CORRUPTED_GOTO(i == 1, done);
> 
> 
>