On 2020-09-07 00:10, Ming Lei wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7affaaf8b98e..a05e431ee62a 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -551,8 +551,25 @@ static void scsi_run_queue_async(struct scsi_device *sdev)
> if (scsi_target(sdev)->single_lun ||
> !list_empty(&sdev->host->starved_list))
> kblockd_schedule_work(&sdev->requeue_work);
> - else
> - blk_mq_run_hw_queues(sdev->request_queue, true);
> + else {
Please follow the Linux kernel coding style and balance braces.
> + /*
> + * smp_mb() implied in either rq->end_io or blk_mq_free_request
> + * is for ordering writing .device_busy in scsi_device_unbusy()
> + * and reading sdev->restarts.
> + */
> + int old = atomic_read(&sdev->restarts);
scsi_run_queue_async() has two callers: scsi_end_request() and scsi_queue_rq().
I don't see how ordering between scsi_device_unbusy() and the above atomic_read()
could be guaranteed if this function is called from scsi_queue_rq()?
Regarding the I/O completion path, my understanding is that the I/O completion
path is as follows if rq->end_io == NULL:
scsi_mq_done()
blk_mq_complete_request()
rq->q->mq_ops->complete(rq) = scsi_softirq_done
scsi_finish_command()
scsi_device_unbusy()
scsi_cmd_to_driver(cmd)->done(cmd)
scsi_io_completion()
scsi_end_request()
blk_update_request()
scsi_mq_uninit_cmd()
__blk_mq_end_request()
blk_mq_free_request()
__blk_mq_free_request()
blk_queue_exit()
scsi_run_queue_async()
I haven't found any store memory barrier between the .device_busy change in
scsi_device_unbusy() and the scsi_run_queue_async() call? Did I perhaps overlook
something?
> + /*
> + * ->restarts has to be kept as non-zero if there new budget
> + * contention comes.
Please fix the grammar in the above sentence.
> + /*
> + * Order writing .restarts and reading .device_busy. Its pair is
> + * implied by __blk_mq_end_request() in scsi_end_request() for
> + * ordering writing .device_busy in scsi_device_unbusy() and
> + * reading .restarts.
> + */
> + smp_mb__after_atomic();
What does "its pair is implied" mean? Please make the above comment
unambiguous.
> + /*
> + * 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);
> + return false;
What will happen if all in-flight requests complete after
scsi_run_queue_async() has read .restarts and before it executes
atomic_cmpxchg()? Will that cause the queue to be run after a delay
although it should be run immediately?
Thanks,
Bart.
