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@xxxxxxx> 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... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
