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.
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...
I already have a fix for this. Do you want me to base it on top of -mm tree
or your latest writeback series?
Honza
> > > 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
