Re: [patch 1/3] fix illogical behavior in balance_dirty_pages()

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Sun, 2007-03-25 at 13:34 +0200, Miklos Szeredi wrote:
> > > 
> > > Please have a look at this:
> > >   http://lkml.org/lkml/2007/3/19/220
> > 
> > 
> > 
> > > +			if (bdi_nr_reclaimable + bdi_stat(bdi, BDI_WRITEBACK) <=
> > > +			     	bdi_thresh)
> > > +				break;
> > > 
> > 
> > Yes, this will resolve the deadlock as well, where balance_dirty_pages()
> > is currently looping forever with:
> 
> Almost.
> 
> This
> 
> > -		if (nr_reclaimable) {
> > +		if (bdi_nr_reclaimable) {
> >  			writeback_inodes(&wbc);
> 
> still makes it loop forever if bdi_nr_reclaimable == 0, since the exit
> condition is not checked. 
> 
> Shouldn't it break out of the loop if bdi_stat(bdi, BDI_WRITEBACK) <=
> bdi_thresh in this case?

        for (;;) {
                struct writeback_control wbc = {
                        .bdi            = bdi,
                        .sync_mode      = WB_SYNC_NONE,
                        .older_than_this = NULL,
                        .nr_to_write    = write_chunk,
                        .range_cyclic   = 1,
                };

                get_dirty_limits(&background_thresh, &dirty_thresh,
                                &bdi_thresh, bdi);
                bdi_nr_reclaimable = bdi_stat(bdi, BDI_DIRTY) +
                                        bdi_stat(bdi, BDI_UNSTABLE);
(A)             if (bdi_nr_reclaimable + bdi_stat(bdi, BDI_WRITEBACK) <=
                        bdi_thresh)
                                break;

                /* Note: nr_reclaimable denotes nr_dirty + nr_unstable.
                 * Unstable writes are a feature of certain networked
                 * filesystems (i.e. NFS) in which data may have been
                 * written to the server's write cache, but has not yet
                 * been flushed to permanent storage.
                 */
(B)              if (bdi_nr_reclaimable) {
                        writeback_inodes(&wbc);

                        get_dirty_limits(&background_thresh, &dirty_thresh,
                                       &bdi_thresh, bdi);
                        bdi_nr_reclaimable = bdi_stat(bdi, BDI_DIRTY) +
                                                bdi_stat(bdi, BDI_UNSTABLE);
(C)                     if (bdi_nr_reclaimable + bdi_stat(bdi, BDI_WRITEBACK) <=
                                bdi_thresh)
                                break;

                        pages_written += write_chunk - wbc.nr_to_write;
                        if (pages_written >= write_chunk)
                                break;          /* We've done our duty */
                }
                congestion_wait(WRITE, HZ/10);
        }

I'm thinking that if bdi_nr_reclaimable == 0, A reduces to
bdi_stat(bdi, BDI_WRITEBACK) <= bdi_thresh and we're still out of the
loop, no?