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

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

On Mon, May 25 2009, Jan Kara wrote:
>   Hi Jens,
> 
>   I've noticed a few more things:
> 
> On Mon 25-05-09 09:34:39, Jens Axboe wrote:
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 1137408..7cb4d02 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -19,6 +19,8 @@
> >  #include <linux/sched.h>
> >  #include <linux/fs.h>
> >  #include <linux/mm.h>
> > +#include <linux/kthread.h>
> > +#include <linux/freezer.h>
> >  #include <linux/writeback.h>
> >  #include <linux/blkdev.h>
> >  #include <linux/backing-dev.h>
> > @@ -61,10 +63,193 @@ int writeback_in_progress(struct backing_dev_info *bdi)
> >   */
> >  static void writeback_release(struct backing_dev_info *bdi)
> >  {
> > -	BUG_ON(!writeback_in_progress(bdi));
> > +	WARN_ON_ONCE(!writeback_in_progress(bdi));
> > +	bdi->wb_arg.nr_pages = 0;
> > +	bdi->wb_arg.sb = NULL;
> >  	clear_bit(BDI_pdflush, &bdi->state);
> >  }
> >  
> > +int bdi_start_writeback(struct backing_dev_info *bdi, struct super_block *sb,
> > +			 long nr_pages, enum writeback_sync_modes sync_mode)
> > +{
> > +	/*
> > +	 * This only happens the first time someone kicks this bdi, so put
> > +	 * it out-of-line.
> > +	 */
> > +	if (unlikely(!bdi->task)) {
> > +		bdi_add_default_flusher_task(bdi);
> > +		return 1;
> > +	}
> > +
> > +	if (writeback_acquire(bdi)) {
> > +		bdi->wb_arg.nr_pages = nr_pages;
> > +		bdi->wb_arg.sb = sb;
> > +		bdi->wb_arg.sync_mode = sync_mode;
> > +		/*
> > +		 * make above store seen before the task is woken
> > +		 */
> > +		smp_mb();
> > +		wake_up(&bdi->wait);
> > +	}
>   Hmm, wouldn't the interface be more useful if we could just pass down the
> whole writeback_control to the flusher threads?

Yeah, that's what the later patch does too, when support for > 1 thread
has been added.

>   Maybe a real question is, what should be the role of flusher threads?
> Should they be just threads for kupdate / pdflush style writeout or do we
> expect them to be used also for other cases when we need to submit IO to
> the BDI?

The thought has occured to me to punt other types of work to these
threads, now that we have them available. I haven't really thought that
through yet (and other changes may be required too), but it could be
things like async IO or even simple things like the unplugging that is
now done by kblockd.

> > + * The maximum number of pages to writeout in a single bdi flush/kupdate
> > + * operation.  We do this so we don't hold I_SYNC against an inode for
> > + * enormous amounts of time, which would block a userspace task which has
> > + * been forced to throttle against that inode.  Also, the code reevaluates
> > + * the dirty each time it has written this many pages.
> > + */
> > +#define MAX_WRITEBACK_PAGES     1024
> > +
> > +/*
> > + * Periodic writeback of "old" data.
> > + *
> > + * Define "old": the first time one of an inode's pages is dirtied, we mark the
> > + * dirtying-time in the inode's address_space.  So this periodic writeback code
> > + * just walks the superblock inode list, writing back any inodes which are
> > + * older than a specific point in time.
> > + *
> > + * Try to run once per dirty_writeback_interval.  But if a writeback event
> > + * takes longer than a dirty_writeback_interval interval, then leave a
> > + * one-second gap.
> > + *
> > + * older_than_this takes precedence over nr_to_write.  So we'll only write back
> > + * all dirty pages if they are all attached to "old" mappings.
> > + */
> > +static void bdi_kupdated(struct backing_dev_info *bdi)
> > +{
> > +	unsigned long oldest_jif;
> > +	long nr_to_write;
> > +	struct writeback_control wbc = {
> > +		.bdi			= bdi,
> > +		.sync_mode		= WB_SYNC_NONE,
> > +		.older_than_this	= &oldest_jif,
> > +		.nr_to_write		= 0,
> > +		.for_kupdate		= 1,
> > +		.range_cyclic		= 1,
> > +	};
> > +
> > +	sync_supers();
>   Hmm, so each BDI flusher thread is going to sync all the superblocks?
> Isn't there a better way? I suppose we *should* be able to somehow go
> from a BDI to a superblock (or maybe a list of those) so that we can write
> per-fs metadata not bound to inodes.

Good catch, we should probably have that be sync_supers_bdi() and have a
way to leaf through the super(s) that are mounted on this bdi. Added to
the list.

> > +
> > +	oldest_jif = jiffies - msecs_to_jiffies(dirty_expire_interval * 10);
> > +
> > +	nr_to_write = global_page_state(NR_FILE_DIRTY) +
> > +			global_page_state(NR_UNSTABLE_NFS) +
> > +			(inodes_stat.nr_inodes - inodes_stat.nr_unused);
> > +
> > +	while (nr_to_write > 0) {
> > +		wbc.more_io = 0;
> > +		wbc.encountered_congestion = 0;
> > +		wbc.nr_to_write = MAX_WRITEBACK_PAGES;
> > +		generic_sync_bdi_inodes(NULL, &wbc);
> > +		if (wbc.nr_to_write > 0)
> > +			break;	/* All the old data is written */
> > +		nr_to_write -= MAX_WRITEBACK_PAGES;
> > +	}
> > +}
> > +
> > +static inline bool over_bground_thresh(void)
> > +{
> > +	unsigned long background_thresh, dirty_thresh;
> > +
> > +	get_dirty_limits(&background_thresh, &dirty_thresh, NULL, NULL);
> > +
> > +	return (global_page_state(NR_FILE_DIRTY) +
> > +		global_page_state(NR_UNSTABLE_NFS) >= background_thresh);
> > +}
> > +
> > +static void bdi_pdflush(struct backing_dev_info *bdi)
> > +{
> > +	struct writeback_control wbc = {
> > +		.bdi			= bdi,
> > +		.sync_mode		= bdi->wb_arg.sync_mode,
> > +		.older_than_this	= NULL,
> > +		.range_cyclic		= 1,
> > +	};
> > +	long nr_pages = bdi->wb_arg.nr_pages;
> > +
> > +	for (;;) {
> > +		if (wbc.sync_mode == WB_SYNC_NONE && nr_pages <= 0 &&
> > +		    !over_bground_thresh())
> > +			break;
> > +
> > +		wbc.more_io = 0;
> > +		wbc.encountered_congestion = 0;
> > +		wbc.nr_to_write = MAX_WRITEBACK_PAGES;
> > +		wbc.pages_skipped = 0;
> > +		generic_sync_bdi_inodes(bdi->wb_arg.sb, &wbc);
> > +		nr_pages -= MAX_WRITEBACK_PAGES - wbc.nr_to_write;
> > +		/*
> > +		 * If we ran out of stuff to write, bail unless more_io got set
> > +		 */
> > +		if (wbc.nr_to_write > 0 || wbc.pages_skipped > 0) {
> > +			if (wbc.more_io)
> > +				continue;
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +/*
> > + * Handle writeback of dirty data for the device backed by this bdi. Also
> > + * wakes up periodically and does kupdated style flushing.
> > + */
> > +int bdi_writeback_task(struct backing_dev_info *bdi)
> > +{
> > +	while (!kthread_should_stop()) {
> > +		unsigned long wait_jiffies;
> > +		DEFINE_WAIT(wait);
> > +
> > +		prepare_to_wait(&bdi->wait, &wait, TASK_INTERRUPTIBLE);
> > +		wait_jiffies = msecs_to_jiffies(dirty_writeback_interval * 10);
> > +		schedule_timeout(wait_jiffies);
> > +		try_to_freeze();
> > +
> > +		/*
> > +		 * We get here in two cases:
> > +		 *
> > +		 *  schedule_timeout() returned because the dirty writeback
> > +		 *  interval has elapsed. If that happens, we will be able
> > +		 *  to acquire the writeback lock and will proceed to do
> > +		 *  kupdated style writeout.
> > +		 *
> > +		 *  Someone called bdi_start_writeback(), which will acquire
> > +		 *  the writeback lock. This means our writeback_acquire()
> > +		 *  below will fail and we call into bdi_pdflush() for
> > +		 *  pdflush style writeout.
> > +		 *
> > +		 */
> > +		if (writeback_acquire(bdi))
> > +			bdi_kupdated(bdi);
> > +		else
> > +			bdi_pdflush(bdi);
> > +
> > +		writeback_release(bdi);
> > +		finish_wait(&bdi->wait, &wait);
> > +	}
>   Hmm, what does protect this thread from racing with umount? Note that old
> flusher threads took s_umount semaphore and also elevated sb->s_count.
> If everything is fine, we should have a comment somewhere around here what
> stops umount from removing things under us or why it does not matter...
>   Maybe this is the reason of the oopses Yanmin saw.

I think it was that oops, it got fixed by generic_sync_sb_inodes() doing
sync writeout again. So I think what happened is that it closed that
hole, but there could still be trouble. I'll look into that as well.

>   BTW, one more difference here: Previously, if pdflush saw congestion on
> the device, it did congestion_wait() and retried. Now it end's up waiting
> whole dirty_writeback_interval so it should result in a bursty writeback on
> a slow disk, couldn't it?

Now it'll block on the queue instead of giving up, so in my opinion that
should provide smoother writeout. From the tests I did, it does.

> > +void bdi_writeback_all(struct super_block *sb, long nr_pages,
> > +		       enum writeback_sync_modes sync_mode)
> > +{
> > +	struct backing_dev_info *bdi, *tmp;
> > +
> > +	mutex_lock(&bdi_lock);
> > +
> > +	list_for_each_entry_safe(bdi, tmp, &bdi_list, bdi_list) {
> > +		if (!bdi_has_dirty_io(bdi))
> > +			continue;
> > +		bdi_start_writeback(bdi, sb, nr_pages, sync_mode);
> > +	}
> > +
> > +	mutex_unlock(&bdi_lock);
> > +}
> > +
> >  /**
> >   *	__mark_inode_dirty -	internal function
> >   *	@inode: inode to mark
> ...
> > @@ -591,13 +718,10 @@ static void generic_sync_bdi_inodes(struct backing_dev_info *bdi,
> >  void generic_sync_sb_inodes(struct super_block *sb,
> >  				struct writeback_control *wbc)
> >  {
> > -	const int is_blkdev_sb = sb_is_blkdev_sb(sb);
> > -	struct backing_dev_info *bdi;
> > -
> > -	mutex_lock(&bdi_lock);
> > -	list_for_each_entry(bdi, &bdi_list, bdi_list)
> > -		generic_sync_bdi_inodes(bdi, wbc, sb, is_blkdev_sb);
> > -	mutex_unlock(&bdi_lock);
> > +	if (wbc->bdi)
> > +		generic_sync_bdi_inodes(sb, wbc);
> > +	else
> > +		bdi_writeback_all(sb, wbc->nr_to_write, wbc->sync_mode);
>   Hmm, you write in the changelog that bdi_writeback_all() writes inline
> now but AFAICS it still happens through the writeback threads. Which, on a
> second though probably *is* what we want because we want writeback to go in
> parallel on all devices we have.

Note that this is patch 4, later it is sync. But yes, I think it should
be async as well, but we do need a way to wait on the submitted work to
be processed in case of WB_SYNC_ALL.

> >   * Start writeback of `nr_pages' pages.  If `nr_pages' is zero, write back
> >   * the whole world.  Returns 0 if a pdflush thread was dispatched.  Returns
> >   * -1 if all pdflush threads were busy.
> >   */
> > -int wakeup_pdflush(long nr_pages)
> > +void wakeup_flusher_threads(long nr_pages)
> >  {
> >  	if (nr_pages == 0)
> >  		nr_pages = global_page_state(NR_FILE_DIRTY) +
> >  				global_page_state(NR_UNSTABLE_NFS);
> > -	return pdflush_operation(background_writeout, nr_pages);
> > +	bdi_writeback_all(NULL, nr_pages, WB_SYNC_NONE);
> > +	return;
> >  }
> ...
> > -
> > -static void laptop_flush(unsigned long unused)
> > -{
> > -	sys_sync();
> > -}
> > -
> >  static void laptop_timer_fn(unsigned long unused)
> >  {
> > -	pdflush_operation(laptop_flush, 0);
> > +	wakeup_flusher_threads(0);
> >  }
> >  
> >  /*
>   Here you significantly change the behavior of laptop mode - previously it
> reliably synced all the data to disk once the "laptop writeout timeout"
> elapsed. The idea is that we want to write all the dirty data we have so
> that the drive can go to sleep for a longer period.

Yeah I know, I wrote the original laptop mode implementation :-)

>   Now you changed that to asynchronous WB_SYNC_NONE writeback of some
> number of pages.  In particular if the disk gets congested, we'll just stop
> doing writeback which probably isn't what we want for laptop mode...

Congestion doesn't matter, we don't do nonblocking writeout from the
threads at all. And it is sync later in the series.

I'll switch it back to async in general throughout the series, and add
some way of making sure we actually have things submitted before doing
the filemap_fdatawait() in generic_sync_sb_inodes(). I think that should
fix it.

-- 
Jens Axboe