Re: sync-Regression in 2.6.28.2?

[Ralf Hildebrandt <Ralf.Hildebrandt@charite.de>]

* Federico Cuello <fedux@lugmen.org.ar>:
> Nick Piggin wrote:
> > [...]
> > Thanks, could you reply-to-all when replying to retain ccs please?
> >
> > Common theme is ext4, which uses no_nrwrite_index_update, and I introduced
> > a bug in there which could possibly cause ext4 to go into a loop...
> >
> > Would it be possible if you can test the following patch?
> >   
> 
> I'll test it as soon as I get home.
> 
> Meanwhile, I think the new patch may be slightly wrong. If I understand
> correctly PageWriteback(page) is called before nr_to_write is tested for
> being > 0 and then decremented if true, but "done" is  not set to 1
> until the next iteration. So another call to PageWriteback(page) while
> take place and then "done" will be set to true (if wbc->sync_mode ==
> WB_SYNC_NONE).
> 
> If nr_to_write == 1 at the beginning of the loop then two pages will be
> written.
> 
> I think the test condition should something like:
> 
> if (--nr_to_write <= 0 && wbc->sync_mode == WB_SYNC_NONE) {
>     done = 1;
>     break;
> }
> 
> .
> .
> 
> > Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > Acked-by: Nick Piggin <npiggin@suse.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  mm/page-writeback.c |   21 +++++++++++++++------
> >  1 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index b493db7..dc32dae 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -1051,13 +1051,22 @@ continue_unlock:
> >  				}
> >   			}
> >  
> > -			if (wbc->sync_mode == WB_SYNC_NONE) {
> > -				wbc->nr_to_write--;
> > -				if (wbc->nr_to_write <= 0) {
> > -					done = 1;
> > -					break;
> > -				}
> > +			if (nr_to_write > 0)
> > +				nr_to_write--;
> > +			else if (wbc->sync_mode == WB_SYNC_NONE) {
> > +				/*
> > +				 * We stop writing back only if we are not
> > +				 * doing integrity sync. In case of integrity
> > +				 * sync we have to keep going because someone
> > +				 * may be concurrently dirtying pages, and we
> > +				 * might have synced a lot of newly appeared
> > +				 * dirty pages, but have not synced all of the
> > +				 * old dirty pages.
> > +				 */
> > +				done = 1;
> > +				break;
> >  			}
> > +
> >  			if (wbc->nonblocking && bdi_write_congested(bdi)) {
> >  				wbc->encountered_congestion = 1;
> >  				done = 1;
> >   
> 
While I didn't test the patch, I can confirm that the problem is also
in 2.6.28.3

-- 
Ralf Hildebrandt                                Ralf.Hildebrandt@charite.de
Charite - Universitätsmedizin Berlin            Tel.  +49 (0)30-450 570-155
Geschäftsbereich IT | Abt. Netzwerk             Fax.  +49 (0)30-450 570-962
Hindenburgdamm 30 | 12200 Berlin