Re: [PATCH 8/7] xfs: delalloc -> unwritten COW fork allocation can go wrong

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 20, 2018 at 08:33:49AM -0800, Christoph Hellwig wrote:
> On Tue, Nov 20, 2018 at 08:45:38AM -0500, Brian Foster wrote:
> > >  		 * Filling in all of a previously delayed allocation extent.
> > > -		 * The right neighbor is contiguous, the left is not.
> > > +		 * The right neighbor is contiguous, the left is not. Take care
> > > +		 * with delay -> unwritten extent allocation here because the
> > > +		 * delalloc record we are overwriting is always written.
> > >  		 */
> > >  		PREV.br_startblock = new->br_startblock;
> > >  		PREV.br_blockcount += RIGHT.br_blockcount;
> > > +		PREV.br_state = new->br_state;
> > >  
> > >  		xfs_iext_next(ifp, &bma->icur);
> > >  		xfs_iext_remove(bma->ip, &bma->icur, state);
> > 
> > Fix looks sane to me, though I'm a little curious why this doesn't
> > follow the "contiguous extent extension" pattern that most of the other
> > contig cases seem to follow. For example, something like the following
> > for the right contig case:
> > 
> >                 old = RIGHT;
> >                 RIGHT.br_startoff = new->br_startoff;
> >                 RIGHT.br_startblock = new->br_startblock;
> >                 RIGHT.br_blockcount += new.br_blockcount;
> > 
> >                 xfs_iext_remove(bma->ip, &bma->icur, state); /* PREV */
> >                 xfs_iext_next(ifp, &bma->icur);
> >                 xfs_iext_update_extent(bma->ip, state, &bma->icur, &RIGHT);
> > 
> > ... and change the subsequent btree update to use old/RIGHT. Maybe
> > Christoph has thoughts on that.
> 
> Not sure the above looks much better.  If I had to do something
> different I'd apply something like this (untested) on top of Daves patch:
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 91b94c5c0cae..efe9e554fd0d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1698,14 +1698,12 @@ xfs_bmap_add_extent_delay_real(
>  		 * with delay -> unwritten extent allocation here because the
>  		 * delalloc record we are overwriting is always written.
>  		 */
> -		PREV.br_startblock = new->br_startblock;
> -		PREV.br_blockcount += RIGHT.br_blockcount;
> -		PREV.br_state = new->br_state;
> +		new->br_blockcount += RIGHT.br_blockcount;
>  
>  		xfs_iext_next(ifp, &bma->icur);
>  		xfs_iext_remove(bma->ip, &bma->icur, state);
>  		xfs_iext_prev(ifp, &bma->icur);
> -		xfs_iext_update_extent(bma->ip, state, &bma->icur, &PREV);
> +		xfs_iext_update_extent(bma->ip, state, &bma->icur, new);
>  
>  		if (bma->cur == NULL)
>  			rval = XFS_ILOG_DEXT;

And that's buggy because it doesn't update the BMBT record
correctly - it uses PREV to overwrite RIGHT, and so this will write
the delalloc record into the BMBT, not the new merged unwritten
extent...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com