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

Peter Zijlstra
2007-03-25T11:51:29Z

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?