diff mbox series

[V5,9/9] SCSI: don't hold device refcount in IO path

Message ID 20190412033032.10418-10-ming.lei@redhat.com (mailing list archive)
State Superseded
Headers show
Series blk-mq: fix races related with freeing queue | expand

Commit Message

Ming Lei April 12, 2019, 3:30 a.m. UTC 
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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

Comments

Hannes Reinecke April 12, 2019, 11:09 a.m. UTC | #1 
On 4/12/19 5:30 AM, 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>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/scsi/scsi_lib.c | 28 ++--------------------------
>   1 file changed, 2 insertions(+), 26 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
Martin K. Petersen April 13, 2019, 12:04 a.m. UTC | #2 
Ming,

> 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.

This all looks good to me. I assume it will go through Jens' tree?

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Ming Lei April 13, 2019, 6:56 a.m. UTC | #3 
On Fri, Apr 12, 2019 at 08:04:40PM -0400, Martin K. Petersen wrote:
> 
> Ming,
> 
> > 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.
> 
> This all looks good to me. I assume it will go through Jens' tree?
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

Hi Nartin,

This patch doesn't depend on patch 1~8, so please take it via scsi tree
if you are fine.


Thanks,
Ming
Ming Lei April 13, 2019, 9:23 a.m. UTC | #4 
On Sat, Apr 13, 2019 at 2:57 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Fri, Apr 12, 2019 at 08:04:40PM -0400, Martin K. Petersen wrote:
> >
> > Ming,
> >
> > > 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.
> >
> > This all looks good to me. I assume it will go through Jens' tree?
> >
> > Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>
> Hi Nartin,

Martin, sorry for the typo, :-(


Thanks,
Ming Lei
Martin K. Petersen April 16, 2019, 2:12 a.m. UTC | #5 
Ming,

> This patch doesn't depend on patch 1~8, so please take it via scsi
> tree if you are fine.

Applied, thanks!
diff mbox series

Patch

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;