[PATCH] block: streamline merge possibility checks

[PATCH] block: streamline merge possibility checks

[Dmitry Fomichev]

Checks for data direction in attempt_merge() and blk_rq_merge_ok()
are redundant and will always succeed when the both I/O request
operations are equal. Therefore, remove them.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block/blk-merge.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index d783bdc4559b..796451aed7d6 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -745,8 +745,7 @@ static struct request *attempt_merge(struct request_queue *q,
 	if (req_op(req) != req_op(next))
 		return NULL;
 
-	if (rq_data_dir(req) != rq_data_dir(next)
-	    || req->rq_disk != next->rq_disk)
+	if (req->rq_disk != next->rq_disk)
 		return NULL;
 
 	if (req_op(req) == REQ_OP_WRITE_SAME &&
@@ -868,10 +867,6 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 	if (req_op(rq) != bio_op(bio))
 		return false;
 
-	/* different data direction or already started, don't merge */
-	if (bio_data_dir(bio) != rq_data_dir(rq))
-		return false;
-
 	/* must be same device */
 	if (rq->rq_disk != bio->bi_disk)
 		return false;
-- 
2.21.0

Re: [PATCH] block: streamline merge possibility checks

[Bob Liu]

On 12/19/19 3:41 AM, Dmitry Fomichev wrote:
> Checks for data direction in attempt_merge() and blk_rq_merge_ok()
> are redundant and will always succeed when the both I/O request
> operations are equal. Therefore, remove them.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  block/blk-merge.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index d783bdc4559b..796451aed7d6 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -745,8 +745,7 @@ static struct request *attempt_merge(struct request_queue *q,
>  	if (req_op(req) != req_op(next))
>  		return NULL;
>  
> -	if (rq_data_dir(req) != rq_data_dir(next)
> -	    || req->rq_disk != next->rq_disk)
> +	if (req->rq_disk != next->rq_disk)
>  		return NULL;
>  
>  	if (req_op(req) == REQ_OP_WRITE_SAME &&
> @@ -868,10 +867,6 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
>  	if (req_op(rq) != bio_op(bio))
>  		return false;
>  
> -	/* different data direction or already started, don't merge */
> -	if (bio_data_dir(bio) != rq_data_dir(rq))
> -		return false;
> -

Reviewed-by: Bob Liu <bob.liu@oracle.com>

>  	/* must be same device */
>  	if (rq->rq_disk != bio->bi_disk)
>  		return false;
> 

Re: [PATCH] block: streamline merge possibility checks

[Damien Le Moal]

On 2019/12/19 4:42, Dmitry Fomichev wrote:
> Checks for data direction in attempt_merge() and blk_rq_merge_ok()
> are redundant and will always succeed when the both I/O request
> operations are equal. Therefore, remove them.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  block/blk-merge.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index d783bdc4559b..796451aed7d6 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -745,8 +745,7 @@ static struct request *attempt_merge(struct request_queue *q,
>  	if (req_op(req) != req_op(next))
>  		return NULL;
>  
> -	if (rq_data_dir(req) != rq_data_dir(next)
> -	    || req->rq_disk != next->rq_disk)
> +	if (req->rq_disk != next->rq_disk)
>  		return NULL;
>  
>  	if (req_op(req) == REQ_OP_WRITE_SAME &&
> @@ -868,10 +867,6 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
>  	if (req_op(rq) != bio_op(bio))
>  		return false;
>  
> -	/* different data direction or already started, don't merge */
> -	if (bio_data_dir(bio) != rq_data_dir(rq))
> -		return false;
> -
>  	/* must be same device */
>  	if (rq->rq_disk != bio->bi_disk)
>  		return false;
> 

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: streamline merge possibility checks

[Bob Liu]

On 12/19/19 3:41 AM, Dmitry Fomichev wrote:
> Checks for data direction in attempt_merge() and blk_rq_merge_ok()

Speak about these two functions, do you think attempt_merge() can be built on blk_rq_merge_ok()?
Things like..
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 48e6725..2a00c4c 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -724,28 +724,7 @@ static enum elv_merge blk_try_req_merge(struct request *req,
 static struct request *attempt_merge(struct request_queue *q,
                                     struct request *req, struct request *next)
 {
-       if (!rq_mergeable(req) || !rq_mergeable(next))
-               return NULL;
-
-       if (req_op(req) != req_op(next))
-               return NULL;
-
-       if (rq_data_dir(req) != rq_data_dir(next)
-           || req->rq_disk != next->rq_disk)
-               return NULL;
-
-       if (req_op(req) == REQ_OP_WRITE_SAME &&
-           !blk_write_same_mergeable(req->bio, next->bio))
-               return NULL;
-
-       /*
-        * Don't allow merge of different write hints, or for a hint with
-        * non-hint IO.
-        */
-       if (req->write_hint != next->write_hint)
-               return NULL;
-
-       if (req->ioprio != next->ioprio)
+       if (!blk_rq_merge_ok(req, next->bio))
                return NULL;


> are redundant and will always succeed when the both I/O request
> operations are equal. Therefore, remove them.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  block/blk-merge.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index d783bdc4559b..796451aed7d6 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -745,8 +745,7 @@ static struct request *attempt_merge(struct request_queue *q,
>  	if (req_op(req) != req_op(next))
>  		return NULL;
>  
> -	if (rq_data_dir(req) != rq_data_dir(next)
> -	    || req->rq_disk != next->rq_disk)
> +	if (req->rq_disk != next->rq_disk)
>  		return NULL;
>  
>  	if (req_op(req) == REQ_OP_WRITE_SAME &&
> @@ -868,10 +867,6 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
>  	if (req_op(rq) != bio_op(bio))
>  		return false;
>  
> -	/* different data direction or already started, don't merge */
> -	if (bio_data_dir(bio) != rq_data_dir(rq))
> -		return false;
> -
>  	/* must be same device */
>  	if (rq->rq_disk != bio->bi_disk)
>  		return false;
> 

Re: [PATCH] block: streamline merge possibility checks

[Christoph Hellwig]

On Wed, Dec 18, 2019 at 02:41:56PM -0500, Dmitry Fomichev wrote:
> Checks for data direction in attempt_merge() and blk_rq_merge_ok()
> are redundant and will always succeed when the both I/O request
> operations are equal. Therefore, remove them.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: streamline merge possibility checks

[Christoph Hellwig]

On Fri, Dec 20, 2019 at 02:50:05PM +0800, Bob Liu wrote:
> On 12/19/19 3:41 AM, Dmitry Fomichev wrote:
> > Checks for data direction in attempt_merge() and blk_rq_merge_ok()
> 
> Speak about these two functions, do you think attempt_merge() can be built on blk_rq_merge_ok()?
> Things like..
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 48e6725..2a00c4c 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -724,28 +724,7 @@ static enum elv_merge blk_try_req_merge(struct request *req,
>  static struct request *attempt_merge(struct request_queue *q,
>                                      struct request *req, struct request *next)
>  {
> -       if (!rq_mergeable(req) || !rq_mergeable(next))
> -               return NULL;
> -
> -       if (req_op(req) != req_op(next))
> -               return NULL;
> -
> -       if (rq_data_dir(req) != rq_data_dir(next)
> -           || req->rq_disk != next->rq_disk)
> -               return NULL;
> -
> -       if (req_op(req) == REQ_OP_WRITE_SAME &&
> -           !blk_write_same_mergeable(req->bio, next->bio))
> -               return NULL;
> -
> -       /*
> -        * Don't allow merge of different write hints, or for a hint with
> -        * non-hint IO.
> -        */
> -       if (req->write_hint != next->write_hint)
> -               return NULL;
> -
> -       if (req->ioprio != next->ioprio)
> +       if (!blk_rq_merge_ok(req, next->bio))
>                 return NULL;

This looks sensible, but we might have to be a bit more careful.
rq_mergeable checks for RQF_NOMERGE_FLAGS and various ops, while
bio_mergeable is missing those.  So I think you need to go through
carefully if we need to keep any extra checks, but otherwise using
blk_rq_merge_ok looks sensible.

RE: [PATCH] block: streamline merge possibility checks

[Dmitry Fomichev]

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Wednesday, January 8, 2020 8:45 AM
> To: Bob Liu <bob.liu@oracle.com>
> Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; linux-
> block@vger.kernel.org; Jens Axboe <axboe@kernel.dk>; Damien Le Moal
> <Damien.LeMoal@wdc.com>
> Subject: Re: [PATCH] block: streamline merge possibility checks
> 
> On Fri, Dec 20, 2019 at 02:50:05PM +0800, Bob Liu wrote:
> > On 12/19/19 3:41 AM, Dmitry Fomichev wrote:
> > > Checks for data direction in attempt_merge() and blk_rq_merge_ok()
> >
> > Speak about these two functions, do you think attempt_merge() can be
> built on blk_rq_merge_ok()?
> > Things like..
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index 48e6725..2a00c4c 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -724,28 +724,7 @@ static enum elv_merge blk_try_req_merge(struct
> request *req,
> >  static struct request *attempt_merge(struct request_queue *q,
> >                                      struct request *req, struct request *next)
> >  {
> > -       if (!rq_mergeable(req) || !rq_mergeable(next))
> > -               return NULL;
> > -
> > -       if (req_op(req) != req_op(next))
> > -               return NULL;
> > -
> > -       if (rq_data_dir(req) != rq_data_dir(next)
> > -           || req->rq_disk != next->rq_disk)
> > -               return NULL;
> > -
> > -       if (req_op(req) == REQ_OP_WRITE_SAME &&
> > -           !blk_write_same_mergeable(req->bio, next->bio))
> > -               return NULL;
> > -
> > -       /*
> > -        * Don't allow merge of different write hints, or for a hint with
> > -        * non-hint IO.
> > -        */
> > -       if (req->write_hint != next->write_hint)
> > -               return NULL;
> > -
> > -       if (req->ioprio != next->ioprio)
> > +       if (!blk_rq_merge_ok(req, next->bio))
> >                 return NULL;
> 
> This looks sensible, but we might have to be a bit more careful.
> rq_mergeable checks for RQF_NOMERGE_FLAGS and various ops, while
> bio_mergeable is missing those.  So I think you need to go through
> carefully if we need to keep any extra checks, but otherwise using
> blk_rq_merge_ok looks sensible.

I tried this patch as is and, indeed, it leads to blktests failures and filesystem
errors, apparently because of the RQF_NOMERGE_FLAGS  difference.
However, the patch below seems to work - I've been running my host system
with it for a couple of days with no issues. This one is added on top of
"block: streamline merge possibility checks" patch.

From: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Date: Wed, 8 Jan 2020 14:24:06 -0500
Subject: [PATCH] block: simplify merge checks

The code parts to decide on merge possibility in attempt_merge() and
blk_rq_merge_ok() look very similar. It is possible to move these
checks to a common inline helper function.

Suggested-by: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block/blk-merge.c | 56 +++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index f68d67b367d6..49052a53051f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -732,6 +732,36 @@ static enum elv_merge blk_try_req_merge(struct request *req,
 	return ELEVATOR_NO_MERGE;
 }
 
+static inline bool blk_rq_mergeable(struct request *rq, struct bio *bio)
+{
+	if (!rq_mergeable(rq))
+		return false;
+
+	if (req_op(rq) != bio_op(bio))
+		return false;
+
+	/* must be same device */
+	if (rq->rq_disk != bio->bi_disk)
+		return false;
+
+	/* must be using the same buffer */
+	if (req_op(rq) == REQ_OP_WRITE_SAME &&
+	    !blk_write_same_mergeable(rq->bio, bio))
+		return false;
+
+	/*
+	 * Don't allow merge of different write hints, or for a hint with
+	 * non-hint IO.
+	 */
+	if (rq->write_hint != bio->bi_write_hint)
+		return false;
+
+	if (rq->ioprio != bio_prio(bio))
+		return false;
+
+	return true;
+}
+
 /*
  * For non-mq, this has to be called with the request spinlock acquired.
  * For mq with scheduling, the appropriate queue wide lock should be held.
@@ -739,7 +769,7 @@ static enum elv_merge blk_try_req_merge(struct request *req,
 static struct request *attempt_merge(struct request_queue *q,
 				     struct request *req, struct request *next)
 {
-	if (!blk_rq_merge_ok(req, next->bio))
+	if (!rq_mergeable(next) || !blk_rq_mergeable(req, next->bio))
 		return NULL;
 
 	/*
@@ -841,35 +871,13 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq,
 
 bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 {
-	if (!rq_mergeable(rq) || !bio_mergeable(bio))
-		return false;
-
-	if (req_op(rq) != bio_op(bio))
-		return false;
-
-	/* must be same device */
-	if (rq->rq_disk != bio->bi_disk)
+	if (!bio_mergeable(bio) || !blk_rq_mergeable(rq, bio))
 		return false;
 
 	/* only merge integrity protected bio into ditto rq */
 	if (blk_integrity_merge_bio(rq->q, rq, bio) == false)
 		return false;
 
-	/* must be using the same buffer */
-	if (req_op(rq) == REQ_OP_WRITE_SAME &&
-	    !blk_write_same_mergeable(rq->bio, bio))
-		return false;
-
-	/*
-	 * Don't allow merge of different write hints, or for a hint with
-	 * non-hint IO.
-	 */
-	if (rq->write_hint != bio->bi_write_hint)
-		return false;
-
-	if (rq->ioprio != bio_prio(bio))
-		return false;
-
 	return true;
 }
 
-- 
2.21.0

Re: [PATCH] block: streamline merge possibility checks

[Bob Liu]

On 1/9/20 6:02 AM, Dmitry Fomichev wrote:
>> -----Original Message-----
>> From: Christoph Hellwig <hch@infradead.org>
>> Sent: Wednesday, January 8, 2020 8:45 AM
>> To: Bob Liu <bob.liu@oracle.com>
>> Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; linux-
>> block@vger.kernel.org; Jens Axboe <axboe@kernel.dk>; Damien Le Moal
>> <Damien.LeMoal@wdc.com>
>> Subject: Re: [PATCH] block: streamline merge possibility checks
>>
>> On Fri, Dec 20, 2019 at 02:50:05PM +0800, Bob Liu wrote:
>>> On 12/19/19 3:41 AM, Dmitry Fomichev wrote:
>>>> Checks for data direction in attempt_merge() and blk_rq_merge_ok()
>>>
>>> Speak about these two functions, do you think attempt_merge() can be
>> built on blk_rq_merge_ok()?
>>> Things like..
>>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>>> index 48e6725..2a00c4c 100644
>>> --- a/block/blk-merge.c
>>> +++ b/block/blk-merge.c
>>> @@ -724,28 +724,7 @@ static enum elv_merge blk_try_req_merge(struct
>> request *req,
>>>  static struct request *attempt_merge(struct request_queue *q,
>>>                                      struct request *req, struct request *next)
>>>  {
>>> -       if (!rq_mergeable(req) || !rq_mergeable(next))
>>> -               return NULL;
>>> -
>>> -       if (req_op(req) != req_op(next))
>>> -               return NULL;
>>> -
>>> -       if (rq_data_dir(req) != rq_data_dir(next)
>>> -           || req->rq_disk != next->rq_disk)
>>> -               return NULL;
>>> -
>>> -       if (req_op(req) == REQ_OP_WRITE_SAME &&
>>> -           !blk_write_same_mergeable(req->bio, next->bio))
>>> -               return NULL;
>>> -
>>> -       /*
>>> -        * Don't allow merge of different write hints, or for a hint with
>>> -        * non-hint IO.
>>> -        */
>>> -       if (req->write_hint != next->write_hint)
>>> -               return NULL;
>>> -
>>> -       if (req->ioprio != next->ioprio)
>>> +       if (!blk_rq_merge_ok(req, next->bio))
>>>                 return NULL;
>>
>> This looks sensible, but we might have to be a bit more careful.
>> rq_mergeable checks for RQF_NOMERGE_FLAGS and various ops, while
>> bio_mergeable is missing those.  So I think you need to go through
>> carefully if we need to keep any extra checks, but otherwise using
>> blk_rq_merge_ok looks sensible.
> 
> I tried this patch as is and, indeed, it leads to blktests failures and filesystem
> errors, apparently because of the RQF_NOMERGE_FLAGS  difference.
> However, the patch below seems to work - I've been running my host system
> with it for a couple of days with no issues. This one is added on top of
> "block: streamline merge possibility checks" patch.
> 
> From: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> Date: Wed, 8 Jan 2020 14:24:06 -0500
> Subject: [PATCH] block: simplify merge checks
> 
> The code parts to decide on merge possibility in attempt_merge() and
> blk_rq_merge_ok() look very similar. It is possible to move these
> checks to a common inline helper function.
> 
> Suggested-by: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  block/blk-merge.c | 56 +++++++++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index f68d67b367d6..49052a53051f 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -732,6 +732,36 @@ static enum elv_merge blk_try_req_merge(struct request *req,
>  	return ELEVATOR_NO_MERGE;
>  }
>  
> +static inline bool blk_rq_mergeable(struct request *rq, struct bio *bio)
> +{
> +	if (!rq_mergeable(rq))
> +		return false;
> +
> +	if (req_op(rq) != bio_op(bio))
> +		return false;
> +
> +	/* must be same device */
> +	if (rq->rq_disk != bio->bi_disk)
> +		return false;
> +
> +	/* must be using the same buffer */
> +	if (req_op(rq) == REQ_OP_WRITE_SAME &&
> +	    !blk_write_same_mergeable(rq->bio, bio))
> +		return false;
> +
> +	/*
> +	 * Don't allow merge of different write hints, or for a hint with
> +	 * non-hint IO.
> +	 */
> +	if (rq->write_hint != bio->bi_write_hint)
> +		return false;
> +
> +	if (rq->ioprio != bio_prio(bio))
> +		return false;
> +
> +	return true;
> +}
> +
>  /*
>   * For non-mq, this has to be called with the request spinlock acquired.
>   * For mq with scheduling, the appropriate queue wide lock should be held.
> @@ -739,7 +769,7 @@ static enum elv_merge blk_try_req_merge(struct request *req,
>  static struct request *attempt_merge(struct request_queue *q,
>  				     struct request *req, struct request *next)
>  {
> -	if (!blk_rq_merge_ok(req, next->bio))
> +	if (!rq_mergeable(next) || !blk_rq_mergeable(req, next->bio))
>  		return NULL;
>  
>  	/*
> @@ -841,35 +871,13 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq,
>  
>  bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
>  {
> -	if (!rq_mergeable(rq) || !bio_mergeable(bio))
> -		return false;
> -
> -	if (req_op(rq) != bio_op(bio))
> -		return false;
> -
> -	/* must be same device */
> -	if (rq->rq_disk != bio->bi_disk)
> +	if (!bio_mergeable(bio) || !blk_rq_mergeable(rq, bio))
>  		return false;
>  

Nitpick, I think bio_mergeable(bio) can also put inside blk_rq_mergeable().
Anyway, looks fine to me, thanks!
Reviewed-by: Bob Liu <bob.liu@oracle.com>

>  	/* only merge integrity protected bio into ditto rq */
>  	if (blk_integrity_merge_bio(rq->q, rq, bio) == false)
>  		return false;
>  
> -	/* must be using the same buffer */
> -	if (req_op(rq) == REQ_OP_WRITE_SAME &&
> -	    !blk_write_same_mergeable(rq->bio, bio))
> -		return false;
> -
> -	/*
> -	 * Don't allow merge of different write hints, or for a hint with
> -	 * non-hint IO.
> -	 */
> -	if (rq->write_hint != bio->bi_write_hint)
> -		return false;
> -
> -	if (rq->ioprio != bio_prio(bio))
> -		return false;
> -
>  	return true;
>  }
>  
>