On 6/11/25 10:55 AM, Mohamed Abuelfotoh, Hazem wrote: > On 11/06/2025 15:53, Jens Axboe wrote: >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. >> >> >> >> Previously, the block layer stored the requests in the plug list in >> LIFO order. For this reason, blk_attempt_plug_merge() would check >> just the head entry for a back merge attempt, and abort after that >> unless requests for multiple queues existed in the plug list. If more >> than one request is present in the plug list, this makes the one-shot >> back merging less useful than before, as it'll always fail to find a >> quick merge candidate. >> >> Use the tail entry for the one-shot merge attempt, which is the last >> added request in the list. If that fails, abort immediately unless >> there are multiple queues available. If multiple queues are available, >> then scan the list. Ideally the latter scan would be a backwards scan >> of the list, but as it currently stands, the plug list is singly linked >> and hence this isn't easily feasible. >> >> Cc: stable@xxxxxxxxxxxxxxx >> Link: https://lore.kernel.org/linux-block/20250611121626.7252-1-abuehaze@xxxxxxxxxx/ >> Reported-by: Hazem Mohamed Abuelfotoh <abuehaze@xxxxxxxxxx> >> Fixes: e70c301faece ("block: don't reorder requests in blk_add_rq_to_plug") >> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx> >> >> --- >> >> diff --git a/block/blk-merge.c b/block/blk-merge.c >> index 3af1d284add5..70d704615be5 100644 >> --- a/block/blk-merge.c >> +++ b/block/blk-merge.c >> @@ -998,20 +998,20 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, >> if (!plug || rq_list_empty(&plug->mq_list)) >> return false; >> >> - rq_list_for_each(&plug->mq_list, rq) { >> - if (rq->q == q) { >> - if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == >> - BIO_MERGE_OK) >> - return true; >> - break; >> - } >> + rq = plug->mq_list.tail; >> + if (rq->q == q) >> + return blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == >> + BIO_MERGE_OK; >> + else if (!plug->multiple_queues) >> + return false; >> >> - /* >> - * Only keep iterating plug list for merges if we have multiple >> - * queues >> - */ >> - if (!plug->multiple_queues) >> - break; >> + rq_list_for_each(&plug->mq_list, rq) { >> + if (rq->q != q) >> + continue; >> + if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == >> + BIO_MERGE_OK) >> + return true; >> + break; >> } >> return false; >> } >> >> -- >> Jens Axboe >> > > Thanks for posting this fix, I can confirm that this patch mitigated > the regression reported in [1], my only concern is the impact when we > have multiple queues available as you explained in the commit message. > Given that reverting e70c301faece ("block: don't reorder requests in > blk_add_rq_to_plug") will break zoned storage support and the plug > list is singly linked, I don't think we have a better solution here. Yes we can't revert it, and honestly I would not want to even if that was an option. If the multi-queue case is particularly important, you could just do something ala the below - keep scanning until you a merge _could_ have happened but didn't. Ideally we'd want to iterate the plug list backwards and then we could keep the same single shot logic, where you only attempt one request that has a matching queue. And obviously we could just doubly link the requests, there's space in the request linkage code to do that. But that'd add overhead in general, I think it's better to shove a bit of that overhead to the multi-queue case. I suspect the below would do the trick, however. diff --git a/block/blk-merge.c b/block/blk-merge.c index 70d704615be5..4313301f131c 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -1008,6 +1008,8 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, rq_list_for_each(&plug->mq_list, rq) { if (rq->q != q) continue; + if (blk_try_merge(rq, bio) == ELEVATOR_NO_MERGE) + continue; if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == BIO_MERGE_OK) return true; -- Jens Axboe
