Re: [PATCH 02/11] writeback: switch to per-bdi threads for flushing data

[Jens Axboe <jens.axboe@oracle.com>]

On Tue, May 19 2009, Richard Kennedy wrote:
> Jens Axboe wrote:
> > This gets rid of pdflush for bdi writeout and kupdated style cleaning.
> > <snip>
> > index 2296ff4..76269f8 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -541,7 +530,7 @@ static void balance_dirty_pages(struct address_space *mapping)
> >  		 * been flushed to permanent storage.
> >  		 */
> >  		if (bdi_nr_reclaimable) {
> > -			writeback_inodes(&wbc);
> > +			generic_sync_bdi_inodes(NULL, &wbc);
> >  			pages_written += write_chunk - wbc.nr_to_write;
> >  			get_dirty_limits(&background_thresh, &dirty_thresh,
> >  				       &bdi_thresh, bdi);
> > @@ -592,7 +581,7 @@ static void balance_dirty_pages(struct address_space *mapping)
> >  			(!laptop_mode && (global_page_state(NR_FILE_DIRTY)
> >  					  + global_page_state(NR_UNSTABLE_NFS)
> >  					  > background_thresh)))
> > -		pdflush_operation(background_writeout, 0);
> > +		bdi_start_writeback(bdi, NULL, 0);
> >  }
> >  
> Hi Jens,
> 
> I'm interested in this slight change of behaviour, when over the
> background dirty limit background_writeout will write any dirty pages
> while bdi_start_writeout writes only pages for the current bdi. Are
> there any benefits in making this change?
> 
> Thinking about the case of 2 apps writing to different bdis. When app A
> stops writing, then next time app B goes over the background dirty
> threshold it will only be able to write its own pages, leaving any from
> app A dirty until they reach their age limit.

The function in question balances dirty pages against a specific address
space, which has a specific mapping. The async part of the background
writeout could be global as you mention. The whole thing is a bit weird
in balance_dirty_pages(), for instance it checks for writeout against a
given queue then proceeds to do a global writeout if not busy. At least
it's consistent now.

> So we may be keeping dirty pages for the app that's finished longer than
> necessary. Keeping pages for a finished app while flushing pages from a
> running app seems a bit strange. I guess this is an odd corner case and
> may not be worth worrying about, but I'd be interested to hear what you
> think.

The kupdated() initiated background writeout will take care of that, if
nobody does a sync on that data first. If nobody is dirtying new data on
the given bdi, then it seems perfectly fine to let normal background
writeout handle it.

> Do you think your new code will require any changes to the per bdi dirty
> limits? It may be informative & interesting to run some tests writing to
> fast & slow devices at the same time.

Generally the code should behave fairly closely to the existing pdflush
based code, so I don't think bdi dirty limit tweaking will be necessary.
I'd definitely welcome some testing though, particularly slow vs fast as
you mention. I've mainly been doing benchmarking to make sure we don't
regress on performance, and that has been for fairly similar hardware.
Since testing does take a lot of time, it would be nice if someone else
would gather their own experiences, especially in areas that have been
problematic in the past (slow vs fast devices, for instance!).

-- 
Jens Axboe