[PATCH v3 0/8] scsi: fixes for targets with many LUNs, and scsi_target_block rework

[PATCH v3 0/8] scsi: fixes for targets with many LUNs, and scsi_target_block rework

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This patch series addresses some issues we saw in a test setup
with a large number of SCSI LUNs. The first two patches simply
increase the number of available sg and bsg devices. 3-6 fix
a large delay we encountered between blocking a Fibre Channel
remote port and the dev_loss_tmo. 7-8 apply additional changes
to scsi_target_block(), as suggested in the review of the v2 series.

Changes v2 -> v3:
 - Split previous 3/3 into 4 separate patches as suggested by
   Christoph Hellwig.
 - Added 7/8 and 8/8, as suggested by Christoph and Bart van Assche.
 - Added s-o-b and reviewed-by tags.

Changes v1 -> v2:
 - call blk_mq_wait_quiesce_done() from scsi_target_block() to
   cover the case where BLK_MQ_F_BL*** SUBJECT HERE ***

Hannes Reinecke (2):
  bsg: increase number of devices
  scsi: sg: increase number of devices

Martin Wilck (6):
  scsi: merge scsi_internal_device_block() and device_block()
  scsi: call scsi_stop_queue() without state_mutex held
  scsi: don't wait for quiesce in scsi_stop_queue()
  scsi: don't wait for quiesce in scsi_device_block()
  scsi: have scsi_target_block() expect a scsi_target parent argument
  scsi: add Scsi_Host argument to scsi_target_block()

 block/bsg.c                         |  2 +-
 drivers/scsi/scsi_lib.c             | 72 +++++++++++++----------------
 drivers/scsi/scsi_transport_fc.c    |  2 +-
 drivers/scsi/scsi_transport_iscsi.c |  3 +-
 drivers/scsi/scsi_transport_srp.c   |  4 +-
 drivers/scsi/sg.c                   |  2 +-
 drivers/scsi/snic/snic_disc.c       |  2 +-
 include/scsi/scsi_device.h          |  2 +-
 8 files changed, 41 insertions(+), 48 deletions(-)

-- 
2.40.1

[PATCH v3 1/8] bsg: increase number of devices

[mwilck]

From: Hannes Reinecke <hare@suse.de>

Larger setups may need to allocate more than 32k bsg devices, so
increase the number of devices to the full range of minor device
numbers.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 block/bsg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bsg.c b/block/bsg.c
index 7eca43f33d7f..c53f24243bf2 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -36,7 +36,7 @@ static inline struct bsg_device *to_bsg_device(struct inode *inode)
 }
 
 #define BSG_DEFAULT_CMDS	64
-#define BSG_MAX_DEVS		32768
+#define BSG_MAX_DEVS		(1 << MINORBITS)
 
 static DEFINE_IDA(bsg_minor_ida);
 static struct class *bsg_class;
-- 
2.40.1

[PATCH v3 2/8] scsi: sg: increase number of devices

[mwilck]

From: Hannes Reinecke <hare@suse.de>

Larger setups may need to allocate more than 32k sg devices, so
increase the number of devices to the full range of minor device
numbers.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 037f8c98a6d3..6c04cf941dac 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -71,7 +71,7 @@ static int sg_proc_init(void);
 
 #define SG_ALLOW_DIO_DEF 0
 
-#define SG_MAX_DEVS 32768
+#define SG_MAX_DEVS (1 << MINORBITS)
 
 /* SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type
  * of sg_io_hdr::cmd_len can only represent 255. All SCSI commands greater
-- 
2.40.1

[PATCH v3 3/8] scsi: merge scsi_internal_device_block() and device_block()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

scsi_internal_device_block() is only called from device_block().
Merge the two functions, and call the result scsi_device_block(),
as the name device_block() is confusingly generic.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 25489fbd94c6..ce5788643011 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2776,13 +2776,12 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev)
 EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
 
 /**
- * scsi_internal_device_block - try to transition to the SDEV_BLOCK state
+ * scsi_device_block - try to transition to the SDEV_BLOCK state
  * @sdev: device to block
+ * @data: dummy argument, ignored
  *
  * Pause SCSI command processing on the specified device and wait until all
- * ongoing scsi_request_fn() / scsi_queue_rq() calls have finished. May sleep.
- *
- * Returns zero if successful or a negative error code upon failure.
+ * ongoing scsi_queue_rq() calls have finished. May sleep.
  *
  * Note:
  * This routine transitions the device to the SDEV_BLOCK state (which must be
@@ -2790,7 +2789,7 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
  * is paused until the device leaves the SDEV_BLOCK state. See also
  * scsi_internal_device_unblock().
  */
