clean up bio merge conditions

clean up bio merge conditions

[Christoph Hellwig]

Hi Jens,

Dan's smatch run pointed out that there is no need to check for a NULL
req->bio in the merge bio into request and request merge helpers
because they can't ever be reached by flush or passthrough requests.

While validing that I also found a few other odd bits directly next
to it, so this two-patch series fixes all of that.

Diffstat:
 blk-merge.c |   35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

[PATCH 1/2] block: don't bother checking the data direction for merges

[Christoph Hellwig]

Because it already is encoded in the opcode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index e0b28e9298c9..64860cbd5e27 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -864,9 +864,6 @@ 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))
-		return NULL;
-
 	if (req->bio && next->bio) {
 		/* Don't merge requests with different write hints. */
 		if (req->bio->bi_write_hint != next->bio->bi_write_hint)
@@ -986,10 +983,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;
-
 	/* don't merge across cgroup boundaries */
 	if (!blk_cgroup_mergeable(rq, bio))
 		return false;
-- 
2.45.2

Re: [PATCH 1/2] block: don't bother checking the data direction for merges

[John Garry]

On 19/11/2024 16:11, Christoph Hellwig wrote:
> Because it already is encoded in the opcode.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-merge.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index e0b28e9298c9..64860cbd5e27 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -864,9 +864,6 @@ 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))
> -		return NULL;
> -
>   	if (req->bio && next->bio) {
>   		/* Don't merge requests with different write hints. */
>   		if (req->bio->bi_write_hint != next->bio->bi_write_hint)
> @@ -986,10 +983,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 */

I tried to check what is meant by "already started", but that comment 
pre-dates git. And even the code from then does not make it obvious, but 
I don't want to check further, so:

Reviewed-by: John Garry <john.g.garry@oracle.com>


> -	if (bio_data_dir(bio) != rq_data_dir(rq))
> -		return false;
> -
>   	/* don't merge across cgroup boundaries */
>   	if (!blk_cgroup_mergeable(rq, bio))
>   		return false;

Re: [PATCH 1/2] block: don't bother checking the data direction for merges

[Christoph Hellwig]

On Tue, Nov 19, 2024 at 04:48:15PM +0000, John Garry wrote:
> I tried to check what is meant by "already started", but that comment 
> pre-dates git. And even the code from then does not make it obvious, but I 
> don't want to check further, so:

Back in the bad old days there was a separare prep_fn callback into
the driver to prepare a request while it was still on the scheduler
lists, and that then set a started flag.   In other words the comment
is long obsolete.

[PATCH 2/2] block: req->bio is always set in the merge code

[Christoph Hellwig]

As smatch, which is a lot smarter than me noticed.  So remove the checks
for it, and condense these checks a bit including the comments stating
the obvious.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 64860cbd5e27..e01383c6e534 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -864,14 +864,10 @@ static struct request *attempt_merge(struct request_queue *q,
 	if (req_op(req) != req_op(next))
 		return NULL;
 
-	if (req->bio && next->bio) {
-		/* Don't merge requests with different write hints. */
-		if (req->bio->bi_write_hint != next->bio->bi_write_hint)
-			return NULL;
-		if (req->bio->bi_ioprio != next->bio->bi_ioprio)
-			return NULL;
-	}
-
+	if (req->bio->bi_write_hint != next->bio->bi_write_hint)
+		return NULL;
+	if (req->bio->bi_ioprio != next->bio->bi_ioprio)
+		return NULL;
 	if (!blk_atomic_write_mergeable_rqs(req, next))
 		return NULL;
 
@@ -983,26 +979,16 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 	if (req_op(rq) != bio_op(bio))
 		return false;
 
-	/* don't merge across cgroup boundaries */
 	if (!blk_cgroup_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;
-
-	/* Only merge if the crypt contexts are compatible */
 	if (!bio_crypt_rq_ctx_compatible(rq, bio))
 		return false;
-
-	if (rq->bio) {
-		/* Don't merge requests with different write hints. */
-		if (rq->bio->bi_write_hint != bio->bi_write_hint)
-			return false;
-		if (rq->bio->bi_ioprio != bio->bi_ioprio)
-			return false;
-	}
-
+	if (rq->bio->bi_write_hint != bio->bi_write_hint)
+		return false;
+	if (rq->bio->bi_ioprio != bio->bi_ioprio)
+		return false;
 	if (blk_atomic_write_mergeable_rq_bio(rq, bio) == false)
 		return false;
 
-- 
2.45.2

Re: [PATCH 2/2] block: req->bio is always set in the merge code

[John Garry]

On 19/11/2024 16:11, Christoph Hellwig wrote:
> As smatch, which is a lot smarter than me noticed.  So remove the checks
> for it, and condense these checks a bit including the comments stating
> the obvious.
> 
> Reported-by: Dan Carpenter<dan.carpenter@linaro.org>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> ---

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: clean up bio merge conditions

[Martin K. Petersen]

Christoph,

> Dan's smatch run pointed out that there is no need to check for a NULL
> req->bio in the merge bio into request and request merge helpers
> because they can't ever be reached by flush or passthrough requests.
>
> While validing that I also found a few other odd bits directly next
> to it, so this two-patch series fixes all of that.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: clean up bio merge conditions

[Johannes Thumshirn]

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: clean up bio merge conditions

[Jens Axboe]

On Tue, 19 Nov 2024 17:11:49 +0100, Christoph Hellwig wrote:
> Dan's smatch run pointed out that there is no need to check for a NULL
> req->bio in the merge bio into request and request merge helpers
> because they can't ever be reached by flush or passthrough requests.
> 
> While validing that I also found a few other odd bits directly next
> to it, so this two-patch series fixes all of that.
> 
> [...]

Applied, thanks!

[1/2] block: don't bother checking the data direction for merges
      commit: 9f8d68283342a48f692f2c02231318bb4a7b207f
[2/2] block: req->bio is always set in the merge code
      commit: 81314bfbde9d089fa2318adba54891dfaadb1c05

Best regards,
-- 
Jens Axboe