| Message ID | 20190403102609.18707-7-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: fix races related with freeing queue | expand |
On Wed, 2019-04-03 at 18:26 +0800, Ming Lei wrote: > /* > - * Before a SCSI command is dispatched, > - * get_device(&sdev->sdev_gendev) is called and the host, > - * target and device busy counters are increased. Since > - * requeuing a request causes these actions to be repeated and > - * since scsi_device_unbusy() has already been called, > - * put_device(&device->sdev_gendev) must still be called. Call > - * put_device() after blk_mq_requeue_request() to avoid that > - * removal of the SCSI device can start before requeueing has > - * happened. > + * Before a SCSI command is dispatched, the host, target and > + * device busy counters are increased. Since requeuing a request > + * causes these actions to be repeated and since scsi_device_unbusy() > + * has already been called. > */ The last sentence of the new comment looks incomplete to me ("Since ..."). Thanks, Bart.
On Wed, Apr 03, 2019 at 09:24:54AM -0700, Bart Van Assche wrote: > On Wed, 2019-04-03 at 18:26 +0800, Ming Lei wrote: > > /* > > - * Before a SCSI command is dispatched, > > - * get_device(&sdev->sdev_gendev) is called and the host, > > - * target and device busy counters are increased. Since > > - * requeuing a request causes these actions to be repeated and > > - * since scsi_device_unbusy() has already been called, > > - * put_device(&device->sdev_gendev) must still be called. Call > > - * put_device() after blk_mq_requeue_request() to avoid that > > - * removal of the SCSI device can start before requeueing has > > - * happened. > > + * Before a SCSI command is dispatched, the host, target and > > + * device busy counters are increased. Since requeuing a request > > + * causes these actions to be repeated and since scsi_device_unbusy() > > + * has already been called. > > */ > > The last sentence of the new comment looks incomplete to me ("Since ..."). Given most of the previous comment is about get/put device, I'd suggest to remove the whole block given we are clean that get/put device isn't required in IO path. Will do that in V4 if no objections. Thanks, Ming
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 601b9f1de267..6772d2ae1357 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); } /** @@ -190,18 +187,12 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy) cmd->result = 0; /* - * Before a SCSI command is dispatched, - * get_device(&sdev->sdev_gendev) is called and the host, - * target and device busy counters are increased. Since - * requeuing a request causes these actions to be repeated and - * since scsi_device_unbusy() has already been called, - * put_device(&device->sdev_gendev) must still be called. Call - * put_device() after blk_mq_requeue_request() to avoid that - * removal of the SCSI device can start before requeueing has - * happened. + * Before a SCSI command is dispatched, the host, target and + * device busy counters are increased. Since requeuing a request + * causes these actions to be repeated and since scsi_device_unbusy() + * has already been called. */ blk_mq_requeue_request(cmd->request, true); - put_device(&device->sdev_gendev); } /* @@ -619,7 +610,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 +1603,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 +1610,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 grabbed in IO path. Turns out it isn't necessary, because 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, especially the refcount isn't required in IO path since blk_queue_enter() / blk_queue_exit() is introduced in the legacy block layer. Cc: Dongli Zhang <dongli.zhang@oracle.com> Cc: James Smart <james.smart@broadcom.com> Cc: Bart Van Assche <bart.vanassche@wdc.com> Cc: linux-scsi@vger.kernel.org, Cc: Martin K . Petersen <martin.petersen@oracle.com>, Cc: Christoph Hellwig <hch@lst.de>, Cc: James E . J . Bottomley <jejb@linux.vnet.ibm.com>, Cc: jianchao wang <jianchao.w.wang@oracle.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/scsi_lib.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-)