[1/1] UBUNTU: [Upstream] elevator: fix fastfail checks to allow merge of readahead requests

Submitted by Andy Whitcroft on Oct. 13, 2009, 11:18 a.m.

Details

Message ID 1255432719-19615-2-git-send-email-apw@canonical.com
State Accepted
Commit 6830be77b871fa5220a52f733f644de5b2b1a670
Headers show

Commit Message

Andy Whitcroft Oct. 13, 2009, 11:18 a.m.
BugLink: http://bugs.launchpad.net/bugs/444915

When a readahead request is injected into the elevator we construct a
bio as below, treating it as fastfail:

    void init_request_from_bio(struct request *req, struct bio *bio)
    [...]
        if (bio_rw_ahead(bio))
                req->cmd_flags |= (REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
                                   REQ_FAILFAST_DRIVER);
    [...]

However when merging requests in the elevator we only allow merges of
new bios with matching fastfail attributes:

    int elv_rq_merge_ok(struct request *rq, struct bio *bio)
    [...]
        if (!bio_failfast_dev(bio)       != !blk_failfast_dev(rq) ||
            !bio_failfast_transport(bio) != !blk_failfast_transport(rq) ||
            !bio_failfast_driver(bio)    != !blk_failfast_driver(rq))
    [...]

This check occurs against the original bio fastfail bits ignoring the
override and thus preventing merge of an existing readahead request with
one in the queue.

Modify the merge check to take account of the fact that all readahead
bios will be treated as fastfail.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 block/elevator.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

Comments

Stefan Bader Oct. 13, 2009, 12:39 p.m.
Having spent quite some time listening to that problem, I believe it is sane. ;-)

Andy Whitcroft wrote:
> BugLink: http://bugs.launchpad.net/bugs/444915
> 
> When a readahead request is injected into the elevator we construct a
> bio as below, treating it as fastfail:
> 
>     void init_request_from_bio(struct request *req, struct bio *bio)
>     [...]
>         if (bio_rw_ahead(bio))
>                 req->cmd_flags |= (REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>                                    REQ_FAILFAST_DRIVER);
>     [...]
> 
> However when merging requests in the elevator we only allow merges of
> new bios with matching fastfail attributes:
> 
>     int elv_rq_merge_ok(struct request *rq, struct bio *bio)
>     [...]
>         if (!bio_failfast_dev(bio)       != !blk_failfast_dev(rq) ||
>             !bio_failfast_transport(bio) != !blk_failfast_transport(rq) ||
>             !bio_failfast_driver(bio)    != !blk_failfast_driver(rq))
>     [...]
> 
> This check occurs against the original bio fastfail bits ignoring the
> override and thus preventing merge of an existing readahead request with
> one in the queue.
> 
> Modify the merge check to take account of the fact that all readahead
> bios will be treated as fastfail.
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>

Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  block/elevator.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index 2d511f9..0353d80 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -101,16 +101,22 @@ int elv_rq_merge_ok(struct request *rq, struct bio *bio)
>  		return 0;
>  
>  	/*
> -	 * Don't merge if failfast settings don't match.
> +	 * Don't merge if failfast settings don't match.  Note readahead
> +	 * requests are always mapped to failfast.
>  	 *
>  	 * FIXME: The negation in front of each condition is necessary
>  	 * because bio and request flags use different bit positions
>  	 * and the accessors return those bits directly.  This
>  	 * ugliness will soon go away.
>  	 */
> -	if (!bio_failfast_dev(bio)	 != !blk_failfast_dev(rq)	||
> -	    !bio_failfast_transport(bio) != !blk_failfast_transport(rq)	||
> -	    !bio_failfast_driver(bio)	 != !blk_failfast_driver(rq))
> +	if (!(bio_rw_ahead(bio) || bio_failfast_dev(bio)) !=
> +						!blk_failfast_dev(rq))
> +		return 0;
> +	if (!(bio_rw_ahead(bio) || bio_failfast_transport(bio)) !=
> +						!blk_failfast_transport(rq))
> +		return 0;
> +	if (!(bio_rw_ahead(bio) || bio_failfast_driver(bio)) !=
> +						!blk_failfast_driver(rq))
>  		return 0;
>  
>  	if (!elv_iosched_allow_merge(rq, bio))

Patch hide | download patch | download mbox

diff --git a/block/elevator.c b/block/elevator.c
index 2d511f9..0353d80 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -101,16 +101,22 @@  int elv_rq_merge_ok(struct request *rq, struct bio *bio)
 		return 0;
 
 	/*
-	 * Don't merge if failfast settings don't match.
+	 * Don't merge if failfast settings don't match.  Note readahead
+	 * requests are always mapped to failfast.
 	 *
 	 * FIXME: The negation in front of each condition is necessary
 	 * because bio and request flags use different bit positions
 	 * and the accessors return those bits directly.  This
 	 * ugliness will soon go away.
 	 */
-	if (!bio_failfast_dev(bio)	 != !blk_failfast_dev(rq)	||
-	    !bio_failfast_transport(bio) != !blk_failfast_transport(rq)	||
-	    !bio_failfast_driver(bio)	 != !blk_failfast_driver(rq))
+	if (!(bio_rw_ahead(bio) || bio_failfast_dev(bio)) !=
+						!blk_failfast_dev(rq))
+		return 0;
+	if (!(bio_rw_ahead(bio) || bio_failfast_transport(bio)) !=
+						!blk_failfast_transport(rq))
+		return 0;
+	if (!(bio_rw_ahead(bio) || bio_failfast_driver(bio)) !=
+						!blk_failfast_driver(rq))
 		return 0;
 
 	if (!elv_iosched_allow_merge(rq, bio))