-static int scsi_internal_device_block(struct scsi_device *sdev)
+static void scsi_device_block(struct scsi_device *sdev, void *data)
 {
 	int err;
 
@@ -2800,7 +2799,8 @@ static int scsi_internal_device_block(struct scsi_device *sdev)
 		scsi_stop_queue(sdev, false);
 	mutex_unlock(&sdev->state_mutex);
 
-	return err;
+	WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s) failed: err = %d\n",
+		  dev_name(&sdev->sdev_gendev), err);
 }
 
 /**
@@ -2883,23 +2883,12 @@ static int scsi_internal_device_unblock(struct scsi_device *sdev,
 	return ret;
 }
 
-static void
-device_block(struct scsi_device *sdev, void *data)
-{
-	int ret;
-
-	ret = scsi_internal_device_block(sdev);
-
-	WARN_ONCE(ret, "scsi_internal_device_block(%s) failed: ret = %d\n",
-		  dev_name(&sdev->sdev_gendev), ret);
-}
-
 static int
 target_block(struct device *dev, void *data)
 {
 	if (scsi_is_target_device(dev))
 		starget_for_each_device(to_scsi_target(dev), NULL,
-					device_block);
+					scsi_device_block);
 	return 0;
 }
 
@@ -2908,7 +2897,7 @@ scsi_target_block(struct device *dev)
 {
 	if (scsi_is_target_device(dev))
 		starget_for_each_device(to_scsi_target(dev), NULL,
-					device_block);
+					scsi_device_block);
 	else
 		device_for_each_child(dev, NULL, target_block);
 }
-- 
2.40.1

[PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[mwilck]

From: Martin Wilck <mwilck@suse.com>

sdev->state_mutex protects only sdev->sdev_state. There's no reason
to keep it held while calling scsi_stop_queue().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ce5788643011..26e7ce25fa05 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2795,9 +2795,9 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
 
 	mutex_lock(&sdev->state_mutex);
 	err = __scsi_internal_device_block_nowait(sdev);
+	mutex_unlock(&sdev->state_mutex);
 	if (err == 0)
 		scsi_stop_queue(sdev, false);
-	mutex_unlock(&sdev->state_mutex);
 
 	WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s) failed: err = %d\n",
 		  dev_name(&sdev->sdev_gendev), err);
-- 
2.40.1

[PATCH v3 5/8] scsi: don't wait for quiesce in scsi_stop_queue()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

scsi_stop_queue() has just two callers, one with and one without
"nowait". As blk_mq_quiesce_queue() comes down to
blk_mq_quiesce_queue_nowait() followed by blk_mq_wait_quiesce_done(),
we might as well open-code this in scsi_device_block().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 26e7ce25fa05..657d10c57205 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2726,24 +2726,16 @@ void scsi_start_queue(struct scsi_device *sdev)
 		blk_mq_unquiesce_queue(sdev->request_queue);
 }
 
-static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
+static void scsi_stop_queue(struct scsi_device *sdev)
 {
 	/*
 	 * The atomic variable of ->queue_stopped covers that
 	 * blk_mq_quiesce_queue* is balanced with blk_mq_unquiesce_queue.
 	 *
-	 * However, we still need to wait until quiesce is done
-	 * in case that queue has been stopped.
+	 * After return, we still need to wait until quiesce is done.
 	 */
-	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
-		if (nowait)
-			blk_mq_quiesce_queue_nowait(sdev->request_queue);
-		else
-			blk_mq_quiesce_queue(sdev->request_queue);
-	} else {
-		if (!nowait)
-			blk_mq_wait_quiesce_done(sdev->request_queue->tag_set);
-	}
+	if (!cmpxchg(&sdev->queue_stopped, 0, 1))
+		blk_mq_quiesce_queue_nowait(sdev->request_queue);
 }
 
 /**
@@ -2770,7 +2762,7 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	 * request queue.
 	 */
 	if (!ret)
