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

From: Ming Lei <ming.lei@redhat.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: linux-scsi@vger.kernel.org,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Jens Axboe <axboe@kernel.dk>,
	"Ewan D . Milne" <emilne@redhat.com>,
	Omar Sandoval <osandov@fb.com>, Christoph Hellwig <hch@lst.de>,
	Kashyap Desai <kashyap.desai@broadcom.com>,
	Hannes Reinecke <hare@suse.de>,
	Laurence Oberman <loberman@redhat.com>,
	Bart Van Assche <bart.vanassche@wdc.com>
Subject: Re: [RFC PATCH V4 2/2] scsi: core: don't limit per-LUN queue depth for SSD
Date: Thu, 10 Oct 2019 08:43:29 +0800	[thread overview]
Message-ID: <20191010004328.GA23292@ming.t460p> (raw)
In-Reply-To: <75fe51d7-714f-8a51-89b5-aeeb7d318fdc@acm.org>

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