[PATCH] SCSI: don't hold device refcount in IO path

[PATCH] SCSI: don't hold device refcount in IO path

[Ming Lei]

scsi_device's refcount is always grabed in IO path.

Turns out it isn't necessary, becasue blk_queue_cleanup() will
drain any in-flight IOs, then cancel timeout/requeue work, and
SCSI's requeue_work is canceled too in __scsi_remove_device().

Also scsi_device won't go away until blk_cleanup_queue() is done.

So don't hold the refcount in IO path.

Cc: Dongli Zhang <dongli.zhang@oracle.com>
Cc: James Smart <james.smart@broadcom.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>,
Cc: jianchao wang <jianchao.w.wang@oracle.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 601b9f1de267..a095426b1c7a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -141,8 +141,6 @@ scsi_set_blocked(struct scsi_cmnd *cmd, int reason)
 
 static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd)
 {
-	struct scsi_device *sdev = cmd->device;
-
 	if (cmd->request->rq_flags & RQF_DONTPREP) {
 		cmd->request->rq_flags &= ~RQF_DONTPREP;
 		scsi_mq_uninit_cmd(cmd);
@@ -150,7 +148,6 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd)
 		WARN_ON_ONCE(true);
 	}
 	blk_mq_requeue_request(cmd->request, true);
-	put_device(&sdev->sdev_gendev);
 }
 
 /**
@@ -201,7 +198,6 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
 	 * happened.
 	 */
 	blk_mq_requeue_request(cmd->request, true);
-	put_device(&device->sdev_gendev);
 }
 
 /*
@@ -619,7 +615,6 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
 		blk_mq_run_hw_queues(q, true);
 
 	percpu_ref_put(&q->q_usage_counter);
-	put_device(&sdev->sdev_gendev);
 	return false;
 }
 
@@ -1613,7 +1608,6 @@ static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)
 	struct scsi_device *sdev = q->queuedata;
 
 	atomic_dec(&sdev->device_busy);
-	put_device(&sdev->sdev_gendev);
 }
 
 static bool scsi_mq_get_budget(struct blk_mq_hw_ctx *hctx)
@@ -1621,16 +1615,9 @@ static bool scsi_mq_get_budget(struct blk_mq_hw_ctx *hctx)
 	struct request_queue *q = hctx->queue;
 	struct scsi_device *sdev = q->queuedata;
 
-	if (!get_device(&sdev->sdev_gendev))
-		goto out;
-	if (!scsi_dev_queue_ready(q, sdev))
-		goto out_put_device;
-
-	return true;
+	if (scsi_dev_queue_ready(q, sdev))
+		return true;
 
-out_put_device:
-	put_device(&sdev->sdev_gendev);
-out:
 	if (atomic_read(&sdev->device_busy) == 0 && !scsi_device_blocked(sdev))
 		blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
 	return false;
-- 
2.9.5

Re: [PATCH] SCSI: don't hold device refcount in IO path

[Bart Van Assche]

On Tue, 2019-04-02 at 19:48 +0800, Ming Lei wrote:
> @@ -201,7 ퟟ,6 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
>  	 * happened.
>  	 */
>  	blk_mq_requeue_request(cmd->request, true);
> -	put_device(&device->sdev_gendev);
>  }

The word "happened" is the last word of a long comment that explains why
the put_device() call is necessary. Please update this patch such that it
also removes that comment.

Thanks,

Bart.

Re: [PATCH] SCSI: don't hold device refcount in IO path

[Bart Van Assche]

On Tue, 2019-04-02 at 19:48 +0800, Ming Lei wrote:
> scsi_device's refcount is always grabed in IO path.
                                   ^^^^^^
                                   grabbed?

> Turns out it isn't necessary, becasue blk_queue_cleanup() will
> drain any in-flight IOs, then cancel timeout/requeue work, and
> SCSI's requeue_work is canceled too in __scsi_remove_device().
> 
> Also scsi_device won't go away until blk_cleanup_queue() is done.
> 
> So don't hold the refcount in IO path.

Holding the device reference count was definitely necessary in the past.
You may want to reflect this in the patch description by mentioning that
grabbing that reference count is no longer required today because the
draining mechanism now waits for requeuing to occur. I don't think that
was the case for the legacy block layer.

Bart.

Re: [PATCH] SCSI: don't hold device refcount in IO path

[Ming Lei]

On Wed, Apr 3, 2019 at 12:03 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Tue, 2019-04-02 at 19:48 +0800, Ming Lei wrote:
> > scsi_device's refcount is always grabed in IO path.
>                                    ^^^^^^
>                                    grabbed?
>
> > Turns out it isn't necessary, becasue blk_queue_cleanup() will
> > drain any in-flight IOs, then cancel timeout/requeue work, and
> > SCSI's requeue_work is canceled too in __scsi_remove_device().
> >
> > Also scsi_device won't go away until blk_cleanup_queue() is done.
> >
> > So don't hold the refcount in IO path.
>
> Holding the device reference count was definitely necessary in the past.
> You may want to reflect this in the patch description by mentioning that
> grabbing that reference count is no longer required today because the
> draining mechanism now waits for requeuing to occur. I don't think that
> was the case for the legacy block layer.

You must forget we have switched to blk_queue_enter() for legacy block
layer for a while, :-)


Thanks,
Ming Lei

Re: [PATCH] SCSI: don't hold device refcount in IO path

[Bart Van Assche]

On 4/2/19 8:41 PM, Ming Lei wrote:
> On Wed, Apr 3, 2019 at 12:03 AM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> On Tue, 2019-04-02 at 19:48 +0800, Ming Lei wrote:
>>> scsi_device's refcount is always grabed in IO path.
>>                                     ^^^^^^
>>                                     grabbed?
>>
>>> Turns out it isn't necessary, becasue blk_queue_cleanup() will
>>> drain any in-flight IOs, then cancel timeout/requeue work, and
>>> SCSI's requeue_work is canceled too in __scsi_remove_device().
>>>
>>> Also scsi_device won't go away until blk_cleanup_queue() is done.
>>>
>>> So don't hold the refcount in IO path.
>>
>> Holding the device reference count was definitely necessary in the past.
>> You may want to reflect this in the patch description by mentioning that
>> grabbing that reference count is no longer required today because the
>> draining mechanism now waits for requeuing to occur. I don't think that
>> was the case for the legacy block layer.
> 
> You must forget we have switched to blk_queue_enter() for legacy block
> layer for a while, :-)

I definitely haven't forgotten that. I was referring to the time before 
blk_queue_enter() / blk_queue_exit() was introduced in the legacy block 
layer.

Bart.

Re: [PATCH] SCSI: don't hold device refcount in IO path

[Dongli Zhang]

On 4/2/19 7:48 PM, Ming Lei wrote:
> scsi_device's refcount is always grabed in IO path.

'grabbed'

> 
> Turns out it isn't necessary, becasue blk_queue_cleanup() will

'because blk_cleanup_queue()'

> drain any in-flight IOs, then cancel timeout/requeue work, and
> SCSI's requeue_work is canceled too in __scsi_remove_device().

Dongli Zhang