-		scsi_stop_queue(sdev, true);
+		scsi_stop_queue(sdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
@@ -2796,8 +2788,10 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
 	mutex_lock(&sdev->state_mutex);
 	err = __scsi_internal_device_block_nowait(sdev);
 	mutex_unlock(&sdev->state_mutex);
-	if (err == 0)
-		scsi_stop_queue(sdev, false);
+	if (err == 0) {
+		scsi_stop_queue(sdev);
+		blk_mq_wait_quiesce_done(sdev->request_queue->tag_set);
+	}
 
 	WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s) failed: err = %d\n",
 		  dev_name(&sdev->sdev_gendev), err);
-- 
2.40.1

[PATCH v3 6/8] scsi: don't wait for quiesce in scsi_device_block()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

scsi_device_block() is only called from scsi_target_block(), which
calls it repeatedly for every child device. For targets with many devices,
waiting for every queue to quiesce may cause a substantial delay
(we measured more than 100s delay for blocking a FC rport with 2048 LUNs).

Just call blk_mq_wait_quiesce_done() once from scsi_target_block() after
stopping all queues.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 657d10c57205..25ec6eb8df7f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2772,8 +2772,8 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
  * @sdev: device to block
  * @data: dummy argument, ignored
  *
- * Pause SCSI command processing on the specified device and wait until all
- * ongoing scsi_queue_rq() calls have finished. May sleep.
+ * Pause SCSI command processing on the specified device. Callers must wait until all
+ * ongoing scsi_queue_rq() calls have finished after this function returns.
  *
  * Note:
  * This routine transitions the device to the SDEV_BLOCK state (which must be
@@ -2788,10 +2788,8 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
 	mutex_lock(&sdev->state_mutex);
 	err = __scsi_internal_device_block_nowait(sdev);
 	mutex_unlock(&sdev->state_mutex);
-	if (err == 0) {
+	if (err == 0)
 		scsi_stop_queue(sdev);
-		blk_mq_wait_quiesce_done(sdev->request_queue->tag_set);
-	}
 
 	WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s) failed: err = %d\n",
 		  dev_name(&sdev->sdev_gendev), err);
@@ -2889,11 +2887,15 @@ target_block(struct device *dev, void *data)
 void
 scsi_target_block(struct device *dev)
 {
+	struct Scsi_Host *shost = dev_to_shost(dev);
+
 	if (scsi_is_target_device(dev))
 		starget_for_each_device(to_scsi_target(dev), NULL,
 					scsi_device_block);
 	else
 		device_for_each_child(dev, NULL, target_block);
+
+	blk_mq_wait_quiesce_done(&shost->tag_set);
 }
 EXPORT_SYMBOL_GPL(scsi_target_block);
 
-- 
2.40.1

[PATCH v3 7/8] scsi: have scsi_target_block() expect a scsi_target parent argument

[mwilck]

From: Martin Wilck <mwilck@suse.com>

All callers (fc_remote_port_delete(), __iscsi_block_session(),
__srp_start_tl_fail_timers(), srp_reconnect_rport(), snic_tgt_del()) pass
parent devices of scsi_target devices to scsi_target_block().
Simplify scsi_target_block() to assume that it is always passed a parent
device.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 25ec6eb8df7f..e572fc56a8dd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2884,17 +2884,25 @@ target_block(struct device *dev, void *data)
 	return 0;
 }
 
+/**
+ * scsi_target_block - transition all SCSI child devices to SDEV_BLOCK state
+ * @dev: a parent device of one or more scsi_target devices
+ *
+ * Iterate over all children of @dev, which should be scsi_target devices,
+ * and switch all subordinate scsi devices to SDEV_BLOCK state. Wait for
+ * ongoing scsi_queue_rq() calls to finish. May sleep.
+ *
+ * Returns zero if successful or a negative error code upon failure.
+ *
+ * Note:
+ * @dev must not itself be a scsi_target device.
+ */
 void
 scsi_target_block(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev);
 
-	if (scsi_is_target_device(dev))
-		starget_for_each_device(to_scsi_target(dev), NULL,
-					scsi_device_block);
-	else
-		device_for_each_child(dev, NULL, target_block);
-
+	device_for_each_child(dev, NULL, target_block);
 	blk_mq_wait_quiesce_done(&shost->tag_set);
 }
 EXPORT_SYMBOL_GPL(scsi_target_block);
