Re: [PATCH 1/3 v2] mm: Stop background writeback if there is other work queued for the thread

[Jan Kara <jack@suse.cz>]

On Thu 05-08-10 16:45:35, Andrew Morton wrote:
> On Thu,  5 Aug 2010 20:53:17 +0200
> Jan Kara <jack@suse.cz> wrote:
> > Background writeback and kupdate-style writeback
> 
> What the heck's the difference between "Background writeback" and
> "kupdate-style" writeback?  afacit "background" means "not due to
> kupdate, but due to a vmscan poke or something like that".  But the
> terms aren't defined anywhere and the wb_writeback_work fields are
> uncommented and the functions are undocumented and no wonder we keep
> making such a mess of this code.
  By "background" I mean the writeback we do when dirty_background_ratio is
exceeded. By "kupdate" I mean the writeback we do to write out inodes older
than dirty_expire_centisecs.

> > are easily livelockable (from
> > a definition of their target).
> 
> Please fully describe the livelock scenario(s).
> 
> > This is inconvenient because it can make sync(1)
> > stall forever waiting on its queued work to be finished.
> 
> And please fully describe the reason for the stall of sync(1).
  Ok, will do.

> Because if these things _are_ described then others are in a better
> position to review your proposed fix and they are in a better position
> to propose alternative fixes, no?
> 
> >  Generally, if someone
> > has a particular requirement for writeback he needs, it makes sense to give it
> > preference over a generic background dirty page cleaning. As soon as that work
> > is done, flusher thread will return back to background cleaning if it is
> > needed. So lets just interrupt background and kupdate writeback if there is
> > some other work to do to fix the livelocking problem.
> > 
> > CC: hch@infradead.org
> > Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/fs-writeback.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index d5be169..542471e 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -633,6 +633,14 @@ static long wb_writeback(struct bdi_writeback *wb,
> >  			break;
> >  
> >  		/*
> > +		 * Background writeout and kupdate-style writeback are
> > +		 * easily livelockable. Stop them if there is other work
> > +		 * to do so that e.g. sync can proceed.
> > +		 */
> > +		if ((work->for_background || work->for_kupdate) &&
> > +		    !list_empty(&wb->bdi->work_list))
> > +			break;
> > +		/*
> 
> So what happens if an application sits in a loop doing write&fsync to a
> file?  The vm's call for help gets ignored and your data doesn't get
> written back for three days??
  write & fsync wouldn't influece this because fsync() doesn't queue any
work for flusher thread (all the IO is done on behalf of the process doing
fsync()). If someone would be doing:
  while (1) sync();
Then this would make bdi-flusher thread ignore any VM's requests. But we
won't have much dirty data in this case anyway.
  The subtle thing here is that noone actually ever calls flusher thread to
do less work than it does when doing "kupdate" or "background" writeback as
defined above. But if we grow some calls to flusher thread for just a
limited amount of pages in future, then your are right it could be a
problem especially if flusher thread could be flooded with such requests.
I'll add above to the changelog as well.

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