Re: [RFC v3] writeback: add elastic bdi in cgwb bdp

[Hillf Danton <hdanton@sina.com>]

On Tue, 12 Nov 2019 17:02:36 -0800 Andrew Morton wrote:
> 
> On Tue, 12 Nov 2019 11:42:27 +0800 Hillf Danton <hdanton@sina.com> wrote:
> > 
> > The elastic bdi (ebdi) which is the mirror bdi of spinning disk,
> > SSD and USB key on market is introduced to balancing dirty pages
> > (bdp).
> > 
> > The risk arises that system runs out of free memory, when dirty
> > pages are produced too many too soon, so bdp is needed in field.
> > 
> > Ebdi facilitates bdp in elastic time intervals e.g. from a jiffy
> > to one HZ, depending on the time it would take to increase dirty
> > pages by the amount which is defined by the variable
> > ratelimit_pages.
> > 
> > During cgroup writeback (cgwb) bdp, ebdi helps observe the
> > changes both in cgwb's dirty pages (dirty speed) and in
> > written-out pages (laundry speed) in elastic time intervals,
> > until a balance is established between the two parties i.e.
> > the two speeds statistically equal.
> > 
> > The above mechanism of elastic equilibrium effectively prevents
> > dirty page hogs, as no chance is left for dirty pages to pile up,
> > thus cuts the risk that system free memory falls to unsafe level.
> > 
> > Thanks to Rong Chen for testing.
> 
> That sounds like a Tested-by:
> 
Yes, Sir, will add Tested-by: Rong Chen <rong.a.chen@intel.com>

> The changelog has no testing results.  Please prepare results which
> show, amongst other things, the change in performance when the kernel
> isn't tight on memory.  As well as the alteration in behaviour when
> memory is short.
> 
Will do.

> Generally, please work on making this code much more understandable?
> 
Will do.

