xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/3] xfs: truncate delalloc extents when IO fails in writeback
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 5 Mar 2010 09:03:09 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100304142851.GA19157@xxxxxxxxxxxxx>
References: <1267667185-7736-1-git-send-email-david@xxxxxxxxxxxxx> <1267667185-7736-3-git-send-email-david@xxxxxxxxxxxxx> <20100304142851.GA19157@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
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@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.  

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@xxxxxxxxxxxxx

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