Re: [RFC PATCH V4 2/2] scsi: core: don't limit per-LUN queue depth for SSD

Ming Lei
2019-10-10T00:43:44Z

On Wed, Oct 09, 2019 at 09:05:26AM -0700, Bart Van Assche wrote:
> On 10/9/19 2:32 AM, Ming Lei wrote:
> > @@ -354,7 +354,8 @@ void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
> > if (starget->can_queue > 0)
> > atomic_dec(&starget->target_busy);
> > - atomic_dec(&sdev->device_busy);
> > + if (!blk_queue_nonrot(sdev->request_queue))
> > + atomic_dec(&sdev->device_busy);
> > }
>
> Hi Ming,
>
> Does this patch impact the meaning of the queue_depth sysfs attribute (see
> also sdev_store_queue_depth()) and also the queue depth ramp up/down
> mechanism (see also scsi_handle_queue_ramp_up())? Have you considered to

This patch only ignores to track inflight SCSI commands on each
LUN, so it doesn't affect the meaning of sdev->queue_depth, which is
just bypassed for SSD.

> enable/disable busy tracking per LUN depending on whether or not
> sdev->queue_depth < shost->can_queue?

Yeah, we can do it, but that isn't contradictory with this patch, :-)
And we can do it even though sdev->queue_depth < shost->can_queue.

Usually sdev->queue_depth is used for the following reasons:

1) this limit isn't a hard limit, which may improve IO merge with cost
of IO latency.

2) fair dispatch among LUNs.

This patch just tries to not apply per-LUN queue depth for SSD, because:

1) fair dispatch has been done by blk-mq in allocating driver tag

2) IO merge doesn't play big role for SSD, and IO latency can be
increased by applying per-LUN queue depth.

3) NVMe doesn't apply per-ns queue depth

>
> The megaraid and mpt3sas drivers read sdev->device_busy directly. Is the
> current version of this patch compatible with these drivers?

For megaraid, sdev->device_busy is checked for choosing reply queue,
this way shouldn't be better than using default managed IRQ's mapping.
Similar usage is done on _base_get_high_iops_msix_index().

The only effect may be in scsih_dev_reset(), and 'SUCCESS' message
is dumped even though there is in-flight command on this LUN, but it
can be fixed easily.

Thanks,
Ming