Re: [PATCH 2/3] xfs: truncate delalloc extents when IO fails in writeback

[Dave Chinner <david@fromorbit.com>]

On Thu, Mar 04, 2010 at 09:28:51AM -0500, Christoph Hellwig wrote:
> On Thu, Mar 04, 2010 at 12:46:24PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We currently use block_invalidatepage() to clean up pages where I/O
> > fails in ->writepage(). Unfortunately, if the page has delalloc
> > regions on it, we fail to remove the delalloc regions when we
> > invalidate the page.  This can result in tripping a BUG() in
> > xfs_get_blocks() later on if a direct IO read is done on that same
> > region - the delalloc extent is returned when none is supposed to be
> > there.
> > 
> > Fix this by truncating away the delalloc regions on the page before
> > invalidating it. Because they are delalloc, we can do this without
> > needing a transaction. Indeed - if we get ENOSPC errors, we have to
> > be able to do this truncation without a transaction as there is
> > no space left for block reservation (typically why we see a ENOSPC
> > in writeback).
> 
> The code looks good to me, but it could use a bit more documentation
> from the page description above inside the code.  

Good point. I'll add some comments to it...

> > +static void
> > +xfs_aops_discard_page(
> 
> This seems like a prime candidate for STATIC to make sure the
> over-eager compiler doesn't inline it into xfs_page_state_convert.

Will do.

> > -		cur->bc_private.b.firstblock = *firstblock;
> > +		cur->bc_private.b.firstblock = firstblock ?
> > +						*firstblock : NULLFSBLOCK;
> >  		cur->bc_private.b.flist = flist;
> >  		cur->bc_private.b.flags = 0;
> >  	} else
> > @@ -5503,7 +5504,8 @@ error0:
> >  		xfs_trans_log_inode(tp, ip, logflags);
> >  	if (cur) {
> >  		if (!error) {
> > -			*firstblock = cur->bc_private.b.firstblock;
> > +			if (firstblock)
> > +				*firstblock = cur->bc_private.b.firstblock;
> 
> I don't think it's a good idea to throw this change in as part of
> this patch.  I'd rather throw in the xfs_bmap_init and two
> useless arguments for now and clean up the calling conventions later.
> For which is more than enough opportunity, e.g. there's a totally
> unused delta argument in xfs_bunmapi.

I was undecided about which method to use. In the end I went with
the above because it was easier to check that correct behaviour was
occurring. i.e. if firstblock get used, then I'd see a panic. I'll
switch it to the variable method, and try to find the right
combination of ASSERT() calls to make sure it hasn't been used.

Thanks Christoph.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs