> -----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;
/* 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
