[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/3] xfs: truncate delalloc extents when IO fails in writeback
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 4 Mar 2010 09:28:51 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1267667185-7736-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1267667185-7736-1-git-send-email-david@xxxxxxxxxxxxx> <1267667185-7736-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Thu, Mar 04, 2010 at 12:46:24PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 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.

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