* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-19 14:31 [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue John Garry
@ 2021-05-20 1:24 ` Ming Lei
2021-05-20 15:57 ` Bart Van Assche
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2021-05-20 1:24 UTC (permalink / raw)
To: John Garry; +Cc: jejb, martin.petersen, linux-scsi, linux-kernel
On Wed, May 19, 2021 at 10:31:02PM +0800, John Garry wrote:
> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
> exceed Shost.can_queue.
>
> The sdev initial value comes from shost cmd_per_lun.
>
> However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
> initial sdev queue depth greater than can_queue.
>
> Such an issue was reported in [0], which caused a hang. That has since
> been fixed in commit fc09acb7de31 ("scsi: scsi_debug: Fix cmd_per_lun,
> set to max_queue").
>
> Stop this possibly happening for other drivers by capping
> shost.cmd_per_lun at shost.can_queue.
>
> [0] https://lore.kernel.org/linux-scsi/YHaez6iN2HHYxYOh@T590/
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> Earlier patch was in https://lore.kernel.org/linux-scsi/1618848384-204144-1-git-send-email-john.garry@huawei.com/
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index ba72bd4202a2..624e2582c3df 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -220,6 +220,9 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> goto fail;
> }
>
> + shost->cmd_per_lun = min_t(short, shost->cmd_per_lun,
> + shost->can_queue);
> +
> error = scsi_init_sense_cache(shost);
> if (error)
> goto fail;
> --
> 2.26.2
>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
--
Ming
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-19 14:31 [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue John Garry
2021-05-20 1:24 ` Ming Lei
@ 2021-05-20 15:57 ` Bart Van Assche
2021-05-20 16:41 ` John Garry
2021-05-22 4:35 ` Martin K. Petersen
2021-05-26 4:07 ` Martin K. Petersen
3 siblings, 1 reply; 9+ messages in thread
From: Bart Van Assche @ 2021-05-20 15:57 UTC (permalink / raw)
To: John Garry, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel, ming.lei
On 5/19/21 7:31 AM, John Garry wrote:
> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
> exceed Shost.can_queue.
>
> The sdev initial value comes from shost cmd_per_lun.
>
> However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
> initial sdev queue depth greater than can_queue.
>
> Such an issue was reported in [0], which caused a hang. That has since
> been fixed in commit fc09acb7de31 ("scsi: scsi_debug: Fix cmd_per_lun,
> set to max_queue").
>
> Stop this possibly happening for other drivers by capping
> shost.cmd_per_lun at shost.can_queue.
>
> [0] https://lore.kernel.org/linux-scsi/YHaez6iN2HHYxYOh@T590/
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> Earlier patch was in https://lore.kernel.org/linux-scsi/1618848384-204144-1-git-send-email-john.garry@huawei.com/
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index ba72bd4202a2..624e2582c3df 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -220,6 +220,9 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> goto fail;
> }
>
> + shost->cmd_per_lun = min_t(short, shost->cmd_per_lun,
> + shost->can_queue);
> +
> error = scsi_init_sense_cache(shost);
> if (error)
> goto fail;
In SCSI header files a mix of int, short and unsigned int is used for
cmd_per_lun and can_queue. How about changing the types of these two
member variables in include/scsi/*h into u16?
Anyway:
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-20 15:57 ` Bart Van Assche
@ 2021-05-20 16:41 ` John Garry
2021-05-20 16:57 ` Bart Van Assche
2021-05-22 4:28 ` Martin K. Petersen
0 siblings, 2 replies; 9+ messages in thread
From: John Garry @ 2021-05-20 16:41 UTC (permalink / raw)
To: Bart Van Assche, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel, ming.lei
On 20/05/2021 16:57, Bart Van Assche wrote:
> On 5/19/21 7:31 AM, John Garry wrote:
>> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
>> exceed Shost.can_queue.
>>
>> The sdev initial value comes from shost cmd_per_lun.
>>
>> However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
>> initial sdev queue depth greater than can_queue.
>>
>> Such an issue was reported in [0], which caused a hang. That has since
>> been fixed in commit fc09acb7de31 ("scsi: scsi_debug: Fix cmd_per_lun,
>> set to max_queue").
>>
>> Stop this possibly happening for other drivers by capping
>> shost.cmd_per_lun at shost.can_queue.
>>
>> [0] https://lore.kernel.org/linux-scsi/YHaez6iN2HHYxYOh@T590/
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>> Earlier patch was in https://lore.kernel.org/linux-scsi/1618848384-204144-1-git-send-email-john.garry@huawei.com/
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index ba72bd4202a2..624e2582c3df 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -220,6 +220,9 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>> goto fail;
>> }
>>
>> + shost->cmd_per_lun = min_t(short, shost->cmd_per_lun,
>> + shost->can_queue);
>> +
>> error = scsi_init_sense_cache(shost);
>> if (error)
>> goto fail;
>
>
Hi Bart,
> In SCSI header files a mix of int, short and unsigned int is used for
> cmd_per_lun and can_queue. How about changing the types of these two
> member variables in include/scsi/*h into u16?
I don't mind doing that, but is there any requirement for can_queue to
not be limited to 16b?
It seems intentional that can_queue is int and cmd_per_lun is short.
As an aside, if short is 16b, it does not even seem to have efficient
packing on Scsi_host today (although we can move things around):
int can_queue;
short cmd_per_lun;
short unsigned int sg_tablesize;
short unsigned int sg_prot_tablesize;
/* 16b hole */
unsigned int max_sectors;
>
> Anyway:
>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>
thanks!
>
>
> .
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-20 16:41 ` John Garry
@ 2021-05-20 16:57 ` Bart Van Assche
2021-05-20 17:24 ` John Garry
2021-05-22 4:28 ` Martin K. Petersen
1 sibling, 1 reply; 9+ messages in thread
From: Bart Van Assche @ 2021-05-20 16:57 UTC (permalink / raw)
To: John Garry, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel, ming.lei
On 5/20/21 9:41 AM, John Garry wrote:
> On 20/05/2021 16:57, Bart Van Assche wrote:
>> In SCSI header files a mix of int, short and unsigned int is used for
>> cmd_per_lun and can_queue. How about changing the types of these two
>> member variables in include/scsi/*h into u16?
> I don't mind doing that, but is there any requirement for can_queue to
> not be limited to 16b?
Maybe I'm missing something but it is not clear to me why different
structures in the SCSI headers use different data types for can_queue
and cmd_per_lun?
$ git grep -nHEw '(cmd_per_lun|can_queue);' include/scsi
include/scsi/scsi_device.h:318: unsigned int can_queue;
include/scsi/scsi_host.h:372: int can_queue;
include/scsi/scsi_host.h:425: short cmd_per_lun;
include/scsi/scsi_host.h:612: int can_queue;
include/scsi/scsi_host.h:613: short cmd_per_lun;
> It seems intentional that can_queue is int and cmd_per_lun is short.
Intentional? It is not clear to me why? Even high-performance drivers
like iSER and SRP set can_queue by default to a value that fits well in
a 16-bit variable (512 and 64 respectively). The highest value that I
found after a quick search is the following:
#define ISCSI_TOTAL_CMDS_MAX 4096
Thanks,
Bart.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-20 16:57 ` Bart Van Assche
@ 2021-05-20 17:24 ` John Garry
0 siblings, 0 replies; 9+ messages in thread
From: John Garry @ 2021-05-20 17:24 UTC (permalink / raw)
To: Bart Van Assche, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel, ming.lei
On 20/05/2021 17:57, Bart Van Assche wrote:
>> not be limited to 16b?
> Maybe I'm missing something but it is not clear to me why different
> structures in the SCSI headers use different data types for can_queue
> and cmd_per_lun?
For cmd_per_lun, is it related to SCSI task tag limit? SAM-3 says upto
64b for task tag, but then SAS uses 16b for TMF tag, so not sure.
Someone with more SCSI spec knowledge than we can clarify this.
>
> $ git grep -nHEw '(cmd_per_lun|can_queue);' include/scsi
> include/scsi/scsi_device.h:318: unsigned int can_queue;
> include/scsi/scsi_host.h:372: int can_queue;
> include/scsi/scsi_host.h:425: short cmd_per_lun;
> include/scsi/scsi_host.h:612: int can_queue;
> include/scsi/scsi_host.h:613: short cmd_per_lun;
>
>> It seems intentional that can_queue is int and cmd_per_lun is short.
> Intentional? It is not clear to me why? Even high-performance drivers
> like iSER and SRP set can_queue by default to a value that fits well in
> a 16-bit variable (512 and 64 respectively). The highest value that I
> found after a quick search is the following:
>
> #define ISCSI_TOTAL_CMDS_MAX 4096
I guess int was used for can_queue as an arbitrarily big number.
And if we try to use 16b for can_queue, reducing size of
variables/structure members sometimes breaks things, from my experience.
Thanks,
John
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-20 16:41 ` John Garry
2021-05-20 16:57 ` Bart Van Assche
@ 2021-05-22 4:28 ` Martin K. Petersen
1 sibling, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2021-05-22 4:28 UTC (permalink / raw)
To: John Garry
Cc: Bart Van Assche, jejb, martin.petersen, linux-scsi, linux-kernel,
ming.lei
John,
> I don't mind doing that, but is there any requirement for can_queue to
> not be limited to 16b?
>
> It seems intentional that can_queue is int and cmd_per_lun is short.
I suspect cmd_per_lun was originally chosen to be 16 bits because of FC
(SPI was 8 bits). And that it seemed unreasonable that the initiator
should be limited to what a single LUN could express. But this is all
guesswork. This code was obviously written a very, very long time ago...
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-19 14:31 [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue John Garry
2021-05-20 1:24 ` Ming Lei
2021-05-20 15:57 ` Bart Van Assche
@ 2021-05-22 4:35 ` Martin K. Petersen
2021-05-26 4:07 ` Martin K. Petersen
3 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2021-05-22 4:35 UTC (permalink / raw)
To: John Garry; +Cc: jejb, martin.petersen, linux-scsi, linux-kernel, ming.lei
John,
> Function sdev_store_queue_depth() enforces that the sdev queue depth
> cannot exceed Shost.can_queue.
Applied to 5.14/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue
2021-05-19 14:31 [PATCH] scsi: core: Cap shost cmd_per_lun at can_queue John Garry
` (2 preceding siblings ...)
2021-05-22 4:35 ` Martin K. Petersen
@ 2021-05-26 4:07 ` Martin K. Petersen
3 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2021-05-26 4:07 UTC (permalink / raw)
To: jejb, John Garry; +Cc: Martin K . Petersen, linux-kernel, linux-scsi, ming.lei
On Wed, 19 May 2021 22:31:02 +0800, John Garry wrote:
> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
> exceed Shost.can_queue.
>
> The sdev initial value comes from shost cmd_per_lun.
>
> However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
> initial sdev queue depth greater than can_queue.
>
> [...]
Applied to 5.14/scsi-queue, thanks!
[1/1] scsi: core: Cap shost cmd_per_lun at can_queue
https://git.kernel.org/mkp/scsi/c/ea2f0f77538c
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread