On Wed, Jun 22, 2011 at 05:14:20AM +0800, Jan Kara wrote:
> On Tue 21-06-11 23:07:52, Wu Fengguang wrote:
> > On Tue, Jun 21, 2011 at 05:38:18AM +0800, Jan Kara wrote:
> > > On Sun 19-06-11 23:01:15, Wu Fengguang wrote:
> > > > When there are only one (or several) dirtiers, dirty_exceeded is always
> > > > (or mostly) off. Converting to timestamp avoids this problem. It helps
> > > > to use smaller write_chunk for smoother throttling.
> > > Hmm, what do you mean by "dirty_exceeded is always (or mostly) off"? I
> > > agree that dirty_exceeded handling is broken in current kernel when there
> > > are more dirtiers because of tasks having different dirty limits. Is this
> > > the problem you are speaking about?
> >
> > For the 1-dirty case, it will quit on either of the two conditions
> >
> > (1) no longer dirty exceeded, or
> > (2) write 1.5 times what it has dirtied
> >
> > Note that (2) implies (1) because there is only 1 dirtier: if it takes
> > 4MB pages to make it dirty exceeded, it will sure drop out of the dirty
> > exceeded state after cleaned 6MB.
> >
> > So the 1 dirtier will mostly see dirty_exceeded=0 inside
> > balance_dirty_pages_ratelimited_nr().
> >
> > However that looks not a big problem. Since there is only 1 dirtier,
> > it will at most get dirty exceeded by 4MB, which is fairly acceptable.
> Yes, 1 dirtier case works OK.
>
> > > If yes, I think I have a cleaner fix for that - setting dirty_exceeded
> > > when bdi_dirty enters the range where per-task dirty limits can be and
> > > resetting it only when we leave that range. I can refresh the fix and
> > > send it to you...
> >
> > Yes there is the per-task dynamic thresholds. However given that the
> > current scheme looks not too bad, maybe we can just do nothing for now
> > and skip this patch?
> Well, the current scheme has problems already when there are two
> dirtiers. Assume process P1 has threshold T1 and process P2 has threshold
> T2, T1 < T2. P1 first hits the threshold, sets dirty_exceeded and does some
> writeback. Meanwhile P2 still dirties pages, eventually it enters
> balance_dirty_pages() sees threshold T2 is not exceeded and clears
> dirty_exceeded even though threshold T1 still is exceeded. After dirtying
> another 4 MB, P1 enters balance_dirty_pages() again and sets dirty_exceeded
> again.
That's right. Good explanations!
> This way, dirty_exceeded is ping-ponged between 0 and 1 basically in a
> random way. If we are unlucky enough, we can get beyond dirty limit by 4 MB
> for each dirtier instead of 32 KB originally designed...
The worst case is approaching the most light dirtier and randomly
throttle it.
> I already have a fix for this. Do you want me to base it on top of -mm tree
> or your latest writeback series?
OK, please. Whatever base should be fine.
Thanks,
Fengguang
> > > > Before patch, the wait time in balance_dirty_pages() are ~200ms:
> > > >
> > > > [ 1093.397700] write_bandwidth: comm=swapper pages=1536 time=204ms
> > > > [ 1093.594319] write_bandwidth: comm=swapper pages=1536 time=196ms
> > > > [ 1093.796642] write_bandwidth: comm=swapper pages=1536 time=200ms
> > > >
> > > > After patch, ~25ms:
> > > >
> > > > [ 90.261339] write_bandwidth: comm=swapper pages=192 time=20ms
> > > > [ 90.293168] write_bandwidth: comm=swapper pages=192 time=24ms
> > > > [ 90.323853] write_bandwidth: comm=swapper pages=192 time=24ms
> > > > [ 90.354510] write_bandwidth: comm=swapper pages=192 time=28ms
> > > > [ 90.389890] write_bandwidth: comm=swapper pages=192 time=28ms
> > > > [ 90.421787] write_bandwidth: comm=swapper pages=192 time=24ms
> > > >
> > > > include/linux/backing-dev.h | 2 +-
> > > > mm/backing-dev.c | 2 --
> > > > mm/page-writeback.c | 24 ++++++++++++------------
> > > > 3 files changed, 13 insertions(+), 15 deletions(-)
> > > >
> > > > --- linux-next.orig/mm/page-writeback.c 2011-06-19 22:59:49.000000000 +0800
> > > > +++ linux-next/mm/page-writeback.c 2011-06-19 22:59:53.000000000 +0800
> > > > @@ -483,6 +483,15 @@ unsigned long bdi_dirty_limit(struct bac
> > > > return bdi_dirty;
> > > > }
> > > >
> > > > +/*
> > > > + * last time exceeded (limit - limit/DIRTY_BRAKE)
> > > > + */
> > > > +static bool dirty_exceeded_recently(struct backing_dev_info *bdi,
> > > > + unsigned long time_window)
> > > > +{
> > > > + return jiffies - bdi->dirty_exceed_time <= time_window;
> > > > +}
> > > > +
> > > > static void bdi_update_write_bandwidth(struct backing_dev_info *bdi,
> > > > unsigned long elapsed,
> > > > unsigned long written)
> > > > @@ -621,7 +630,6 @@ static void balance_dirty_pages(struct a
> > > > unsigned long bdi_thresh;
> > > > unsigned long pages_written = 0;
> > > > unsigned long pause = 1;
> > > > - bool dirty_exceeded = false;
> > > > struct backing_dev_info *bdi = mapping->backing_dev_info;
> > > > unsigned long start_time = jiffies;
> > > >
> > > > @@ -669,14 +677,9 @@ static void balance_dirty_pages(struct a
> > > > * bdi or process from holding back light ones; The latter is
> > > > * the last resort safeguard.
> > > > */
> > > > - dirty_exceeded = (bdi_dirty > bdi_thresh) ||
> > > > - (nr_dirty > dirty_thresh);
> > > > -
> > > > - if (!dirty_exceeded)
> > > > + if (bdi_dirty <= bdi_thresh && nr_dirty <= dirty_thresh)
> > > > break;
> > > > -
> > > > - if (!bdi->dirty_exceeded)
> > > > - bdi->dirty_exceeded = 1;
> > > > + bdi->dirty_exceed_time = jiffies;
> > > >
> > > > bdi_update_bandwidth(bdi, dirty_thresh, nr_dirty, bdi_dirty,
> > > > start_time);
> > > > @@ -719,9 +722,6 @@ static void balance_dirty_pages(struct a
> > > > pause = HZ / 10;
> > > > }
> > > >
> > > > - if (!dirty_exceeded && bdi->dirty_exceeded)
> > > > - bdi->dirty_exceeded = 0;
> > > > -
> > > > if (writeback_in_progress(bdi))
> > > > return;
> > > >
> > > > @@ -775,7 +775,7 @@ void balance_dirty_pages_ratelimited_nr(
> > > > return;
> > > >
> > > > ratelimit = ratelimit_pages;
> > > > - if (mapping->backing_dev_info->dirty_exceeded)
> > > > + if (dirty_exceeded_recently(bdi, MAX_PAUSE))
> > > > ratelimit = 8;
> > > >
> > > > /*
> > > > --- linux-next.orig/include/linux/backing-dev.h 2011-06-19 22:54:58.000000000 +0800
> > > > +++ linux-next/include/linux/backing-dev.h 2011-06-19 22:59:53.000000000 +0800
> > > > @@ -79,7 +79,7 @@ struct backing_dev_info {
> > > > unsigned long avg_write_bandwidth;
> > > >
> > > > struct prop_local_percpu completions;
> > > > - int dirty_exceeded;
> > > > + unsigned long dirty_exceed_time;
> > > >
> > > > unsigned int min_ratio;
> > > > unsigned int max_ratio, max_prop_frac;
> > > > --- linux-next.orig/mm/backing-dev.c 2011-06-19 22:59:49.000000000 +0800
> > > > +++ linux-next/mm/backing-dev.c 2011-06-19 22:59:53.000000000 +0800
> > > > @@ -670,8 +670,6 @@ int bdi_init(struct backing_dev_info *bd
> > > > goto err;
> > > > }
> > > >
> > > > - bdi->dirty_exceeded = 0;
> > > > -
> > > > bdi->bw_time_stamp = jiffies;
> > > > bdi->written_stamp = 0;
> > > >
> > > >
> > > >
> > > --
> > > Jan Kara <jack@xxxxxxx>
> > > SUSE Labs, CR
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
--
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
