On 1/9/20 6:02 AM, Dmitry Fomichev wrote:
>> -----Original Message-----
>> From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>> Sent: Wednesday, January 8, 2020 8:45 AM
>> To: Bob Liu <bob.liu@xxxxxxxxxx>
>> Cc: Dmitry Fomichev <Dmitry.Fomichev@xxxxxxx>; linux-
>> block@xxxxxxxxxxxxxxx; Jens Axboe <axboe@xxxxxxxxx>; Damien Le Moal
>> <Damien.LeMoal@xxxxxxx>
>> 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@xxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
> /* 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;
> }
>
>
