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
|