xfs
[Top] [All Lists]

Re: [PATCH 03/27] xfs: use write_cache_pages for writeback clustering

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 03/27] xfs: use write_cache_pages for writeback clustering
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 30 Jun 2011 02:57:34 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20110630020013.GX561@dastard>
References: <20110629140109.003209430@xxxxxxxxxxxxxxxxxxxxxx> <20110629140336.950805096@xxxxxxxxxxxxxxxxxxxxxx> <20110630020013.GX561@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jun 30, 2011 at 12:00:13PM +1000, Dave Chinner wrote:
> > -   if (iohead)
> > -           xfs_cancel_ioend(iohead);
> > -
> > -   if (err == -EAGAIN)
> > -           goto redirty;
> > -
> 
> Should this EAGAIN handling be dealt with in the removing-the-non-
> blocking-mode patch?

Probably.

> > +   ret = write_cache_pages(mapping, wbc, __xfs_vm_writepage, &ctx);
> > +
> > +   if (ctx.iohead) {
> > +           if (ret)
> > +                   xfs_cancel_ioend(ctx.iohead);
> > +           else
> > +                   xfs_submit_ioend(wbc, ctx.iohead);
> > +   }
> 
> I think this error handling does not work. If we have put pages into
> the ioend (i.e. successful ->writepage calls) and then have a
> ->writepage call fail, we'll get all the pages under writeback (i.e.
> those on the ioend) remain in that state, and not ever get written
> back (so move into the clean state) or redirtied (so written again
> later)
> 
> xfs_cancel_ioend() was only ever called for the first page sent down
> to ->writepage, and on error that page was redirtied separately.
> Hence it doesn't handle this case at all as it never occurs in the
> existing code.
> 
> I'd suggest that regardless of whether an error is returned here,
> the existence of ctx.iohead indicates a valid ioend that needs to be
> submitted....

Ok.  That would also solve the problem of the trylock failures.  I'll
see how we can deal with it nicely.

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