* [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
@ 2021-04-15 1:50 Douglas Gilbert
2021-04-15 9:15 ` John Garry
2021-04-16 9:12 ` John Garry
0 siblings, 2 replies; 11+ messages in thread
From: Douglas Gilbert @ 2021-04-15 1:50 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, jejb, kashyap.desai, ming.lei, john.garry, axboe
Make sure that the cmd_per_lun value placed in the host template
never exceeds the can_queue value. If the max_queue driver
parameter is not specified then both cmd_per_lun and can_queue are
set to CAN_QUEUE. CAN_QUEUE is a compile time constant and is used
to dimension an array to hold queued requests. If the max_queue
driver parameter is given it is must be less than or equal to
CAN_QUEUE and if so, the host template values are adjusted.
Remove undocumented code that allowed queue_depth to exceed
CAN_QUEUE and cause stack full type errors. There is a documented
way to do that with every_nth and
echo 0x8000 > /sys/bus/pseudo/drivers/scsi_debug/opts
See: https://sg.danny.cz/sg/scsi_debug.html
Tweak some formatting, and add a suggestion to the "trim
poll_queues" warning.
Reported-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
drivers/scsi/scsi_debug.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 70165be10f00..a5d1633b5bd8 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -218,7 +218,7 @@ static const char *sdebug_version_date = "20200710";
*/
#define SDEBUG_CANQUEUE_WORDS 3 /* a WORD is bits in a long */
#define SDEBUG_CANQUEUE (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
-#define DEF_CMD_PER_LUN 255
+#define DEF_CMD_PER_LUN SDEBUG_CANQUEUE
/* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
#define F_D_IN 1 /* Data-in command (e.g. READ) */
@@ -5695,8 +5695,8 @@ MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP command (def=0)");
MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit (def=0)");
MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (def=0)");
MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
-MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
MODULE_PARM_DESC(lun_format, "LUN format: 0->peripheral (def); 1 --> flat address method");
+MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on MEDIUM error");
MODULE_PARM_DESC(medium_error_start, "starting sector number to return MEDIUM error");
@@ -5710,7 +5710,7 @@ MODULE_PARM_DESC(opt_xferlen_exp, "optimal transfer length granularity exponent
MODULE_PARM_DESC(opts, "1->noise, 2->medium_err, 4->timeout, 8->recovered_err... (def=0)");
MODULE_PARM_DESC(per_host_store, "If set, next positive add_host will get new store (def=0)");
MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
-MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to max(submit_queues - 1)");
+MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to max(submit_queues - 1))");
MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
MODULE_PARM_DESC(random, "If set, uniformly randomize command duration between 0 and delay_in_ns");
MODULE_PARM_DESC(removable, "claim to have removable media (def=0)");
@@ -7165,12 +7165,15 @@ static int sdebug_change_qdepth(struct scsi_device *sdev, int qdepth)
}
num_in_q = atomic_read(&devip->num_in_q);
+ if (qdepth > SDEBUG_CANQUEUE) {
+ qdepth = SDEBUG_CANQUEUE;
+ pr_warn("%s: requested qdepth [%d] exceeds canqueue [%d], trim\n", __func__,
+ qdepth, SDEBUG_CANQUEUE);
+ }
if (qdepth < 1)
qdepth = 1;
- /* allow to exceed max host qc_arr elements for testing */
- if (qdepth > SDEBUG_CANQUEUE + 10)
- qdepth = SDEBUG_CANQUEUE + 10;
- scsi_change_queue_depth(sdev, qdepth);
+ if (qdepth != sdev->queue_depth)
+ scsi_change_queue_depth(sdev, qdepth);
if (SDEBUG_OPT_Q_NOISE & sdebug_opts) {
sdev_printk(KERN_INFO, sdev, "%s: qdepth=%d, num_in_q=%d\n",
@@ -7558,6 +7561,7 @@ static int sdebug_driver_probe(struct device *dev)
sdbg_host = to_sdebug_host(dev);
sdebug_driver_template.can_queue = sdebug_max_queue;
+ sdebug_driver_template.cmd_per_lun = sdebug_max_queue;
if (!sdebug_clustering)
sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
@@ -7593,7 +7597,11 @@ static int sdebug_driver_probe(struct device *dev)
* If condition not met, trim poll_queues to 1 (just for simplicity).
*/
if (poll_queues >= submit_queues) {
- pr_warn("%s: trim poll_queues to 1\n", my_name);
+ if (submit_queues < 3)
+ pr_warn("%s: trim poll_queues to 1\n", my_name);
+ else
+ pr_warn("%s: trim poll_queues to 1. Perhaps try poll_queues=%d\n",
+ my_name, submit_queues - 1);
poll_queues = 1;
}
if (poll_queues)
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-15 1:50 [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue Douglas Gilbert
@ 2021-04-15 9:15 ` John Garry
2021-04-16 1:46 ` Ming Lei
2021-04-16 9:12 ` John Garry
1 sibling, 1 reply; 11+ messages in thread
From: John Garry @ 2021-04-15 9:15 UTC (permalink / raw)
To: Douglas Gilbert, linux-scsi
Cc: martin.petersen, jejb, kashyap.desai, ming.lei, axboe
This looks ok.
Apart from this, I tested linux-next (without this patch) - which
includes Ming's changes to replace sdev-->device_busy with sbitmap -
and, as expected, it has the issue.
So I think it is also worth having this to stop this happening elsewhere:
------>8-------
Subject: [PATCH] scsi: core: Cap initial sdev queue depth at Shost.can_queue
Function sdev_store_queue_depth() enforces that the sdev queue depth
cannot exceed Shost.can_queue.
However, the LLDD may still set cmd_per_lun > can_queue, which would
lead to an initial sdev queue depth greater than can_queue.
Stop this happened by capping initial sdev queue depth at can_queue.
<insert credits>
Signed-off-by: John Garry <john.garry@huawei.com>
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 9af50e6f94c4..fec6c17ff37c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -218,6 +218,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
scsi_target *starget,
struct scsi_device *sdev;
int display_failure_msg = 1, ret;
struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+ int depth;
sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
GFP_KERNEL);
@@ -276,8 +277,13 @@ static struct scsi_device *scsi_alloc_sdev(struct
scsi_target *starget,
WARN_ON_ONCE(!blk_get_queue(sdev->request_queue));
sdev->request_queue->queuedata = sdev;
- scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
- sdev->host->cmd_per_lun : 1);
+ if (sdev->host->cmd_per_lun)
+ depth = min_t(int, sdev->host->cmd_per_lun,
+ sdev->host->can_queue);
+ else
+ depth = 1;
+
+ scsi_change_queue_depth(sdev, depth);
scsi_sysfs_device_initialize(sdev);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-15 9:15 ` John Garry
@ 2021-04-16 1:46 ` Ming Lei
2021-04-16 8:17 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2021-04-16 1:46 UTC (permalink / raw)
To: John Garry
Cc: Douglas Gilbert, linux-scsi, martin.petersen, jejb, kashyap.desai, axboe
On Thu, Apr 15, 2021 at 10:15:27AM +0100, John Garry wrote:
> This looks ok.
>
> Apart from this, I tested linux-next (without this patch) - which includes
> Ming's changes to replace sdev-->device_busy with sbitmap - and, as
> expected, it has the issue.
>
> So I think it is also worth having this to stop this happening elsewhere:
>
> ------>8-------
>
> Subject: [PATCH] scsi: core: Cap initial sdev queue depth at Shost.can_queue
>
> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
> exceed Shost.can_queue.
>
> However, the LLDD may still set cmd_per_lun > can_queue, which would lead to
> an initial sdev queue depth greater than can_queue.
>
> Stop this happened by capping initial sdev queue depth at can_queue.
>
> <insert credits>
> Signed-off-by: John Garry <john.garry@huawei.com>
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 9af50e6f94c4..fec6c17ff37c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -218,6 +218,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
> scsi_target *starget,
> struct scsi_device *sdev;
> int display_failure_msg = 1, ret;
> struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
> + int depth;
>
> sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
> GFP_KERNEL);
> @@ -276,8 +277,13 @@ static struct scsi_device *scsi_alloc_sdev(struct
> scsi_target *starget,
> WARN_ON_ONCE(!blk_get_queue(sdev->request_queue));
> sdev->request_queue->queuedata = sdev;
>
> - scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
> - sdev->host->cmd_per_lun : 1);
> + if (sdev->host->cmd_per_lun)
> + depth = min_t(int, sdev->host->cmd_per_lun,
> + sdev->host->can_queue);
> + else
> + depth = 1;
> +
> + scsi_change_queue_depth(sdev, depth);
'cmd_per_lun' should have been set as correct from the beginning instead
of capping it for changing queue depth:
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 697c09ef259b..0d9954eabbb8 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -414,7 +414,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
shost->can_queue = sht->can_queue;
shost->sg_tablesize = sht->sg_tablesize;
shost->sg_prot_tablesize = sht->sg_prot_tablesize;
- shost->cmd_per_lun = sht->cmd_per_lun;
+ shost->cmd_per_lun = min_t(int, sht->cmd_per_lun, shost->can_queue);
shost->no_write_same = sht->no_write_same;
shost->host_tagset = sht->host_tagset;
Thanks,
Ming
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 1:46 ` Ming Lei
@ 2021-04-16 8:17 ` John Garry
2021-04-16 8:26 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2021-04-16 8:17 UTC (permalink / raw)
To: Ming Lei
Cc: Douglas Gilbert, linux-scsi, martin.petersen, jejb, kashyap.desai, axboe
On 16/04/2021 02:46, Ming Lei wrote:
>> int display_failure_msg = 1, ret;
>> struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>> + int depth;
>>
>> sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
>> GFP_KERNEL);
>> @@ -276,8 +277,13 @@ static struct scsi_device *scsi_alloc_sdev(struct
>> scsi_target *starget,
>> WARN_ON_ONCE(!blk_get_queue(sdev->request_queue));
>> sdev->request_queue->queuedata = sdev;
>>
>> - scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
>> - sdev->host->cmd_per_lun : 1);
>> + if (sdev->host->cmd_per_lun)
>> + depth = min_t(int, sdev->host->cmd_per_lun,
>> + sdev->host->can_queue);
>> + else
>> + depth = 1;
>> +
>> + scsi_change_queue_depth(sdev, depth);
> 'cmd_per_lun' should have been set as correct from the beginning instead
> of capping it for changing queue depth:
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 697c09ef259b..0d9954eabbb8 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -414,7 +414,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> shost->can_queue = sht->can_queue;
> shost->sg_tablesize = sht->sg_tablesize;
> shost->sg_prot_tablesize = sht->sg_prot_tablesize;
> - shost->cmd_per_lun = sht->cmd_per_lun;
> + shost->cmd_per_lun = min_t(int, sht->cmd_per_lun, shost->can_queue);
> shost->no_write_same = sht->no_write_same;
> shost->host_tagset = sht->host_tagset;
My concern here is that it is a common pattern in LLDDs to overwrite the
initial shost member values between scsi_host_alloc() and scsi_add_host().
Thanks,
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 8:17 ` John Garry
@ 2021-04-16 8:26 ` Ming Lei
2021-04-16 8:33 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2021-04-16 8:26 UTC (permalink / raw)
To: John Garry
Cc: Douglas Gilbert, linux-scsi, martin.petersen, jejb, kashyap.desai, axboe
On Fri, Apr 16, 2021 at 09:17:09AM +0100, John Garry wrote:
> On 16/04/2021 02:46, Ming Lei wrote:
> > > int display_failure_msg = 1, ret;
> > > struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
> > > + int depth;
> > >
> > > sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
> > > GFP_KERNEL);
> > > @@ -276,8 +277,13 @@ static struct scsi_device *scsi_alloc_sdev(struct
> > > scsi_target *starget,
> > > WARN_ON_ONCE(!blk_get_queue(sdev->request_queue));
> > > sdev->request_queue->queuedata = sdev;
> > >
> > > - scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
> > > - sdev->host->cmd_per_lun : 1);
> > > + if (sdev->host->cmd_per_lun)
> > > + depth = min_t(int, sdev->host->cmd_per_lun,
> > > + sdev->host->can_queue);
> > > + else
> > > + depth = 1;
> > > +
> > > + scsi_change_queue_depth(sdev, depth);
> > 'cmd_per_lun' should have been set as correct from the beginning instead
> > of capping it for changing queue depth:
> >
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 697c09ef259b..0d9954eabbb8 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -414,7 +414,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> > shost->can_queue = sht->can_queue;
> > shost->sg_tablesize = sht->sg_tablesize;
> > shost->sg_prot_tablesize = sht->sg_prot_tablesize;
> > - shost->cmd_per_lun = sht->cmd_per_lun;
> > + shost->cmd_per_lun = min_t(int, sht->cmd_per_lun, shost->can_queue);
> > shost->no_write_same = sht->no_write_same;
> > shost->host_tagset = sht->host_tagset;
>
> My concern here is that it is a common pattern in LLDDs to overwrite the
> initial shost member values between scsi_host_alloc() and scsi_add_host().
OK, then can we move the fix into beginning of scsi_add_host()?
Thanks,
Ming
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 8:26 ` Ming Lei
@ 2021-04-16 8:33 ` John Garry
2021-04-16 8:50 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2021-04-16 8:33 UTC (permalink / raw)
To: Ming Lei
Cc: Douglas Gilbert, linux-scsi, martin.petersen, jejb, kashyap.desai, axboe
On 16/04/2021 09:26, Ming Lei wrote:
>>> 'cmd_per_lun' should have been set as correct from the beginning instead
>>> of capping it for changing queue depth:
>>>
>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>> index 697c09ef259b..0d9954eabbb8 100644
>>> --- a/drivers/scsi/hosts.c
>>> +++ b/drivers/scsi/hosts.c
>>> @@ -414,7 +414,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>> shost->can_queue = sht->can_queue;
>>> shost->sg_tablesize = sht->sg_tablesize;
>>> shost->sg_prot_tablesize = sht->sg_prot_tablesize;
>>> - shost->cmd_per_lun = sht->cmd_per_lun;
>>> + shost->cmd_per_lun = min_t(int, sht->cmd_per_lun, shost->can_queue);
>>> shost->no_write_same = sht->no_write_same;
>>> shost->host_tagset = sht->host_tagset;
>> My concern here is that it is a common pattern in LLDDs to overwrite the
>> initial shost member values between scsi_host_alloc() and scsi_add_host().
> OK, then can we move the fix into beginning of scsi_add_host()?
I suppose that would be ok, but we don't do much sanitizing shost values
at that point. Apart from failing can_queue == 0. I suppose failing
can_queue < cmd_per_lun could also be added.
Thanks,
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 8:33 ` John Garry
@ 2021-04-16 8:50 ` Ming Lei
2021-04-16 9:07 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2021-04-16 8:50 UTC (permalink / raw)
To: John Garry
Cc: Douglas Gilbert, linux-scsi, martin.petersen, jejb, kashyap.desai, axboe
On Fri, Apr 16, 2021 at 09:33:48AM +0100, John Garry wrote:
> On 16/04/2021 09:26, Ming Lei wrote:
> > > > 'cmd_per_lun' should have been set as correct from the beginning instead
> > > > of capping it for changing queue depth:
> > > >
> > > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > > > index 697c09ef259b..0d9954eabbb8 100644
> > > > --- a/drivers/scsi/hosts.c
> > > > +++ b/drivers/scsi/hosts.c
> > > > @@ -414,7 +414,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> > > > shost->can_queue = sht->can_queue;
> > > > shost->sg_tablesize = sht->sg_tablesize;
> > > > shost->sg_prot_tablesize = sht->sg_prot_tablesize;
> > > > - shost->cmd_per_lun = sht->cmd_per_lun;
> > > > + shost->cmd_per_lun = min_t(int, sht->cmd_per_lun, shost->can_queue);
> > > > shost->no_write_same = sht->no_write_same;
> > > > shost->host_tagset = sht->host_tagset;
> > > My concern here is that it is a common pattern in LLDDs to overwrite the
> > > initial shost member values between scsi_host_alloc() and scsi_add_host().
> > OK, then can we move the fix into beginning of scsi_add_host()?
>
> I suppose that would be ok, but we don't do much sanitizing shost values at
> that point. Apart from failing can_queue == 0.
.can_queue has been finalized in scsi_add_host(), since it will be used for
setting tagset, so .can_queue is reliable at that time.
>I suppose failing can_queue < cmd_per_lun could also be added.
That will fail add host for scsi_debug simply.
Thanks,
Ming
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 8:50 ` Ming Lei
@ 2021-04-16 9:07 ` John Garry
0 siblings, 0 replies; 11+ messages in thread
From: John Garry @ 2021-04-16 9:07 UTC (permalink / raw)
To: Ming Lei
Cc: Douglas Gilbert, linux-scsi, martin.petersen, jejb, kashyap.desai, axboe
On 16/04/2021 09:50, Ming Lei wrote:
>>>> My concern here is that it is a common pattern in LLDDs to overwrite the
>>>> initial shost member values between scsi_host_alloc() and scsi_add_host().
>>> OK, then can we move the fix into beginning of scsi_add_host()?
>> I suppose that would be ok, but we don't do much sanitizing shost values at
>> that point. Apart from failing can_queue == 0.
> .can_queue has been finalized in scsi_add_host(), since it will be used for
> setting tagset, so .can_queue is reliable at that time.
>
>> I suppose failing can_queue < cmd_per_lun could also be added.
> That will fail add host for scsi_debug simply.
But we still should have Doug's patch regardless.
Anyway, I'll prepare a patch, so we can discuss further on that thread.
Thanks,
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-15 1:50 [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue Douglas Gilbert
2021-04-15 9:15 ` John Garry
@ 2021-04-16 9:12 ` John Garry
2021-04-16 16:32 ` Douglas Gilbert
1 sibling, 1 reply; 11+ messages in thread
From: John Garry @ 2021-04-16 9:12 UTC (permalink / raw)
To: Douglas Gilbert, linux-scsi
Cc: martin.petersen, jejb, kashyap.desai, ming.lei, axboe
On 15/04/2021 02:50, Douglas Gilbert wrote:
> Make sure that the cmd_per_lun value placed in the host template
> never exceeds the can_queue value. If the max_queue driver
> parameter is not specified then both cmd_per_lun and can_queue are
> set to CAN_QUEUE. CAN_QUEUE is a compile time constant and is used
> to dimension an array to hold queued requests. If the max_queue
> driver parameter is given it is must be less than or equal to
> CAN_QUEUE and if so, the host template values are adjusted.
>
> Remove undocumented code that allowed queue_depth to exceed
> CAN_QUEUE and cause stack full type errors. There is a documented
> way to do that with every_nth and
> echo 0x8000 > /sys/bus/pseudo/drivers/scsi_debug/opts
> See: https://sg.danny.cz/sg/scsi_debug.html
>
> Tweak some formatting, and add a suggestion to the "trim
> poll_queues" warning.
>
> Reported-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
Just one comment, below:
Reviewed-by: John Garry <john.garry@hauwei.com>
Thanks
> ---
> drivers/scsi/scsi_debug.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 70165be10f00..a5d1633b5bd8 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -218,7 +218,7 @@ static const char *sdebug_version_date = "20200710";
> */
> #define SDEBUG_CANQUEUE_WORDS 3 /* a WORD is bits in a long */
> #define SDEBUG_CANQUEUE (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
> -#define DEF_CMD_PER_LUN 255
> +#define DEF_CMD_PER_LUN SDEBUG_CANQUEUE
>
> /* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
> #define F_D_IN 1 /* Data-in command (e.g. READ) */
> @@ -5695,8 +5695,8 @@ MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP command (def=0)");
> MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit (def=0)");
> MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (def=0)");
> MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
> -MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
> MODULE_PARM_DESC(lun_format, "LUN format: 0->peripheral (def); 1 --> flat address method");
> +MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
Changes like this should really be in another patch.
> MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
> MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on MEDIUM error");
> MODULE_PARM_DESC(medium_error_start, "starting sector number to return MEDIUM error");
> @@ -5710,7 +5710,7 @@ MODULE_PARM_DESC(opt_xferlen_exp, "optimal transfer length granularity exponent
> MODULE_PARM_DESC(opts, "1->noise, 2->medium_err, 4->timeout, 8->recovered_err... (def=0)");
> MODULE_PARM_DESC(per_host_store, "If set, next positive add_host will get new store (def=0)");
> MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
> -MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to max(submit_queues - 1)");
> +MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to max(submit_queues - 1))");
> MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
> MODULE_PARM_DESC(random, "If set, uniformly randomize command duration between 0 and delay_in_ns");
> MODULE_PARM_DESC(removable, "claim to have removable media (def=0)");
> @@ -7165,12 +7165,15 @@ static int sdebug_change_qdepth(struct scsi_device *sdev, int qdepth)
> }
> num_in_q = atomic_read(&devip->num_in_q);
>
> + if (qdepth > SDEBUG_CANQUEUE) {
> + qdepth = SDEBUG_CANQUEUE;
> + pr_warn("%s: requested qdepth [%d] exceeds canqueue [%d], trim\n", __func__,
> + qdepth, SDEBUG_CANQUEUE);
> + }
> if (qdepth < 1)
> qdepth = 1;
> - /* allow to exceed max host qc_arr elements for testing */
> - if (qdepth > SDEBUG_CANQUEUE + 10)
> - qdepth = SDEBUG_CANQUEUE + 10;
> - scsi_change_queue_depth(sdev, qdepth);
> + if (qdepth != sdev->queue_depth)
> + scsi_change_queue_depth(sdev, qdepth);
>
> if (SDEBUG_OPT_Q_NOISE & sdebug_opts) {
> sdev_printk(KERN_INFO, sdev, "%s: qdepth=%d, num_in_q=%d\n",
> @@ -7558,6 +7561,7 @@ static int sdebug_driver_probe(struct device *dev)
> sdbg_host = to_sdebug_host(dev);
>
> sdebug_driver_template.can_queue = sdebug_max_queue;
> + sdebug_driver_template.cmd_per_lun = sdebug_max_queue;
> if (!sdebug_clustering)
> sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
>
> @@ -7593,7 +7597,11 @@ static int sdebug_driver_probe(struct device *dev)
> * If condition not met, trim poll_queues to 1 (just for simplicity).
> */
> if (poll_queues >= submit_queues) {
> - pr_warn("%s: trim poll_queues to 1\n", my_name);
> + if (submit_queues < 3)
> + pr_warn("%s: trim poll_queues to 1\n", my_name);
> + else
> + pr_warn("%s: trim poll_queues to 1. Perhaps try poll_queues=%d\n",
> + my_name, submit_queues - 1);
> poll_queues = 1;
> }
> if (poll_queues)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 9:12 ` John Garry
@ 2021-04-16 16:32 ` Douglas Gilbert
2021-04-29 13:17 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Douglas Gilbert @ 2021-04-16 16:32 UTC (permalink / raw)
To: John Garry, linux-scsi
Cc: martin.petersen, jejb, kashyap.desai, ming.lei, axboe
On 2021-04-16 5:12 a.m., John Garry wrote:
> On 15/04/2021 02:50, Douglas Gilbert wrote:
>> Make sure that the cmd_per_lun value placed in the host template
>> never exceeds the can_queue value. If the max_queue driver
>> parameter is not specified then both cmd_per_lun and can_queue are
>> set to CAN_QUEUE. CAN_QUEUE is a compile time constant and is used
>> to dimension an array to hold queued requests. If the max_queue
>> driver parameter is given it is must be less than or equal to
>> CAN_QUEUE and if so, the host template values are adjusted.
>>
>> Remove undocumented code that allowed queue_depth to exceed
>> CAN_QUEUE and cause stack full type errors. There is a documented
>> way to do that with every_nth and
>> echo 0x8000 > /sys/bus/pseudo/drivers/scsi_debug/opts
>> See: https://sg.danny.cz/sg/scsi_debug.html
>>
>> Tweak some formatting, and add a suggestion to the "trim
>> poll_queues" warning.
>>
>> Reported-by: Kashyap Desai <kashyap.desai@broadcom.com>
>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
>
> Just one comment, below:
The scsi_debug driver has around 64 driver startup and sysfs
parameters, many from other users. Trying to track and document
these is a pain, so to lighten my load I try to keep them in
alphabetical order. In my experience suggesting that a new patch
follows that convention doesn't always work, it gets applied in
its original form.
Anyway, I just updated:
https://sg.danny.cz/sg/scsi_debug.html
Suggested improvements welcome.
> Reviewed-by: John Garry <john.garry@hauwei.com>
Thanks.
Doug Gilbert
>> ---
>> drivers/scsi/scsi_debug.c | 24 ++++++++++++++++--------
>> 1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 70165be10f00..a5d1633b5bd8 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -218,7 +218,7 @@ static const char *sdebug_version_date = "20200710";
>> */
>> #define SDEBUG_CANQUEUE_WORDS 3 /* a WORD is bits in a long */
>> #define SDEBUG_CANQUEUE (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
>> -#define DEF_CMD_PER_LUN 255
>> +#define DEF_CMD_PER_LUN SDEBUG_CANQUEUE
>> /* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
>> #define F_D_IN 1 /* Data-in command (e.g. READ) */
>> @@ -5695,8 +5695,8 @@ MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP
>> command (def=0)");
>> MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit
>> (def=0)");
>> MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit
>> (def=0)");
>> MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
>> -MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
>> MODULE_PARM_DESC(lun_format, "LUN format: 0->peripheral (def); 1 --> flat
>> address method");
>> +MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
>
> Changes like this should really be in another patch.
>
>> MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
>> MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on
>> MEDIUM error");
>> MODULE_PARM_DESC(medium_error_start, "starting sector number to return
>> MEDIUM error");
>> @@ -5710,7 +5710,7 @@ MODULE_PARM_DESC(opt_xferlen_exp, "optimal transfer
>> length granularity exponent
>> MODULE_PARM_DESC(opts, "1->noise, 2->medium_err, 4->timeout,
>> 8->recovered_err... (def=0)");
>> MODULE_PARM_DESC(per_host_store, "If set, next positive add_host will get
>> new store (def=0)");
>> MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
>> -MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to
>> max(submit_queues - 1)");
>> +MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to
>> max(submit_queues - 1))");
>> MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
>> MODULE_PARM_DESC(random, "If set, uniformly randomize command duration
>> between 0 and delay_in_ns");
>> MODULE_PARM_DESC(removable, "claim to have removable media (def=0)");
>> @@ -7165,12 +7165,15 @@ static int sdebug_change_qdepth(struct scsi_device
>> *sdev, int qdepth)
>> }
>> num_in_q = atomic_read(&devip->num_in_q);
>> + if (qdepth > SDEBUG_CANQUEUE) {
>> + qdepth = SDEBUG_CANQUEUE;
>> + pr_warn("%s: requested qdepth [%d] exceeds canqueue [%d], trim\n",
>> __func__,
>> + qdepth, SDEBUG_CANQUEUE);
>> + }
>> if (qdepth < 1)
>> qdepth = 1;
>> - /* allow to exceed max host qc_arr elements for testing */
>> - if (qdepth > SDEBUG_CANQUEUE + 10)
>> - qdepth = SDEBUG_CANQUEUE + 10;
>> - scsi_change_queue_depth(sdev, qdepth);
>> + if (qdepth != sdev->queue_depth)
>> + scsi_change_queue_depth(sdev, qdepth);
>> if (SDEBUG_OPT_Q_NOISE & sdebug_opts) {
>> sdev_printk(KERN_INFO, sdev, "%s: qdepth=%d, num_in_q=%d\n",
>> @@ -7558,6 +7561,7 @@ static int sdebug_driver_probe(struct device *dev)
>> sdbg_host = to_sdebug_host(dev);
>> sdebug_driver_template.can_queue = sdebug_max_queue;
>> + sdebug_driver_template.cmd_per_lun = sdebug_max_queue;
>> if (!sdebug_clustering)
>> sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
>> @@ -7593,7 +7597,11 @@ static int sdebug_driver_probe(struct device *dev)
>> * If condition not met, trim poll_queues to 1 (just for simplicity).
>> */
>> if (poll_queues >= submit_queues) {
>> - pr_warn("%s: trim poll_queues to 1\n", my_name);
>> + if (submit_queues < 3)
>> + pr_warn("%s: trim poll_queues to 1\n", my_name);
>> + else
>> + pr_warn("%s: trim poll_queues to 1. Perhaps try poll_queues=%d\n",
>> + my_name, submit_queues - 1);
>> poll_queues = 1;
>> }
>> if (poll_queues)
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue
2021-04-16 16:32 ` Douglas Gilbert
@ 2021-04-29 13:17 ` John Garry
0 siblings, 0 replies; 11+ messages in thread
From: John Garry @ 2021-04-29 13:17 UTC (permalink / raw)
To: dgilbert, linux-scsi, martin.petersen
Cc: jejb, kashyap.desai, ming.lei, axboe
On 16/04/2021 17:32, Douglas Gilbert wrote:
> On 2021-04-16 5:12 a.m., John Garry wrote:
>> On 15/04/2021 02:50, Douglas Gilbert wrote:
>>> Make sure that the cmd_per_lun value placed in the host template
>>> never exceeds the can_queue value. If the max_queue driver
>>> parameter is not specified then both cmd_per_lun and can_queue are
>>> set to CAN_QUEUE. CAN_QUEUE is a compile time constant and is used
>>> to dimension an array to hold queued requests. If the max_queue
>>> driver parameter is given it is must be less than or equal to
>>> CAN_QUEUE and if so, the host template values are adjusted.
>>>
>>> Remove undocumented code that allowed queue_depth to exceed
>>> CAN_QUEUE and cause stack full type errors. There is a documented
>>> way to do that with every_nth and
>>> echo 0x8000 > /sys/bus/pseudo/drivers/scsi_debug/opts
>>> See: https://sg.danny.cz/sg/scsi_debug.html
>>>
>>> Tweak some formatting, and add a suggestion to the "trim
>>> poll_queues" warning.
>>>
>>> Reported-by: Kashyap Desai <kashyap.desai@broadcom.com>
>>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
Hi Martin,
Could we get this picked up please?
Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-04-29 13:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-15 1:50 [PATCH] scsi_debug: fix cmd_per_lun, set to max_queue Douglas Gilbert
2021-04-15 9:15 ` John Garry
2021-04-16 1:46 ` Ming Lei
2021-04-16 8:17 ` John Garry
2021-04-16 8:26 ` Ming Lei
2021-04-16 8:33 ` John Garry
2021-04-16 8:50 ` Ming Lei
2021-04-16 9:07 ` John Garry
2021-04-16 9:12 ` John Garry
2021-04-16 16:32 ` Douglas Gilbert
2021-04-29 13:17 ` John Garry
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.