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

[Wu Fengguang <fengguang.wu@intel.com>]

On Sun, Aug 08, 2010 at 09:59:18PM +0800, Jan Kara wrote:
> On Sun 08-08-10 07:07:26, Jens Axboe wrote:
> > On 08/08/2010 03:29 AM, Wu Fengguang wrote:
> > > On Sun, Aug 08, 2010 at 12:12:30PM +0800, Dave Chinner wrote:
> > >> On Thu, Aug 05, 2010 at 04:45:35PM -0700, Andrew Morton wrote:
> > >>> On Thu,  5 Aug 2010 20:53:17 +0200
> > >>> Jan Kara <jack@suse.cz> wrote:
> > >>>> ---
> > >>>>  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??
> > >>
> > >> To avoid the possibility of any such occurrence, perhaps requeuing
> > >> the work rather than cancelling it would be better? i.e. stop, put
> > >> it behind whatever work just came in and so when the new work
> > >> completes, we restart the background/expiry based writeback?
> > > 
> > > That would be better. Ie. add a flag BDI_background_writeback to
> > > indicate the background work should be started after finishing with
> > > the queued works. bdi_start_background_writeback() can be modified to
> > > simply set the bit and wake up the flusher thread.
> > 
> > I think that is better, we ideally want background writeout to
> > interleave smoothly with any other write activity. But why not just
> > mark the 'background writeout was interrupted' bit here when breaking,
> > and have the thread check-clear that when finishing some other piece
> > of work (or when going idle)?
>   OK, although what would probably work as well would be to extend
> wb_check_old_data_flush() to also start background writeback (not only
> kupdate) if needed. Which probably makes sense regardless of any
> interruption of background writeback we might do... Maybe let's talk about
> this in some of the writeback sessions.

Good idea.  Then bdi_start_background_writeback() can simply wake it
up. I'd vote for this way before hitting more requirements possibility
in future.

Thanks,
Fengguang