lkml.org 
[lkml]   [2009]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: mmotm 2009-06-02-16-11 uploaded (readahead)
    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.


    \
     
     \ /
      Last update: 2009-06-09 06:53    [W:0.029 / U:0.040 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site