| Message ID | 20190402114800.2281-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCSI: don't hold device refcount in IO path | expand |
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.
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.
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
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.
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
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;
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(-)