-- 
2.40.1

[PATCH v3 8/8] scsi: add Scsi_Host argument to scsi_target_block()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Instead of deriving the Scsi_Host from dev_to_shost, require
callers to pass in the Scsi_Host the given device belongs to.

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Martin Wilck <mwilck@suse.com>
Cc: Karan Tilak Kumar <kartilak@cisco.com>
Cc: Sesidhar Baddela <sebaddel@cisco.com>
---
 drivers/scsi/scsi_lib.c             | 5 ++---
 drivers/scsi/scsi_transport_fc.c    | 2 +-
 drivers/scsi/scsi_transport_iscsi.c | 3 ++-
 drivers/scsi/scsi_transport_srp.c   | 4 ++--
 drivers/scsi/snic/snic_disc.c       | 2 +-
 include/scsi/scsi_device.h          | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e572fc56a8dd..b89d69a5bab0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2887,6 +2887,7 @@ target_block(struct device *dev, void *data)
 /**
  * scsi_target_block - transition all SCSI child devices to SDEV_BLOCK state
  * @dev: a parent device of one or more scsi_target devices
+ * @shost: the Scsi_Host to which this device belongs
  *
  * Iterate over all children of @dev, which should be scsi_target devices,
  * and switch all subordinate scsi devices to SDEV_BLOCK state. Wait for
@@ -2898,10 +2899,8 @@ target_block(struct device *dev, void *data)
  * @dev must not itself be a scsi_target device.
  */
 void
-scsi_target_block(struct device *dev)
+scsi_target_block(struct device *dev, struct Scsi_Host *shost)
 {
-	struct Scsi_Host *shost = dev_to_shost(dev);
-
 	device_for_each_child(dev, NULL, target_block);
 	blk_mq_wait_quiesce_done(&shost->tag_set);
 }
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 64ff2629eaf9..3155565e66f1 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3451,7 +3451,7 @@ fc_remote_port_delete(struct fc_rport  *rport)
 
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	scsi_target_block(&rport->dev);
+	scsi_target_block(&rport->dev, shost);
 
 	/* see if we need to kill io faster than waiting for device loss */
 	if ((rport->fast_io_fail_tmo != -1) &&
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index b9b97300e3b3..9f2f4c9403de 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1943,13 +1943,14 @@ static void __iscsi_block_session(struct work_struct *work)
 	struct iscsi_cls_session *session =
 			container_of(work, struct iscsi_cls_session,
 				     block_work);
+	struct Scsi_Host *shost = iscsi_session_to_shost(session);
 	unsigned long flags;
 
 	ISCSI_DBG_TRANS_SESSION(session, "Blocking session\n");
 	spin_lock_irqsave(&session->lock, flags);
 	session->state = ISCSI_SESSION_FAILED;
 	spin_unlock_irqrestore(&session->lock, flags);
-	scsi_target_block(&session->dev);
+	scsi_target_block(&session->dev, shost);
 	ISCSI_DBG_TRANS_SESSION(session, "Completed SCSI target blocking\n");
 	if (session->recovery_tmo >= 0)
 		queue_delayed_work(session->workq,
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 87d0fb8dc503..8219180a7b58 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -480,7 +480,7 @@ static void __srp_start_tl_fail_timers(struct srp_rport *rport)
 	    srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) {
 		pr_debug("%s new state: %d\n", dev_name(&shost->shost_gendev),
 			 rport->state);
-		scsi_target_block(&shost->shost_gendev);
+		scsi_target_block(&shost->shost_gendev, shost);
 		if (fast_io_fail_tmo >= 0)
 			queue_delayed_work(system_long_wq,
 					   &rport->fast_io_fail_work,
@@ -548,7 +548,7 @@ int srp_reconnect_rport(struct srp_rport *rport)
 		 * later is ok though, scsi_internal_device_unblock_nowait()
 		 * treats SDEV_TRANSPORT_OFFLINE like SDEV_BLOCK.
 		 */
-		scsi_target_block(&shost->shost_gendev);
+		scsi_target_block(&shost->shost_gendev, shost);
 	res = rport->state != SRP_RPORT_LOST ? i->f->reconnect(rport) : -ENODEV;
 	pr_debug("%s (state %d): transport.reconnect() returned %d\n",
 		 dev_name(&shost->shost_gendev), rport->state, res);
diff --git a/drivers/scsi/snic/snic_disc.c b/drivers/scsi/snic/snic_disc.c
index 8fbf3c1b1311..3749853439ac 100644
--- a/drivers/scsi/snic/snic_disc.c
+++ b/drivers/scsi/snic/snic_disc.c
@@ -214,7 +214,7 @@ snic_tgt_del(struct work_struct *work)
 		scsi_flush_work(shost);
 
 	/* Block IOs on child devices, stops new IOs */
-	scsi_target_block(&tgt->dev);
+	scsi_target_block(&tgt->dev, shost);
 
 	/* Cleanup IOs */
 	snic_tgt_scsi_abort_io(tgt);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index b2cdb078b7bd..9fd76b71b533 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -456,7 +456,7 @@ extern void scsi_scan_target(struct device *parent, unsigned int channel,
 			     unsigned int id, u64 lun,
 			     enum scsi_scan_mode rescan);
 extern void scsi_target_reap(struct scsi_target *);
-extern void scsi_target_block(struct device *);
+extern void scsi_target_block(struct device *, struct Scsi_Host *);
 extern void scsi_target_unblock(struct device *, enum scsi_device_state);
 extern void scsi_remove_target(struct device *);
 extern const char *scsi_device_state_name(enum scsi_device_state);
-- 
2.40.1

Re: [PATCH v3 3/8] scsi: merge scsi_internal_device_block() and device_block()

[Bart Van Assche]

On 6/7/23 11:22, mwilck@suse.com wrote:
> scsi_internal_device_block() is only called from device_block().
> Merge the two functions, and call the result scsi_device_block(),
> as the name device_block() is confusingly generic.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Bart Van Assche]

On 6/7/23 11:22, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> sdev->state_mutex protects only sdev->sdev_state. There's no reason
> to keep it held while calling scsi_stop_queue().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/scsi/scsi_lib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ce5788643011..26e7ce25fa05 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2795,9 +2795,9 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
>   
>   	mutex_lock(&sdev->state_mutex);
>   	err = __scsi_internal_device_block_nowait(sdev);
> +	mutex_unlock(&sdev->state_mutex);
>   	if (err == 0)
>   		scsi_stop_queue(sdev, false);
> -	mutex_unlock(&sdev->state_mutex);
>   
>   	WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s) failed: err = %d\n",
>   		  dev_name(&sdev->sdev_gendev), err);

