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

[Christoph Hellwig]

On Thu, Mar 04, 2010 at 12:46:24PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner at 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.  

> +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.

> -		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.