Re: [patch 1/7] mm: write_cache_pages writepage error fix

[Nick Piggin <npiggin@suse.de>]

On Wed, Oct 22, 2008 at 09:37:15AM -0700, Andrew Morton wrote:
> On Wed, 22 Oct 2008 11:36:32 +0200 Nick Piggin <npiggin@suse.de> wrote:
> 
> > > >  				wbc->encountered_congestion = 1;
> > > > 
> > > > -- 
> > > 
> > > I don't think I like the implementation much.
> > > 
> > > In all cases in that function where we set done=1, we want to bale out
> > > right now at this page, rather than processing the remaining pages in
> > > the pagevec.
> > > 
> > > So it would be better to implement a bit of code which releases the
> > > pagevec pages and then breaks out of the loop.  Then this bug
> > > automatically gets fixed.
> > 
> > I had almost exactly that at some point, then I rethought it because it
> > was a bigger behaviour change than the bugfix-only.
> > 
> > I thought the !done thing must be just for batching and to process the
> > pages already looked up in the pagevec.
> > 
> > I'll need to backport at least some of these to stable -- any chance of
> > making the change you suggest toward the end of the patchset?
> 
> That works.
> 
> > Let me
> > know if you want a patch or will do one yourself.
> 
> I couldn't hope to match your level of testing ;)

OK, looking at this closer, I think the aversion to breaking out of the
loop early is because it becomes more difficult to keep track of you
end index, for cyclic writeback. It's no longer the index that comes out
of the radix tree lookup.

And whether we go for the current page index or the previous one depends
on the reason we have to bail out.

So I don't know if it will fall out quite so nicely as you had it. But
I missed this too: I didn't handle it properly either in the early bailout
from writepage error case. So I'll have to do that.

The problem that would cause in normal writeout operations is that you'd
bail out when nr_to_write reaches 0, but you would most likely set the
writeback_index to skip a few dirty pages that were in the pagevec but
not yet written out. For error-cases it's not so bad as it will be an
abnormal condition, but AFAIKS nowhere is documented that range_cyclic
cannot be used for data integrity, so it is better to be consistent and
make it all work as expected.

Anyway, I just noticed this as I was about to merge your changes and
resend, so I'll need a little longer to fix and test...