There is a reason why scsi_stop_queue() is called with the sdev state 
mutex held: if this mutex is not held, unblocking of a SCSI device can 
start before the scsi_stop_queue() call has finished. It is not allowed 
to swap the order of the blk_mq_quiesce_queue() and 
blk_mq_unquiesce_queue() calls.

Bart.

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Martin Wilck]

On Wed, 2023-06-07 at 12:16 -0700, Bart Van Assche wrote:
> On 6/7/23 11:22, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > sdev->state_mutex protects only sdev->sdev_state. There's no reason
> > to keep it held while calling scsi_stop_queue().
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >   drivers/scsi/scsi_lib.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index ce5788643011..26e7ce25fa05 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -2795,9 +2795,9 @@ static void scsi_device_block(struct
> > scsi_device *sdev, void *data)
> >   
> >         mutex_lock(&sdev->state_mutex);
> >         err = __scsi_internal_device_block_nowait(sdev);
> > +       mutex_unlock(&sdev->state_mutex);
> >         if (err == 0)
> >                 scsi_stop_queue(sdev, false);
> > -       mutex_unlock(&sdev->state_mutex);
> >   
> >         WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s)
> > failed: err = %d\n",
> >                   dev_name(&sdev->sdev_gendev), err);
> 
> There is a reason why scsi_stop_queue() is called with the sdev state
> mutex held: if this mutex is not held, unblocking of a SCSI device
> can 
> start before the scsi_stop_queue() call has finished. It is not
> allowed 
> to swap the order of the blk_mq_quiesce_queue() and 
> blk_mq_unquiesce_queue() calls.

Thanks. This wasn't obvious to me from the current code. I'll add a
comment in the next version.

Regards
Martin

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Martin Wilck]

Bart,

