| Message ID | 20210415015031.607153-1-dgilbert@interlog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fc09acb7de31badb2ea9e85d21e071be1a5736e4 |
| Headers | show |
| Series | scsi_debug: fix cmd_per_lun, set to max_queue | expand |
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);
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
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
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
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
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
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
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) >
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) >> >
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!
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)
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(-)