Re: [PATCH 2/2] writeback: Replace some redirty_tail() calls with requeue_io()

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

On Wed, Oct 12, 2011 at 05:53:59AM +0800, Jan Kara wrote:
> On Tue 11-10-11 10:36:38, Wu Fengguang wrote:
> > On Tue, Oct 11, 2011 at 07:30:07AM +0800, Jan Kara wrote:
> > > On Mon 10-10-11 19:31:30, Wu Fengguang wrote:
> > > > On Mon, Oct 10, 2011 at 07:21:33PM +0800, Jan Kara wrote:
> > > > >   Hi Fengguang,
> > > > > 
> > > > > On Sat 08-10-11 12:00:36, Wu Fengguang wrote:
> > > > > > The test results look not good: btrfs is heavily impacted and the
> > > > > > other filesystems are slightly impacted.
> > > > > >
> > > > > > I'll send you the detailed logs in private emails (too large for the
> > > > > > mailing list).  Basically I noticed many writeback_wait traces that never
> > > > > > appear w/o this patch.
> > > > >   OK, thanks for running these tests. I'll have a look at detailed logs.
> > > > > I guess the difference can be caused by changes in redirty/requeue logic in
> > > > > the second patch (the changes in the first patch could possibly make
> > > > > several writeback_wait events from one event but never could introduce new
> > > > > events).
> > > > > 
> > > > > I guess I'll also try to reproduce the problem since it should be pretty
> > > > > easy when you see such a huge regression even with 1 dd process on btrfs
> > > > > filesystem.
> > > > > 
> > > > > > In the btrfs cases that see larger regressions, I see large fluctuations
> > > > > > in the writeout bandwidth and long disk idle periods. It's still a bit
> > > > > > puzzling how all these happen..
> > > > >   Yes, I don't understand it yet either...
> > > > 
> > > > Jan, it's obviously caused by this chunk, which is not really
> > > > necessary for fixing Christoph's problem. So the easy way is to go
> > > > ahead without this chunk.
> > >   Yes, thanks a lot for debugging this! I'd still like to understand why
> > > the hunk below is causing such a big problem to btrfs. I was looking into
> > > the traces and all I could find so far was that for some reason relevant
> > > inode (ino 257) was not getting queued for writeback for a long time (e.g.
> > > 20 seconds) which introduced disk idle times and thus bad throughput. But I
> > > don't understand why the inode was not queue for such a long time yet...
> > > Today it's too late but I'll continue with my investigation tomorrow.
> > 
> > Yeah, I have exactly the same observation and puzzle..
>   OK, I dug more into this and I think I found an explanation. The problem
> starts at
>    flush-btrfs-1-1336  [005]    20.688011: writeback_start: bdi btrfs-1:
> sb_dev 0:0 nr_pages=23685 sync_mode=0 kupdate=1 range_cyclic=1 background=0
> reason=periodic
>   in the btrfs trace you sent me when we start "kupdate" style writeback
> for bdi "btrfs-1". This work then blocks flusher thread upto moment:
>    flush-btrfs-1-1336  [007]    45.707479: writeback_start: bdi btrfs-1:
> sb_dev 0:0 nr_pages=18173 sync_mode=0 kupdate=1 range_cyclic=1 background=0
> reason=periodic
>    flush-btrfs-1-1336  [007]    45.707479: writeback_written: bdi btrfs-1:
> sb_dev 0:0 nr_pages=18173 sync_mode=0 kupdate=1 range_cyclic=1 background=0
> reason=periodic
> 
>   (i.e. for 25 seconds). The reason why this work blocks flusher thread for
> so long is that btrfs has "btree inode" - essentially an inode holding
> filesystem metadata and btrfs ignores any ->writepages() request for this
> inode coming from kupdate style writeback. So we always try to write this
> inode, make no progress, requeue inode (as it has still mapping tagged as
> dirty), see that b_more_io is nonempty so we sleep for a while and then
> retry. We do not include inode 257 with real dirty data into writeback
> because this is kupdate style writeback and inode 257 does not have dirty
> timestamp old enough. This loop would break either after 30s when inode
> with data becomes old enough or - as we see above - at the moment when
> btrfs decided to do transaction commit and cleaned metadata inode by it's
> own methods. In either case this is far too late...

Yes indeed. Good catch! 

The implication of this case is, never put an inode to b_more_io
unless made some progress on cleaning some pages or the metadata.

Failing to do so will lead to

- busy looping (which can be fixed by patch 1/2 "writeback: Improve busyloop prevention")

- block the current work (and in turn the other queued works) for long time,
  where the other pending works may tend to work on a different set of
  inodes or have different criteria for the FS to make progress. The
  existing examples are the for_kupdate test in btrfs and the SYNC vs
  ASYNC tests in general. And I'm planning to send writeback works
  from the vmscan code to write a specific inode..

In this sense, it looks not the right direction to convert the
redirty_tail() calls to requeue_io().

If we change redirty_tail() to the earlier proposed requeue_io_wait(),
all the known problems can be solved nicely.

>   So for now I don't see a better alternative than to revert to old
> behavior in writeback_single_inode() as you suggested earlier. That way we
> would redirty_tail() inodes which we cannot clean and thus they won't cause
> livelocking of kupdate work.

requeue_io_wait() can equally avoid touching inode->dirtied_when :)

> Longer term we might want to be more clever in
> switching away from kupdate style writeback to pure background writeback
> but it's not yet clear to me what the logic should be so that we give
> enough preference to old inodes...

We'll need to adequately update older_than_this in the wb_writeback()
loop for background work. Then we can make the switch.

>   New version of the second patch is attached.
> 
> 								Honza

> @@ -583,10 +597,10 @@ static long writeback_sb_inodes(struct super_block *sb,
>  			wrote++;
>  		if (wbc.pages_skipped) {
>  			/*
> -			 * writeback is not making progress due to locked
> -			 * buffers.  Skip this inode for now.
> +			 * Writeback is not making progress due to unavailable
> +			 * fs locks or similar condition. Retry in next round.
>  			 */
> -			redirty_tail(inode, wb);
> +			requeue_io(inode, wb);
>  		}
>  		spin_unlock(&inode->i_lock);
>  		spin_unlock(&wb->list_lock);

In the case writeback_single_inode() just redirty_tail()ed the inode,
it's not good to requeue_io() it here. So I'd suggest to keep the
original code, or remove the if(pages_skipped){} block totally.

Thanks,
Fengguang