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

login
register
mail settings
Submitter Andy Whitcroft
Date Oct. 13, 2009, 11:18 a.m.
Message ID <1255432719-19615-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/35843/
State Accepted
Commit 6830be77b871fa5220a52f733f644de5b2b1a670
Headers show

Comments

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(-)
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

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))