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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
