[V6] scsi: core: only re-run queue in scsi_end_request() if device queue is busy

Now the request queue is run in scsi_end_request() unconditionally if both
target queue and host queue is ready. We should have re-run request queue
only after this device queue becomes busy for restarting this LUN only.

Recently Long Li reported that cost of run queue may be very heavy in
case of high queue depth. So improve this situation by only running
the request queue when this LUN is busy.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Reviewed-by: John Garry <john.garry@huawei.com>
Tested-by: Long Li <longli@microsoft.com>
Reported-by: Long Li <longli@microsoft.com>
Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V6:
        - patch style && comment change, as suggested by Bart	
        - add reviewed-by & tested-by tag
V5:
        - patch style && comment change, as suggested by Bart
        - add reviewed-by & tested-by tag
V4:
        - fix one race reported by Kashyap, and simplify the implementation
        a bit; also pass Kashyap's both function and performance test
V3:
        - add one smp_mb() in scsi_mq_get_budget() and comment
V2:
        - commit log change, no any code change
        - add reported-by tag

 drivers/scsi/scsi_lib.c    | 51 +++++++++++++++++++++++++++++++++++---
 include/scsi/scsi_device.h |  1 +
 2 files changed, 48 insertions(+), 4 deletions(-)

On 2020-09-09 00:29, Ming Lei wrote:
> +		if (old) {
> +			/*
> +			 * ->restarts has to be kept as non-zero if there is
> +			 *  new budget contention comes.
> +			 *
> +			 *  No need to run queue when either another re-run
> +			 *  queue wins in updating ->restarts or one new budget
> +			 *  contention comes.
> +			 */
> +			if (atomic_cmpxchg(&sdev->restarts, old, 0) == old)
> +				blk_mq_run_hw_queues(sdev->request_queue, true);

How about combining the above two if-statements into a single if-statement to
keep the indentation level low?

>  /* Returns false when no more bytes to process, true if there are more */
> @@ -1611,8 +1630,32 @@ static void scsi_mq_put_budget(struct request_queue *q)
>  static bool scsi_mq_get_budget(struct request_queue *q)
>  {
>  	struct scsi_device *sdev = q->queuedata;
> +	int ret = scsi_dev_queue_ready(q, sdev);
>  
> -	return scsi_dev_queue_ready(q, sdev);
> +	if (ret)
> +		return true;

I like Ewan's comment about the above code:

"I think this should just be:

	if (scsi_dev_queue_ready(q, sdev))
		return true;

There's no particular reason to call the function in a local variable
initializer, this just makes the code less clear to me.  And "ret"
isn't needed for any other reason."

> +	/*
> +	 * If all in-flight requests originated from this LUN are completed
> +	 * before setting .restarts, sdev->device_busy will be observed as
> +	 * zero, then blk_mq_delay_run_hw_queues() will dispatch this request
> +	 * soon. Otherwise, completion of one of these request will observe
> +	 * the .restarts flag, and the request queue will be run for handling
> +	 * this request, see scsi_end_request().
> +	 */
> +	if (unlikely(atomic_read(&sdev->device_busy) == 0 &&
> +				!scsi_device_blocked(sdev)))
> +		blk_mq_delay_run_hw_queues(sdev->request_queue, SCSI_QUEUE_DELAY);

The above comment doesn't explain what happens if the all pending SCSI
commands complete after .restarts has been incremented and before .device_busy
is read. Since I think that case is handled, consider changing "before setting
.restarts" into "before reading .device_busy".

Anyway, since I think the code is fine, feel free to add:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>