Re: [PATCH] block: streamline merge possibility checks

[Bob Liu <bob.liu@oracle.com>]

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;
>  }
>  
>