| Message ID | 20190404084320.24681-8-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: fix races related with freeing queue | expand |
On Thu, 2019-04-04 at 16:43 +0800, Ming Lei wrote: > 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 | 28 ++-------------------------- > 1 file changed, 2 insertions(+), 26 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 601b9f1de267..3369d66911eb 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); > } > > /** > @@ -189,19 +186,7 @@ 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. > - */ > blk_mq_requeue_request(cmd->request, true); > - put_device(&device->sdev_gendev); > } > > /* > @@ -619,7 +604,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 +1597,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 +1604,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; Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 601b9f1de267..3369d66911eb 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); } /** @@ -189,19 +186,7 @@ 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. - */ blk_mq_requeue_request(cmd->request, true); - put_device(&device->sdev_gendev); } /* @@ -619,7 +604,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 +1597,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 +1604,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 | 28 ++-------------------------- 1 file changed, 2 insertions(+), 26 deletions(-)