> >
> > ...
> >
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -811,6 +811,8 @@ static long wb_split_bdi_pages(struct bd
> >  	if (nr_pages == LONG_MAX)
> >  		return LONG_MAX;
> >  
> > +	return nr_pages;
> > +
> >  	/*
> >  	 * This may be called on clean wb's and proportional distribution
> >  	 * may not make sense, just use the original @nr_pages in those
> > @@ -1604,6 +1606,7 @@ static long writeback_chunk_size(struct
> >  		pages = min(pages, work->nr_pages);
> >  		pages = round_down(pages + MIN_WRITEBACK_PAGES,
> >  				   MIN_WRITEBACK_PAGES);
> > +		pages = work->nr_pages;
> 
> It's unclear what this is doing, but it makes the three preceding
> statements non-operative.
> 
This change, and the above one as well, is trying to bypass the
current bandwidth, and a couple of rounds of cleanup are needed
after it survives the LTP.

> >  	}
> >  
> >  	return pages;
> > @@ -2092,6 +2095,9 @@ void wb_workfn(struct work_struct *work)
> >  		wb_wakeup_delayed(wb);
> >  
> >  	current->flags &= ~PF_SWAPWRITE;
> > +
> > +	if (waitqueue_active(&wb->bdp_waitq))
> > +		wake_up_all(&wb->bdp_waitq);
> 
> Please add a comment explaining why this is being done here.
> 
After writing out some dirty pages, it it a check point to see if
a balance is already set up between the dirty speed and laundry
speed. Those under throttling will be unthrottled after seeing
a balance in place.

A comment will be added.

> >  }
> >  
> >  /*
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -1830,6 +1830,67 @@ pause:
> >  		wb_start_background_writeback(wb);
> >  }
> >  
> > +/**
> > + * cgwb_bdp_should_throttle()	tell if a wb should be throttled
> > + * @wb bdi_writeback to throttle
> > + *
> > + * To avoid the risk of exhausting the system free memory, we check
> > + * and try much to prevent too many dirty pages from being produced
> > + * too soon.
> > + *
> > + * For cgroup writeback, it is essencially to keep an equilibrium
> 
> "it is essential"?
> 
Yes Sir.

> > + * between its dirty speed and laundry speed i.e. dirty pages are
> > + * written out as fast as they are produced in an ideal state.
> > + */
> > +static bool cgwb_bdp_should_throttle(struct bdi_writeback *wb)
> > +{
> > +	struct dirty_throttle_control gdtc = { GDTC_INIT_NO_WB };
> > +
> > +	if (fatal_signal_pending(current))
> > +		return false;
> > +
> > +	gdtc.avail = global_dirtyable_memory();
> > +
> > +	domain_dirty_limits(&gdtc);
> > +
> > +	gdtc.dirty = global_node_page_state(NR_FILE_DIRTY) +
> > +		     global_node_page_state(NR_UNSTABLE_NFS) +
> > +		     global_node_page_state(NR_WRITEBACK);
> > +
> > +	if (gdtc.dirty < gdtc.bg_thresh)
> > +		return false;
> > +
> > +	if (!writeback_in_progress(wb))
> > +		wb_start_background_writeback(wb);
> 
> This is a bit ugly.  Something called "bool cgwb_bdp_should_throttle()"
> shoiuld just check whether we should throttle.  But here it is, also
> initiating writeback.  That's an inappropriate thing for this function
> to do?
> 
It is the current bdp behavior trying to keep dirty pages below the
user-configurable background threshold by waking up flushers, because
no dirty page will be sent to disk without flusher's efforts, please
see 143dfe8611a6 ("writeback: IO-less balance_dirty_pages()").

Will try to find some chance to pinch it out.

> Also, we don't know *why* this is being done here, because there's no
> code comment explaining the reasoning to us.
> 
Will add a comment.

> 
> > +	if (gdtc.dirty < gdtc.thresh)
> > +		return false;
> > +
> > +	/*
> > +	 * throttle wb if there is the risk that wb's dirty speed is
> > +	 * running away from its laundry speed, better with statistic
> > +	 * error taken into account.
> > +	 */
> > +	return  wb_stat(wb, WB_DIRTIED) >
> > +		wb_stat(wb, WB_WRITTEN) + wb_stat_error();
> > +}
> > +
> >
> > ...
> >
> > @@ -1888,29 +1945,38 @@ void balance_dirty_pages_ratelimited(str
> >  	 * 1000+ tasks, all of them start dirtying pages at exactly the same
> >  	 * time, hence all honoured too large initial task->nr_dirtied_pause.
> >  	 */
> > -	p =  this_cpu_ptr(&bdp_ratelimits);
> > -	if (unlikely(current->nr_dirtied >= ratelimit))
> > -		*p = 0;
> > -	else if (unlikely(*p >= ratelimit_pages)) {
> > -		*p = 0;
> > -		ratelimit = 0;
> > -	}
> > +	dirty = this_cpu_ptr(&bdp_ratelimits);
> > +
> >  	/*
> >  	 * Pick up the dirtied pages by the exited tasks. This avoids lots of
> >  	 * short-lived tasks (eg. gcc invocations in a kernel build) escaping
> >  	 * the dirty throttling and livelock other long-run dirtiers.
> >  	 */
> > -	p = this_cpu_ptr(&dirty_throttle_leaks);
> > -	if (*p > 0 && current->nr_dirtied < ratelimit) {
> > -		unsigned long nr_pages_dirtied;
> > -		nr_pages_dirtied = min(*p, ratelimit - current->nr_dirtied);
> > -		*p -= nr_pages_dirtied;
> > -		current->nr_dirtied += nr_pages_dirtied;
> > +	leak = this_cpu_ptr(&dirty_throttle_leaks);
> > +
> > +	if (*dirty + *leak < ratelimit_pages) {
> > +		/*
> > +		 * nothing to do as it would take some more time to
> > +		 * eat out ratelimit_pages
> > +		 */
> > +		try_bdp = false;
> > +	} else {
> > +		try_bdp = true;
> > +
> > +		/*
> > +		 * bdp in flight helps detect dirty page hogs soon
> > +		 */
> 
> How?  Please expand on this comment a lot.  
> 
We should be cautious here in red zone after paying the ratelimit_pages
price; we might soon have to tackle a deluge of dirty page hogs.

Will cut it.

> > +		flights = this_cpu_ptr(&bdp_in_flight);
> > +
> > +		if ((*flights)++ & 1) {
> 
> What is that "& 1" doing?
> 
It helps to tell if a bdp is alredy in flight.

It would have been something like

		if (*flights == 0) {
			(*flights)++;
		} else {
			*flights = 0;
> > +			*dirty = *dirty + *leak - ratelimit_pages;
> > +			*leak = 0;
> > +		}

but I was curious to see the flights in long run.

Thanks
Hillf

> >  	}
> >  	preempt_enable();
> >  
> > -	if (unlikely(current->nr_dirtied >= ratelimit))
> > -		balance_dirty_pages(wb, current->nr_dirtied);
> > +	if (try_bdp)
> > +		cgwb_bdp(wb);
> >  
> >  	wb_put(wb);