Re: [PATCH] mm: Avoid livelocking of WB_SYNC_ALL writeback

[Jan Kara <jack@suse.cz>]

On Sat 06-11-10 12:12:02, Wu Fengguang wrote:
> On Sat, Nov 06, 2010 at 05:26:23AM +0800, Jan Kara wrote:
> 
> > +	/*
> > +	 * In WB_SYNC_ALL mode, we just want to ignore nr_to_write as
> > +	 * we need to write everything and livelock avoidance is implemented
> > +	 * differently.
> > +	 */
> > +	if (wbc.sync_mode == WB_SYNC_NONE)
> > +		write_chunk = MAX_WRITEBACK_PAGES;
> > +	else
> > +		write_chunk = LONG_MAX;
> 
> This looks like a safe change for .37.  I updated the patch on the
> above comment and made no other changes (it seems OK to also remove
> the below line, however that's not the necessary change as a bug fix,
> so I'd rather leave the extra change to the next merge window).
> write_cache_pages():
> 
> -->                     /*
> -->                      * We stop writing back only if we are not doing
> -->                      * integrity sync. In case of integrity sync we have to
> -->                      * keep going until we have written all the pages
> -->                      * we tagged for writeback prior to entering this loop.
> -->                      */
>                         if (--wbc->nr_to_write <= 0 &&
> ==>                         wbc->sync_mode == WB_SYNC_NONE) {
>                                 done = 1;
>                                 break;
  Well, I'd rather leave the test as is. In fact, in my mind-model the
target rather is to completely ignore nr_to_write when we do WB_SYNC_ALL
writeback since obeying it is never what a caller wants to happen...

> +	/*
> +	 * WB_SYNC_ALL mode does livelock avoidance by syncing dirty
> +	 * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX
> +	 * here avoids calling into writeback_inodes_wb() more than once.
  Maybe I'd add here:
The intended call sequence for WB_SYNC_ALL writeback is:
> +	 *
> +	 *      wb_writeback()
> +	 *          writeback_inodes_wb()       <== called only once
> +	 *              write_cache_pages()     <== called once for each inode
> +	 *                   (quickly) tag currently dirty pages
> +	 *                   (maybe slowly) sync all tagged pages
> +	 */
> +	if (wbc.sync_mode == WB_SYNC_NONE)
> +		write_chunk = MAX_WRITEBACK_PAGES;
> +	else
> +		write_chunk = LONG_MAX;
> +

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR