get drivers out of setting queue flags

get drivers out of setting queue flags

[Christoph Hellwig]

Hi all,

now that driver features have been moved out of the queue flags,
the abuses where drivers set random internal queue flags stand out
even more.  This series fixes them up.

Diffstat:
 block/loop.c                      |   15 ++-------------
 block/rnbd/rnbd-clt.c             |    2 --
 scsi/megaraid/megaraid_sas_base.c |    2 --
 scsi/mpt3sas/mpt3sas_scsih.c      |    6 ------
 4 files changed, 2 insertions(+), 23 deletions(-)

[PATCH 1/5] loop: don't set QUEUE_FLAG_NOMERGES

[Christoph Hellwig]

QUEUE_FLAG_NOMERGES isn't really a driver interface, but a user tunable.
There also isn't any good reason to set it in the loop driver.

The original commit adding it (5b5e20f421c0b6d "block: loop: set
QUEUE_FLAG_NOMERGES for request queue of loop") claims that "It doesn't
make sense to enable merge because the I/O submitted to backing file is
handled page by page."  which of course isn't true for multi-page bvec
now, and it never has been for direct I/O, for which commit 40326d8a33d
("block/loop: allow request merge for directio mode") alredy disabled
the nomerges flag.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 86b5d956dc4e02..3a116314877109 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -211,13 +211,10 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
 	if (lo->lo_state == Lo_bound)
 		blk_mq_freeze_queue(lo->lo_queue);
 	lo->use_dio = use_dio;
-	if (use_dio) {
-		blk_queue_flag_clear(QUEUE_FLAG_NOMERGES, lo->lo_queue);
+	if (use_dio)
 		lo->lo_flags |= LO_FLAGS_DIRECT_IO;
-	} else {
-		blk_queue_flag_set(QUEUE_FLAG_NOMERGES, lo->lo_queue);
+	else
 		lo->lo_flags &= ~LO_FLAGS_DIRECT_IO;
-	}
 	if (lo->lo_state == Lo_bound)
 		blk_mq_unfreeze_queue(lo->lo_queue);
 }
@@ -2033,14 +2030,6 @@ static int loop_add(int i)
 	}
 	lo->lo_queue = lo->lo_disk->queue;
 
-	/*
-	 * By default, we do buffer IO, so it doesn't make sense to enable
-	 * merge because the I/O submitted to backing file is handled page by
-	 * page. For directio mode, merge does help to dispatch bigger request
-	 * to underlayer disk. We will enable merge once directio is enabled.
-	 */
-	blk_queue_flag_set(QUEUE_FLAG_NOMERGES, lo->lo_queue);
-
 	/*
 	 * Disable partition scanning by default. The in-kernel partition
 	 * scanning can be requested individually per-device during its
-- 
2.43.0

[PATCH 2/5] megaraid_sas: don't set QUEUE_FLAG_NOMERGES

[Christoph Hellwig]

Setting QUEUE_FLAG_NOMERGES was added in commit 15dd03811d99dcf
("scsi: megaraid_sas: NVME Interface detection and prop settings")
without any explanation.  Drivers should second guess the block
layer merge decisions, so remove the flag.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/megaraid/megaraid_sas_base.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 88acefbf9aeaba..6c79c350a4d5ba 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -1981,8 +1981,6 @@ megasas_set_nvme_device_properties(struct scsi_device *sdev,
 
 	lim->max_hw_sectors = max_io_size / 512;
 	lim->virt_boundary_mask = mr_nvme_pg_size - 1;
-
-	blk_queue_flag_set(QUEUE_FLAG_NOMERGES, sdev->request_queue);
 }
 
 /*
-- 
2.43.0

[PATCH 3/5] mpt3sas_scsih: don't set QUEUE_FLAG_NOMERGES

[Christoph Hellwig]

Setting QUEUE_FLAG_NOMERGES was added in commit d1b01d14b7ba ("scsi:
mpt3sas: Set NVMe device queue depth as 128") without any explanation.
Drivers should second guess the block layer merge decisions, so remove
the flag.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 12d08d8ba5382d..b050aedc9d4334 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2680,12 +2680,6 @@ scsih_device_configure(struct scsi_device *sdev, struct queue_limits *lim)
 		pcie_device_put(pcie_device);
 		spin_unlock_irqrestore(&ioc->pcie_device_lock, flags);
 		mpt3sas_scsih_change_queue_depth(sdev, qdepth);
-		/* Enable QUEUE_FLAG_NOMERGES flag, so that IOs won't be
-		 ** merged and can eliminate holes created during merging
-		 ** operation.
-		 **/
-		blk_queue_flag_set(QUEUE_FLAG_NOMERGES,
-				sdev->request_queue);
 		lim->virt_boundary_mask = ioc->page_size - 1;
 		return 0;
 	}
-- 
2.43.0

[PATCH 4/5] rnbd: don't set QUEUE_FLAG_SAME_COMP

[Christoph Hellwig]

QUEUE_FLAG_SAME_COMP is already set by default.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/rnbd/rnbd-clt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 4918b0f68b46cd..0e3773fe479706 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -1397,7 +1397,6 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev,
 	dev->queue = dev->gd->queue;
 	rnbd_init_mq_hw_queues(dev);
 
-	blk_queue_flag_set(QUEUE_FLAG_SAME_COMP, dev->queue);
 	blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
 	return rnbd_clt_setup_gen_disk(dev, rsp, idx);
 }
-- 
2.43.0

[PATCH 5/5] rnbd-cnt: don't set QUEUE_FLAG_SAME_FORCE

[Christoph Hellwig]

QUEUE_FLAG_SAME_FORCE has been set by rnbd-cnt since the initial
merge.  There is no good reason for a driver to force exact core
delivery, which is tunable for very specific workloads and not a
driver setting.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/rnbd/rnbd-clt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index 0e3773fe479706..c34695d2eea7fe 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -1397,7 +1397,6 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev,
 	dev->queue = dev->gd->queue;
 	rnbd_init_mq_hw_queues(dev);
 
-	blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
 	return rnbd_clt_setup_gen_disk(dev, rsp, idx);
 }
 
-- 
2.43.0

Re: [PATCH 4/5] rnbd: don't set QUEUE_FLAG_SAME_COMP

[Jinpu Wang]

On Thu, Jun 27, 2024 at 2:49 PM Christoph Hellwig <hch@lst.de> wrote:
>
> QUEUE_FLAG_SAME_COMP is already set by default.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

 Acked-by: Jack Wang <jinpu.wang@ionos.com>
Thx!
>
> ---
>  drivers/block/rnbd/rnbd-clt.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 4918b0f68b46cd..0e3773fe479706 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -1397,7 +1397,6 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev,
>         dev->queue = dev->gd->queue;
>         rnbd_init_mq_hw_queues(dev);
>
> -       blk_queue_flag_set(QUEUE_FLAG_SAME_COMP, dev->queue);
>         blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
>         return rnbd_clt_setup_gen_disk(dev, rsp, idx);
>  }
> --
> 2.43.0
>

Re: [PATCH 5/5] rnbd-cnt: don't set QUEUE_FLAG_SAME_FORCE

[Jinpu Wang]

On Thu, Jun 27, 2024 at 2:49 PM Christoph Hellwig <hch@lst.de> wrote:
>
> QUEUE_FLAG_SAME_FORCE has been set by rnbd-cnt since the initial
> merge.  There is no good reason for a driver to force exact core
> delivery, which is tunable for very specific workloads and not a
> driver setting.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
en, I think it's fine, we still have the option to set it via block
sysfs interface.
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/block/rnbd/rnbd-clt.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
> index 0e3773fe479706..c34695d2eea7fe 100644
> --- a/drivers/block/rnbd/rnbd-clt.c
> +++ b/drivers/block/rnbd/rnbd-clt.c
> @@ -1397,7 +1397,6 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev,
>         dev->queue = dev->gd->queue;
>         rnbd_init_mq_hw_queues(dev);
>
> -       blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
>         return rnbd_clt_setup_gen_disk(dev, rsp, idx);
>  }
>
> --
> 2.43.0
>

Re: get drivers out of setting queue flags

[Bart Van Assche]

On 6/27/24 5:49 AM, Christoph Hellwig wrote:
> now that driver features have been moved out of the queue flags,
> the abuses where drivers set random internal queue flags stand out
> even more.  This series fixes them up.

For the entire series:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Thanks!

Bart.

Re: get drivers out of setting queue flags

[John Garry]

On 27/06/2024 13:49, Christoph Hellwig wrote:
> Hi all,
> 
> now that driver features have been moved out of the queue flags,
> the abuses where drivers set random internal queue flags stand out
> even more.  This series fixes them up.

If no driver needs to know about these flags, could they now live in 
block/*.h ?

> 
> Diffstat:
>   block/loop.c                      |   15 ++-------------
>   block/rnbd/rnbd-clt.c             |    2 --
>   scsi/megaraid/megaraid_sas_base.c |    2 --
>   scsi/mpt3sas/mpt3sas_scsih.c      |    6 ------
>   4 files changed, 2 insertions(+), 23 deletions(-)
> 

Re: get drivers out of setting queue flags

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 08:46:08AM +0100, John Garry wrote:
> On 27/06/2024 13:49, Christoph Hellwig wrote:
>> Hi all,
>>
>> now that driver features have been moved out of the queue flags,
>> the abuses where drivers set random internal queue flags stand out
>> even more.  This series fixes them up.
>
> If no driver needs to know about these flags, could they now live in 
> block/*.h ?

That is my ultimate plan, but currently they still are checked in
a few places in drivers.  I'll need some more time to sort this
out properly, but I hope that we'll get there.

Re: get drivers out of setting queue flags

[Jens Axboe]

On Thu, 27 Jun 2024 14:49:10 +0200, Christoph Hellwig wrote:
> now that driver features have been moved out of the queue flags,
> the abuses where drivers set random internal queue flags stand out
> even more.  This series fixes them up.
> 
> Diffstat:
>  block/loop.c                      |   15 ++-------------
>  block/rnbd/rnbd-clt.c             |    2 --
>  scsi/megaraid/megaraid_sas_base.c |    2 --
>  scsi/mpt3sas/mpt3sas_scsih.c      |    6 ------
>  4 files changed, 2 insertions(+), 23 deletions(-)
> 
> [...]

Applied, thanks!

[1/5] loop: don't set QUEUE_FLAG_NOMERGES
      commit: 667ea36378cf7f669044b27871c496e1559c872a
[2/5] megaraid_sas: don't set QUEUE_FLAG_NOMERGES
      commit: aa57abe6a7f91fafe53fb98d0f1e74db951bce24
[3/5] mpt3sas_scsih: don't set QUEUE_FLAG_NOMERGES
      commit: 8b77f23fadcbb030a898f168bebe74f465e5d5a2
[4/5] rnbd: don't set QUEUE_FLAG_SAME_COMP
      commit: 40988f15907baee227d3b83bd4d8f8fdfeb95dd3
[5/5] rnbd-cnt: don't set QUEUE_FLAG_SAME_FORCE
      commit: caffa7cdce47718a0c2e3195c9a1bcf786d655a4

Best regards,
-- 
Jens Axboe