linux-kernel - Re: [PATCH] Revert "block: don't reorder requests in blk_add_rq_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f9976d55-f418-45b8-82ac-e0557e713b4c@amazon.com>
Date: Wed, 11 Jun 2025 16:15:26 +0100
From: "Mohamed Abuelfotoh, Hazem" <abuehaze@...zon.com>
To: Jens Axboe <axboe@...nel.dk>
CC: <stable@...r.kernel.org>, kernel test robot <oliver.sang@...el.com>, Hagar
 Hemdan <hagarhem@...zon.com>, Shaoying Xu <shaoyi@...zon.com>, "Michael S.
 Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>, Paolo Bonzini
	<pbonzini@...hat.com>, Stefan Hajnoczi <stefanha@...hat.com>,
	Eugenio Pérez <eperezma@...hat.com>, Xuan Zhuo
	<xuanzhuo@...ux.alibaba.com>, Keith Busch <kbusch@...nel.org>, "Christoph
 Hellwig" <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
	<linux-block@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<virtualization@...ts.linux.dev>, <linux-nvme@...ts.infradead.org>
Subject: Re: [PATCH] Revert "block: don't reorder requests in blk_add_rq_to_plug"

On 11/06/2025 13:56, 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.
> 
> 
> 
> On 6/11/25 6:14 AM, Hazem Mohamed Abuelfotoh wrote:
>> This reverts commit e70c301faece15b618e54b613b1fd6ece3dd05b4.
>>
>> Commit <e70c301faece> ("block: don't reorder requests in
>> blk_add_rq_to_plug") reversed how requests are stored in the blk_plug
>> list, this had significant impact on bio merging with requests exist on
>> the plug list. This impact has been reported in [1] and could easily be
>> reproducible using 4k randwrite fio benchmark on an NVME based SSD without
>> having any filesystem on the disk.
> 
> Rather than revert this commit, why not just attempt a tail merge?
> Something ala this, totally untested.
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 3af1d284add5..708ded67d52a 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -998,6 +998,10 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
>          if (!plug || rq_list_empty(&plug->mq_list))
>                  return false;
> 
> +       rq = plug->mq_list.tail;
> +       if (rq->q == q)
> +               return blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == BIO_MERGE_OK;
> +
>          rq_list_for_each(&plug->mq_list, rq) {
>                  if (rq->q == q) {
>                          if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) ==
> 
> --
> Jens Axboe

I thought about that solution before submitting the revert and I believe 
it will help with the case we are discussing here  but what about the 
case where we have raid disks for which we need to iterate the plug 
list? In this case we will iterate the plug list from head to tail while 
the most recent requests (where merging will likely happen) will be 
closer to that tail so there will be additional overhead to iterate 
through the whole plug list and I believe the revert will also be better 
for this specific case unless I am missing something here :)