lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 9 Jun 2009 05:59:16 +0200
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Randy Dunlap <randy.dunlap@...cle.com>,
	linux-kernel@...r.kernel.org, hifumi.hisashi@....ntt.co.jp,
	Wu Fengguang <fengguang.wu@...el.com>
Subject: Re: mmotm 2009-06-02-16-11 uploaded (readahead)

On Wed, Jun 03 2009, Andrew Morton wrote:
> On Tue, 02 Jun 2009 20:54:39 -0700
> Randy Dunlap <randy.dunlap@...cle.com> wrote:
> 
> > akpm@...ux-foundation.org wrote:
> > > The mm-of-the-moment snapshot 2009-06-02-16-11 has been uploaded to
> > > 
> > >    http://userweb.kernel.org/~akpm/mmotm/
> > > 
> > > and will soon be available at
> > > 
> > >    git://git.zen-sources.org/zen/mmotm.git
> > 
> > 
> > readahead-add-blk_run_backing_dev.patch:
> > 
> > mm/readahead.c: In function 'page_cache_async_readahead':
> > mm/readahead.c:559: error: implicit declaration of function 'blk_run_backing_dev'
> 
> hm, yeah, CONFIG_BLOCK=n.
> 
> 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. 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.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists