Re: mmotm 2009-06-02-16-11 uploaded (readahead)

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

On Tue, Jun 09, 2009 at 12:38:17PM +0800, Andrew Morton wrote:
> On Tue, 9 Jun 2009 05:59:16 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > ...
> > > Doing a block-specific call from inside page_cache_async_readahead() is
> > > a bit of a layering violation - this may not be a block-backed
> > > filesystem at all.
> > > 
> > > otoh, perhaps blk_run_backing_dev() is wrongly named and defined in the
> > > wrong place.  Perhaps non-block-backed backing_devs want to implement
> > > an unplug-style function too?  In which case the whole thing should be
> > > renamed and moved outside blkdev.h.
> > > 
> > > If we don't want to do that, shouldn't backing_dev_info.unplug* be
> > > wrapped in #ifdef CONFIG_BLOCK?  And wasn't it a layering violation to
> > > put block-specific things into the backing_dev_info?
> > > 
> > > Jens, talk to me!
> > > 
> > > From the readahead POV: does it make sense to call the backing-dev's
> > > "unplug" function even if that isn't a block-based device?  Or was this
> > > just a weird block-device-only performance problem?  Hard to say.
> > 
> > Layering wise, I don't think it's that bad. It would have looked cleaner
> > to do:
> > 
> >         blk_run_address_space(mapping);
> > 
> > instead, but we would still need to make that available outside of
> > CONFIG_BLOCK as well.
> > 
> > What I don't like about the patch is that it's a heuristic, a "I poked
> > this and it made that faster" with nobody really understanding why.
> 
> Well.  I _think_ we understand it.  I'm not sure that we understand why
> it made scst faster though.

Because the NFS/SCST servers are running RAID?

Also the client side NFS/SCST IO request may be slitted up and served
by a pool of server processes, which introduces the same disorderness
as in RAID configuration. But I wonder whether blk_* work for them,
or NFS/SCST have the "plug" concept at all.

> > And
> > it's second guessing the block layer unplugging, so perhaps the real fix
> > should be going on there. Or perhaps it's just fine and this micro
> > optimization just helps this one case and that's great.
> > 
> > So ho humm, not terribly excited about it, but I guess we can shove it
> > in there for testing. But lets please use blk_run_address_space() and
> > add an empty stub for that.
> 
> But blk_anything() shouldn't be in the readahead code - readahead isn't
> specific to block-based devices!

Yup, the "#ifdef CONFIG_BLOCK" looks ugly..

Thanks,
Fengguang

> y:/usr/src/25> egrep "blk|block" mm/readahead.c 
> #include <linux/blkdev.h>
>  * block layer to abandon the readahead if request allocation would block.
>  * force_page_cache_readahead() will ignore queue congestion and will block on
> y:/usr/src/25> 
> 
> 
> >From a layering POV we should have some mapping_start_io(address_space
> *) which of course calls blk_run_address_space() if it's a block-backed
> and calls <something else> if it's not block-backed.  Problem is, if
> the backing device is, say, NFS then we have no reason to believe that
> starting IO at this time is beneficial to NFS.
> 
> But sure, the world wouldn't end if we put a block-specific IO hint in
> there.  It just isn't quite right.