Re: [PATCH V4 11/12] scsi: make sure sdev->queue_depth is <= max(shost->can_queue, 1024)

Hannes Reinecke
2020-11-16T10:21:51Z

On 11/16/20 10:07 AM, Ming Lei wrote:
> Limit scsi device's queue depth is less than max(host->can_queue, 1024)
> in scsi_change_queue_depth(), and 1024 is big enough for saturating
> current fast SCSI LUN(SSD, or raid volume on multiple SSDs).
>
> We need this patch for replacing sdev->device_busy with sbitmap which
> has to be pre-allocated with reasonable max depth.
>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com>
> Cc: Ewan D. Milne <emilne@redhat.com>
> Tested-by: Sumanesh Samanta <sumanesh.samanta@broadcom.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/scsi/scsi.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 24619c3bebd5..a28d48c850cf 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -214,6 +214,15 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
> scsi_io_completion(cmd, good_bytes);
> }
>
> +
> +/*
> + * 1024 is big enough for saturating the fast scsi LUN now
> + */
> +static int scsi_device_max_queue_depth(struct scsi_device *sdev)
> +{
> + return max_t(int, sdev->host->can_queue, 1024);
> +}
> +

Shouldn't this rather be initialized with scsi_host->can_queue?
These 'should be enough' settings inevitable turn out to be not enough
in the long run ...

> /**
> * scsi_change_queue_depth - change a device's queue depth
> * @sdev: SCSI Device in question
> @@ -223,6 +232,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
> */
> int scsi_change_queue_depth(struct scsi_device *sdev, int depth)
> {
> + depth = min_t(int, depth, scsi_device_max_queue_depth(sdev));
> +
> if (depth > 0) {
> sdev->queue_depth = depth;
> wmb();
>

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer