All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chris Mason <chris.mason@oracle.com>
To: Jeff Moyer <jmoyer@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>,
	Lars Ellenberg <lars.ellenberg@linbit.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org, dm-devel@redhat.com,
	Neil Brown <neilb@suse.de>
Subject: Re: Why does __do_page_cache_readahead submit READ, not READA?
Date: Thu, 30 Jul 2009 12:56:16 -0400	[thread overview]
Message-ID: <20090730165616.GB5021@think> (raw)
In-Reply-To: <x49fxcedso6.fsf@segfault.boston.devel.redhat.com>

On Thu, Jul 30, 2009 at 12:47:21PM -0400, Jeff Moyer wrote:
> Chris Mason <chris.mason@oracle.com> writes:
> 
> > On Thu, Jul 30, 2009 at 08:06:49AM +0200, Jens Axboe wrote:
> >> On Wed, Jul 29 2009, Chris Mason wrote:
> >> > On Wed, Jul 29, 2009 at 11:18:45PM +0200, Jens Axboe wrote:
> >> > > On Wed, Jul 29 2009, Lars Ellenberg wrote:
> >> > > > I naively assumed, from the "readahead" in the name, that readahead
> >> > > > would be submitting READA bios. It does not.
> >> > > > 
> >> > > > I recently did some statistics on how many READ and READA requests
> >> > > > we actually see on the block device level.
> >> > > > I was suprised that READA is basically only used for file system
> >> > > > internal meta data (and not even for all file systems),
> >> > > > but _never_ for file data.
> >> > > > 
> >> > > > A simple
> >> > > > 	dd if=bigfile of=/dev/null bs=4k count=1
> >> > > > will absolutely cause readahead of the configured amount, no problem.
> >> > > > But on the block device level, these are READ requests, where I'd
> >> > > > expected them to be READA requests, based on the name.
> >> > > > 
> >> > > > This is because __do_page_cache_readahead() calls read_pages(),
> >> > > > which in turn is mapping->a_ops->readpages(), or, as fallback,
> >> > > > mapping->a_ops->readpage().
> >> > > > 
> >> > > > On that level, all variants end up submitting as READ.
> >> > > > 
> >> > > > This may even be intentional.
> >> > > > But if so, I'd like to understand that.
> >> > > 
> >> > > I don't think it's intentional, and if memory serves, we used to use
> >> > > READA when submitting read-ahead. Not sure how best to improve the
> >> > > situation, since (as you describe), we lose the read-ahead vs normal
> >> > > read at that level. I did some experimentation some time ago for
> >> > > flagging this, see:
> >> > > 
> >> > > http://git.kernel.dk/?p=linux-2.6-block.git;a=commitdiff;h=16cfe64e3568cda412b3cf6b7b891331946b595e
> >> > > 
> >> > > which should pass down READA properly.
> >> > 
> >> > One of the problems in the past was that reada would fail if there
> >> > wasn't a free request when we actually wanted it to go ahead and wait.
> >> > Or something.  We've switched it around a few times I think.
> >> 
> >> Yes, we did used to do that, whether it was 2.2 or 2.4 I
> >> don't recall :-)
> >> 
> >> It should be safe to enable know, whether there's a prettier way
> >> than the above, I don't know. It works by detecting the read-ahead
> >> marker, but it's a bit of a fragile design.
> >
> > I dug through my old email and found this fun bug w/buffer heads and
> > reada.
> >
> > 1) submit reada ll_rw_block on ext3 directory block
> > 2) decide that we really really need to wait on this block
> > 3) wait_on_buffer(bh) ; check up to date bit when done
> >
> > The problem in the bugzilla was that reada was returning EAGAIN or
> > EWOULDBLOCK, and the whole filesystem world expects that if we
> > wait_on_buffer and don't find the buffer up to date, its time
> > set things read only and run around screaming.
> >
> > The expectations in the code at the time were that the caller needs to
> > be aware the request may fail with EAGAIN/EWOULDBLOCK, but the reality
> > was that everyone who found that locked buffer also needed to be able to
> > check for it.  This one bugzilla had a teeny window where the reada
> > buffer head was leaked to the world.
> >
> > So, I think we can start using it again if it is just a hint to the
> > elevator about what to do with the IO, and we never actually turn the
> > READA into a transient failure (which I think is mostly true today, there
> > weren't many READA tests in the code I could see).
> 
> Well, is it a hint to the elevator or to the driver (or both)?

I would say both as long as they don't fail it.  IOW a priority decision
instead of a discard this request at will decision.

> The one
> bug I remember regarding READA failing was due to the FAILFAST bit
> getting set for READA I/O, and the powerpath driver returning a failure.
> Is that the bug to which you are referring?

This was a rhel bug with ext3 and (both dm and powerpath ) multipath, but in
theory it could be triggered on regular drives.  I don't think we ever
managed to, but removing READA definitely fixed it.

It was bug 213921 in the RH bugzilla, and I think it had been fixed in
other ways in mainline by the time we found it.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Chris Mason <chris.mason@oracle.com>
To: Jeff Moyer <jmoyer@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>,
	Lars Ellenberg <lars.ellenberg@linbit.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org, dm-devel@redhat.com,
	Neil Brown <neilb@suse.de>
Subject: Re: Why does __do_page_cache_readahead submit READ, not READA?
Date: Thu, 30 Jul 2009 12:56:16 -0400	[thread overview]
Message-ID: <20090730165616.GB5021@think> (raw)
In-Reply-To: <x49fxcedso6.fsf@segfault.boston.devel.redhat.com>

On Thu, Jul 30, 2009 at 12:47:21PM -0400, Jeff Moyer wrote:
> Chris Mason <chris.mason@oracle.com> writes:
> 
> > On Thu, Jul 30, 2009 at 08:06:49AM +0200, Jens Axboe wrote:
> >> On Wed, Jul 29 2009, Chris Mason wrote:
> >> > On Wed, Jul 29, 2009 at 11:18:45PM +0200, Jens Axboe wrote:
> >> > > On Wed, Jul 29 2009, Lars Ellenberg wrote:
> >> > > > I naively assumed, from the "readahead" in the name, that readahead
> >> > > > would be submitting READA bios. It does not.
> >> > > > 
> >> > > > I recently did some statistics on how many READ and READA requests
> >> > > > we actually see on the block device level.
> >> > > > I was suprised that READA is basically only used for file system
> >> > > > internal meta data (and not even for all file systems),
> >> > > > but _never_ for file data.
> >> > > > 
> >> > > > A simple
> >> > > > 	dd if=bigfile of=/dev/null bs=4k count=1
> >> > > > will absolutely cause readahead of the configured amount, no problem.
> >> > > > But on the block device level, these are READ requests, where I'd
> >> > > > expected them to be READA requests, based on the name.
> >> > > > 
> >> > > > This is because __do_page_cache_readahead() calls read_pages(),
> >> > > > which in turn is mapping->a_ops->readpages(), or, as fallback,
> >> > > > mapping->a_ops->readpage().
> >> > > > 
> >> > > > On that level, all variants end up submitting as READ.
> >> > > > 
> >> > > > This may even be intentional.
> >> > > > But if so, I'd like to understand that.
> >> > > 
> >> > > I don't think it's intentional, and if memory serves, we used to use
> >> > > READA when submitting read-ahead. Not sure how best to improve the
> >> > > situation, since (as you describe), we lose the read-ahead vs normal
> >> > > read at that level. I did some experimentation some time ago for
> >> > > flagging this, see:
> >> > > 
> >> > > http://git.kernel.dk/?p=linux-2.6-block.git;a=commitdiff;h=16cfe64e3568cda412b3cf6b7b891331946b595e
> >> > > 
> >> > > which should pass down READA properly.
> >> > 
> >> > One of the problems in the past was that reada would fail if there
> >> > wasn't a free request when we actually wanted it to go ahead and wait.
> >> > Or something.  We've switched it around a few times I think.
> >> 
> >> Yes, we did used to do that, whether it was 2.2 or 2.4 I
> >> don't recall :-)
> >> 
> >> It should be safe to enable know, whether there's a prettier way
> >> than the above, I don't know. It works by detecting the read-ahead
> >> marker, but it's a bit of a fragile design.
> >
> > I dug through my old email and found this fun bug w/buffer heads and
> > reada.
> >
> > 1) submit reada ll_rw_block on ext3 directory block
> > 2) decide that we really really need to wait on this block
> > 3) wait_on_buffer(bh) ; check up to date bit when done
> >
> > The problem in the bugzilla was that reada was returning EAGAIN or
> > EWOULDBLOCK, and the whole filesystem world expects that if we
> > wait_on_buffer and don't find the buffer up to date, its time
> > set things read only and run around screaming.
> >
> > The expectations in the code at the time were that the caller needs to
> > be aware the request may fail with EAGAIN/EWOULDBLOCK, but the reality
> > was that everyone who found that locked buffer also needed to be able to
> > check for it.  This one bugzilla had a teeny window where the reada
> > buffer head was leaked to the world.
> >
> > So, I think we can start using it again if it is just a hint to the
> > elevator about what to do with the IO, and we never actually turn the
> > READA into a transient failure (which I think is mostly true today, there
> > weren't many READA tests in the code I could see).
> 
> Well, is it a hint to the elevator or to the driver (or both)?

I would say both as long as they don't fail it.  IOW a priority decision
instead of a discard this request at will decision.

> The one
> bug I remember regarding READA failing was due to the FAILFAST bit
> getting set for READA I/O, and the powerpath driver returning a failure.
> Is that the bug to which you are referring?

This was a rhel bug with ext3 and (both dm and powerpath ) multipath, but in
theory it could be triggered on regular drives.  I don't think we ever
managed to, but removing READA definitely fixed it.

It was bug 213921 in the RH bugzilla, and I think it had been fixed in
other ways in mainline by the time we found it.

-chris


  reply	other threads:[~2009-07-30 16:56 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-07-29 16:14 Why does __do_page_cache_readahead submit READ, not READA? Lars Ellenberg
2009-07-29 16:14 ` Lars Ellenberg
2009-07-29 21:18 ` Jens Axboe
2009-07-29 21:18   ` Jens Axboe
2009-07-29 22:55   ` Chris Mason
2009-07-29 22:55     ` Chris Mason
2009-07-30  6:06     ` Jens Axboe
2009-07-30  6:06       ` Jens Axboe
2009-07-30 14:34       ` Chris Mason
2009-07-30 14:34         ` Chris Mason
2009-07-30 16:47         ` Jeff Moyer
2009-07-30 16:47           ` Jeff Moyer
2009-07-30 16:56           ` Chris Mason [this message]
2009-07-30 16:56             ` Chris Mason
2009-08-03  7:52       ` Wu Fengguang
2009-08-03  7:52         ` Wu Fengguang
2009-08-03  7:59         ` Jens Axboe
2009-08-03  7:59           ` Jens Axboe
2009-08-03  8:23           ` Wu Fengguang
2009-08-03  8:23             ` Wu Fengguang
2009-08-03  9:25             ` Jens Axboe
2009-08-03  9:25               ` Jens Axboe
2009-08-03  9:34               ` Wu Fengguang
2009-08-03  9:34                 ` Wu Fengguang
2009-08-03  9:37                 ` Jens Axboe
2009-08-03  9:37                   ` Jens Axboe
2009-08-03  9:44                   ` Wu Fengguang
2009-08-03  9:44                     ` Wu Fengguang
2009-08-03 14:26           ` [dm-devel] " James Bottomley
2009-08-03 14:26             ` James Bottomley
2009-08-03 21:03             ` Jens Axboe
2009-08-03 21:03               ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090730165616.GB5021@think \
    --to=chris.mason@oracle.com \
    --cc=dm-devel@redhat.com \
    --cc=jens.axboe@oracle.com \
    --cc=jmoyer@redhat.com \
    --cc=lars.ellenberg@linbit.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=neilb@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.