On Wed, 2023-06-07 at 21:37 +0200, Martin Wilck wrote:
> On Wed, 2023-06-07 at 12:16 -0700, Bart Van Assche wrote:
> > On 6/7/23 11:22, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > sdev->state_mutex protects only sdev->sdev_state. There's no
> > > reason
> > > to keep it held while calling scsi_stop_queue().
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >   drivers/scsi/scsi_lib.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index ce5788643011..26e7ce25fa05 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -2795,9 +2795,9 @@ static void scsi_device_block(struct
> > > scsi_device *sdev, void *data)
> > >   
> > >         mutex_lock(&sdev->state_mutex);
> > >         err = __scsi_internal_device_block_nowait(sdev);
> > > +       mutex_unlock(&sdev->state_mutex);
> > >         if (err == 0)
> > >                 scsi_stop_queue(sdev, false);
> > > -       mutex_unlock(&sdev->state_mutex);
> > >   
> > >         WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s)
> > > failed: err = %d\n",
> > >                   dev_name(&sdev->sdev_gendev), err);
> > 
> > There is a reason why scsi_stop_queue() is called with the sdev
> > state
> > mutex held: if this mutex is not held, unblocking of a SCSI device
> > can 
> > start before the scsi_stop_queue() call has finished. It is not
> > allowed 
> > to swap the order of the blk_mq_quiesce_queue() and 
> > blk_mq_unquiesce_queue() calls.
> 
> Thanks. This wasn't obvious to me from the current code. I'll add a
> comment in the next version.

The crucial question is now, is it sufficient to call
blk_mq_quiesce_queue_nowait() under the mutex, or does the call to
blk_mq_wait_quiesce_done() have to be under the mutex, too?
The latter would actually kill off our attempt to fix the delay
in fc_remote_port_delete() that was caused by repeated
synchronize_rcu() calls.

But if I understand you correctly, moving the wait out of the mutex
should be ok. I'll update the series accordingly.

Thanks,
Martin

Re: [PATCH v3 3/8] scsi: merge scsi_internal_device_block() and device_block()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Christoph Hellwig]

> > Thanks. This wasn't obvious to me from the current code. I'll add a
> > comment in the next version.
> 
> The crucial question is now, is it sufficient to call
> blk_mq_quiesce_queue_nowait() under the mutex, or does the call to
> blk_mq_wait_quiesce_done() have to be under the mutex, too?
> The latter would actually kill off our attempt to fix the delay
> in fc_remote_port_delete() that was caused by repeated
> synchronize_rcu() calls.
> 
> But if I understand you correctly, moving the wait out of the mutex
> should be ok. I'll update the series accordingly.

I can't think of a reason we'd want to lock over the wait, but Bart
knows this code way better than I do.

Re: [PATCH v3 5/8] scsi: don't wait for quiesce in scsi_stop_queue()

[Christoph Hellwig]

Looks good.  Per the discussion on the last patch it probably makes
sense to move this patch before the replacement for patch 2.

Btw, your mail address for Bart is very, very outdated.

Re: [PATCH v3 7/8] scsi: have scsi_target_block() expect a scsi_target parent argument

[Christoph Hellwig]

On Wed, Jun 07, 2023 at 08:22:48PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> All callers (fc_remote_port_delete(), __iscsi_block_session(),
> __srp_start_tl_fail_timers(), srp_reconnect_rport(), snic_tgt_del()) pass
> parent devices of scsi_target devices to scsi_target_block().
> Simplify scsi_target_block() to assume that it is always passed a parent
> device.

Btw, one thing I realized is that the function has been a bit misnamed
for a while, and becomes even more so now.  Maybe scsi_block_targets
is a better name as it blocks all the targets hanging off the
passed in devices as children?

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Bart Van Assche]

On 6/7/23 22:44, Christoph Hellwig wrote:
>>> Thanks. This wasn't obvious to me from the current code. I'll add a
>>> comment in the next version.
>>
>> The crucial question is now, is it sufficient to call
>> blk_mq_quiesce_queue_nowait() under the mutex, or does the call to
>> blk_mq_wait_quiesce_done() have to be under the mutex, too?
>> The latter would actually kill off our attempt to fix the delay
>> in fc_remote_port_delete() that was caused by repeated
>> synchronize_rcu() calls.
>>
>> But if I understand you correctly, moving the wait out of the mutex
>> should be ok. I'll update the series accordingly.
> 
> I can't think of a reason we'd want to lock over the wait, but Bart
> knows this code way better than I do.

