Re: [PATCH 8/8] vm: Add an tuning knob for vm.max_writeback_mb

[Chris Mason <chris.mason@oracle.com>]

On Tue, Sep 08, 2009 at 06:56:23PM +0200, Peter Zijlstra wrote:
> On Tue, 2009-09-08 at 12:29 -0400, Chris Mason wrote:
> 
> > > I'm still not convinced this knob is worth the patch and I'm inclined to
> > > flat out NAK it..
> > > 
> > > The whole point of MAX_WRITEBACK_PAGES seems to occasionally check the
> > > dirty stats again and not write out too much.
> > 
> > The problem is that 'too much' is a very abstract thing.  When a process
> > is stuck in balance_dirty_pages, we want them to do the minimal amount
> > of work (or waiting) required to get them safely back inside file_write().
> 
> >From the VMs POV I think we'd like to keep near the dirty limit as that
> maximizes the write cache efficiency. Of course that needs to be
> balanced against write out efficiency.
> 
> > > Clearly the current limit isn't sufficient for some people,
> > >  - xfs/btrfs seem generally stuck in balance_dirty_pages()'s
> > > congestion_wait()
> > >  - ext4 generates inconveniently small extents
> > 
> > This is actually two different side of the same problem.  The filesystem
> > knows that bytes 0-N in the file are setup for delayed allocation.
> > Writepage is called on byte 0, and now the filesystem gets to decide how
> > big an extent to make.
> > 
> > It could decide to make an extent based on the total number of bytes
> > under delayed allocation, and hope the caller of writepage will be kind
> > enough to send down the pages contiguously afterward (xfs), or it could
> > make a smaller extent based on something closer to the total number of
> > bytes this particular writepages() call plans on writing (I guess what
> > ext4 is doing).
> > 
> > Either way, if pdflush or the bdi thread or whoever ends up switching to
> > another file during a big streaming write, the end result is that we
> > fragment.  We may fragment the file (ext4) or we may fragment the
> > writeback (xfs), but the end result isn't good.
> 
> OK, so what we want is for a way to re-enter the whole
> writeback_inodes() path onto the same file, right?

It would help.

> 
> That would result in the writeback continuing where it left off last.
> 
> Wu, can we make writeback_inodes() do something like that? Pass some
> magic along in wbc maybe?
> 
> > Looking at two xfs examples, this is the IO for two concurrent streaming
> > writers (two different files) on 2.6.31-rc8 (pdflush is doing all the IO
> > in this graph, sorry the legend colors wrapped on me).  If you squint,
> > you can kind of see the fingers of IO as pdflush switches between files.
> > 
> > http://oss.oracle.com/~mason/seekwatcher/xfs-tag.png
> > 
> > And here is the IO when XFS forces nr_to_write much higher with a patch
> > from Christoph:
> > 
> > http://oss.oracle.com/~mason/seekwatcher/xfs-extend-tag.png
> > 
> > These graphs would look the same no matter what I did with
> > congestion_wait().  The first graph is slower just because pdflush
> > switches from one file to another.
> > 
> > > 
> > > 
> > > The first seems to suggest to me the number isn't well balanced against
> > > whatever drives congestion_wait() (that thing still gives me a
> > > head-ache).
> > > 
> > > # git grep clear_bdi_congested
> > > drivers/block/pktcdvd.c:                clear_bdi_congested(&pd->disk->queue->backing_dev_info,
> > > fs/fuse/dev.c:                  clear_bdi_congested(&fc->bdi, BLK_RW_SYNC);
> > > fs/fuse/dev.c:                  clear_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
> > > fs/nfs/write.c:         clear_bdi_congested(&nfss->backing_dev_info, BLK_RW_ASYNC);
> > > include/linux/backing-dev.h:void clear_bdi_congested(struct backing_dev_info *bdi, int sync);
> > > include/linux/blkdev.h: clear_bdi_congested(&q->backing_dev_info, sync);
> > > mm/backing-dev.c:void clear_bdi_congested(struct backing_dev_info *bdi, int sync)
> > > mm/backing-dev.c:EXPORT_SYMBOL(clear_bdi_congested);
> > > 
> > > Suggests that regular block devices don't even manage device congestion
> > > and it reverts to a simple timeout -- should we fix that?
> > 
> > Look for blk_clear_queue_congested().  It is managed, I personally don't
> > think it is very useful.  But, that's a different thread ;)
> 
> Ah, how blind I am ;-)
> 
> Right, so what can we do to make it useful? I think the intent is to
> limit the number of pages in writeback and provide some progress
> feedback to the vm.
> 
> Going by your experience we're failing there.

Well, congestion_wait is a stop sign but not a queue.  So, if you're
being nice and honoring congestion but another process (say O_DIRECT
random writes) doesn't, then you back off forever and none of your IO
gets done.

To get around this, you can add code to make sure that you do
_some_ io, but this isn't enough for your work to get done
quickly, and you do end up waiting in get_request() so the async
benefits of using the congestion test go away.

If we changed everyone to honor congestion, we end up with a poll model
because a ton of congestion_wait() callers create a thundering herd.

So, we could add a queue, and then congestion_wait() would look a lot
like get_request_wait().  I'd rather that everyone just used
get_request_wait, and then have us fix any latency problems in the
elevator.

For me, perfect would be one or more threads per-bdi doing the
writeback, and never checking for congestion (like what Jens' code
does).  The congestion_wait inside balance_dirty_pages() is really just
a schedule_timeout(), on a fully loaded box the congestion doesn't go
away anyway.  We should switch that to a saner system of waiting for
progress on the bdi writeback + dirty thresholds.

Btrfs would love to be able to send down a bio non-blocking.  That would
let me get rid of the congestion check I have today (I think Jens said
that would be an easy change and then I talked him into some small mods
of the writeback path).

> 
> > > Now, suppose it were to do something useful, I'd think we'd want to
> > > limit write-out to whatever it takes so saturate the BDI.
> > 
> > If we don't want a blanket increase, 
> 
> The thing is, this sysctl seems an utter cop out, we can't even explain
> how to calculate a number that'll work for a situation, the best we can
> do is say, prod at it and pray -- that's not good.
> 
> Last time I also asked if an increased number is good for every
> situation, I have a machine with a RAID5 array and USB storage, will it
> harm either situation?

If the goal is to make sure that pdflush or balance_dirty_pages only
does IO until some condition is met, we should add a flag to the bdi
that gets set when that condition is met.  Things will go a lot more
smoothly than magic numbers.

Then we can add the fs_hint as another change so the FS can tell
write_cache_pages callers how to do optimal IO based on its allocation
decisions.

> 
> > I'd suggest that we just give the
> > FS a way to say: 'I know nr_to_write is only 32, but if you just write a
> > few blocks more, the system will be better off'.
> > 
> > Something like wbc->fs_write_hint
> > 
> > This way, when the FS allocates a great big contiguous delalloc extent,
> > it can set the wbc to reflect that we've got cheap and easy IO here.
> 
> I think that's certainly a possibility.
> 
> What's the down-side of allocating extents based on the available dirty
> pages instead of the current write-out request? As long as we're good at
> generating sequential IO in general (yeah, I know we suck now) it
> doesn't really matter when it will be filled, as we know it will
> eventually be.

I'm guessing the small extents from ext4 come from tuning the allocator
for writeback performance instead of anti-fragmentation.  But I'm
guessing.

-chris