Unless deep changes would be made in the block layer 
blk_mq_quiesce_queue_nowait() and/or blk_mq_wait_quiesce_done() 
functions, moving blk_mq_wait_quiesce_done() outside the critical 
section seems fine to me.

Thanks,

Bart.

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Mike Christie]

On 6/8/23 9:12 AM, Bart Van Assche wrote:
> On 6/7/23 22:44, Christoph Hellwig wrote:
>>>> Thanks. This wasn't obvious to me from the current code. I'll add a
>>>> comment in the next version.
>>>
>>> The crucial question is now, is it sufficient to call
>>> blk_mq_quiesce_queue_nowait() under the mutex, or does the call to
>>> blk_mq_wait_quiesce_done() have to be under the mutex, too?
>>> The latter would actually kill off our attempt to fix the delay
>>> in fc_remote_port_delete() that was caused by repeated
>>> synchronize_rcu() calls.
>>>
>>> But if I understand you correctly, moving the wait out of the mutex
>>> should be ok. I'll update the series accordingly.
>>
>> I can't think of a reason we'd want to lock over the wait, but Bart
>> knows this code way better than I do.
> 
> Unless deep changes would be made in the block layer blk_mq_quiesce_queue_nowait() and/or blk_mq_wait_quiesce_done() functions, moving blk_mq_wait_quiesce_done() outside the critical section seems fine to me.

If it helps, I tested with iscsi and the mutex changes discussed above
and it worked ok for me.

Also thanks for fixing this Martin.

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Martin Wilck]

On Thu, 2023-06-08 at 13:54 -0500, Mike Christie wrote:
> On 6/8/23 9:12 AM, Bart Van Assche wrote:
> > On 6/7/23 22:44, Christoph Hellwig wrote:
> > > > > Thanks. This wasn't obvious to me from the current code. I'll
> > > > > add a
> > > > > comment in the next version.
> > > > 
> > > > The crucial question is now, is it sufficient to call
> > > > blk_mq_quiesce_queue_nowait() under the mutex, or does the call
> > > > to
> > > > blk_mq_wait_quiesce_done() have to be under the mutex, too?
> > > > The latter would actually kill off our attempt to fix the delay
> > > > in fc_remote_port_delete() that was caused by repeated
> > > > synchronize_rcu() calls.
> > > > 
> > > > But if I understand you correctly, moving the wait out of the
> > > > mutex
> > > > should be ok. I'll update the series accordingly.
> > > 
> > > I can't think of a reason we'd want to lock over the wait, but
> > > Bart
> > > knows this code way better than I do.
> > 
> > Unless deep changes would be made in the block layer
> > blk_mq_quiesce_queue_nowait() and/or blk_mq_wait_quiesce_done()
> > functions, moving blk_mq_wait_quiesce_done() outside the critical
> > section seems fine to me.
> 
> If it helps, I tested with iscsi and the mutex changes discussed
> above
> and it worked ok for me.

I guess the race that Bart was hinting at is hard to trigger.

I would like to remark that the fact that we need to hold the SCSI
state_mutex while calling blk_mq_quiesce_queue_nowait() looks like a
layering issue to me. Not sure if, and how, this could be avoided,
though.

Regards
Martin

Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Bart Van Assche]

On 6/12/23 04:15, Martin Wilck wrote:
> I guess the race that Bart was hinting at is hard to trigger.

Are you sure about this? I think this scenario can be triggered by 
writing into the sysfs attribute that changes the SCSI device state 
while a scsi_target_block() call is in progress. See also 
store_state_field().

> I would like to remark that the fact that we need to hold the SCSI
> state_mutex while calling blk_mq_quiesce_queue_nowait() looks like a
> layering issue to me. Not sure if, and how, this could be avoided,
> though.

I do not agree that this is a layering issue. Is holding a mutex around 
a call of a function in a lower layer ever a layering issue?

What matters is to be very careful with locks while invoking callback 
functions. See also slide 7 in Ousterhout's presentation "Why Threads 
Are A Bad Idea (for most purposes)" from 1996 
(https://web.stanford.edu/~ouster/cgi-bin/papers/threads.pdf).

Bart.