[PATCH 0/5] block/target queue/LUN reset support

[PATCH 0/5] block/target queue/LUN reset support

[mchristi]

Currently, for SCSI LUN_RESETs the target layer can only wait on
bio/requests it has sent. This normally results in the LUN_RESET
timing out on the initiator side and that SCSI error handler
escalating to something more disruptive.

To fix this, the following patches add a block layer helper and
callout to reset a request queue which the target layer can use
to force drivers to complete/fail executing requests.

Patches were made over Jens's block tree's for-next branch.

[PATCH 1/5] blk mq: take ref to q when running it

[mchristi]

From: Mike Christie <mchristi@redhat.com>

If the __blk_mq_run_hw_queue is a result of a async run or retry
then we do not have a reference to the queue. At this time,
blk_cleanup_queue could begin tearing it down while the queue_rq
function is being run.

This patch just has us do a blk_queue_enter call.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 block/blk-mq.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7df9c92..5958a02 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -738,8 +738,13 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 
 	WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask));
 
-	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
+	if (blk_queue_enter(q, false)) {
+		blk_mq_run_hw_queue(hctx, true);
 		return;
+	}
+
+	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
+		goto exit_queue;
 
 	hctx->run++;
 
@@ -832,6 +837,9 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 		 **/
 		blk_mq_run_hw_queue(hctx, true);
 	}
+
+exit_queue:
+	blk_queue_exit(q);
 }
 
 /*
-- 
2.7.2

[PATCH 2/5] block: add queue reset support

[mchristi]

From: Mike Christie <mchristi@redhat.com>

This adds a request_queue/mq_ops callout which when called
should force the completion/failure of requests that have been
dequeued by the driver. Requests can be completed normally
or should be failed with -EINTR.

On success the reset callout should complete/fail dequeued
requests and then return BLK_EH_HANDLED.

If the reset callout fails, it should return BLK_EH_NOT_HANDLED.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 block/blk-core.c       |  8 ++++++
 block/blk-settings.c   |  6 +++++
 block/blk-timeout.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blk-mq.h |  5 ++++
 include/linux/blkdev.h |  8 ++++++
 5 files changed, 95 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2475b1c7..2aeac9c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -575,6 +575,9 @@ void blk_cleanup_queue(struct request_queue *q)
 	if (!q->mq_ops)
 		__blk_drain_queue(q, true);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
+
+	/* wait for resets that might have started as result of drain */
+	wait_event_lock_irq(q->reset_wq, !blk_queue_resetting(q), *lock);
 	spin_unlock_irq(lock);
 
 	/* for synchronous bio-based driver finish in-flight integrity i/o */
@@ -728,6 +731,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 
 	kobject_init(&q->kobj, &blk_queue_ktype);
 
+	init_waitqueue_head(&q->reset_wq);
 	mutex_init(&q->sysfs_lock);
 	spin_lock_init(&q->__queue_lock);
 
@@ -850,6 +854,7 @@ blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
 
 	INIT_WORK(&q->timeout_work, blk_timeout_work);
 	q->request_fn		= rfn;
+	q->reset_fn		= NULL,
 	q->prep_rq_fn		= NULL;
 	q->unprep_rq_fn		= NULL;
 	q->queue_flags		|= QUEUE_FLAG_DEFAULT;
@@ -2619,6 +2624,9 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
 		case -ENODATA:
 			error_type = "critical medium";
 			break;
+		case -EINTR:
+			error_type = "critical command";
+			break;
 		case -EIO:
 		default:
 			error_type = "I/O";
diff --git a/block/blk-settings.c b/block/blk-settings.c
index f679ae1..1d529ba 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -71,6 +71,12 @@ void blk_queue_rq_timed_out(struct request_queue *q, rq_timed_out_fn *fn)
 }
 EXPORT_SYMBOL_GPL(blk_queue_rq_timed_out);
 
+void blk_queue_reset(struct request_queue *q, reset_fn *fn)
+{
+	q->reset_fn = fn;
+}
+EXPORT_SYMBOL_GPL(blk_queue_reset);
+
 void blk_queue_lld_busy(struct request_queue *q, lld_busy_fn *fn)
 {
 	q->lld_busy_fn = fn;
diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index a30441a..96b73786 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -5,6 +5,7 @@
 #include <linux/module.h>
 #include <linux/blkdev.h>
 #include <linux/fault-inject.h>
+#include <linux/delay.h>
 
 #include "blk.h"
 #include "blk-mq.h"
@@ -172,6 +173,73 @@ void blk_abort_request(struct request *req)
 }
 EXPORT_SYMBOL_GPL(blk_abort_request);
 
+/**
+ * blk_reset_queue - force completion of requests executing in queue
+ * @q:		request queue to reset
+ *
+ * On success the driver returns BLK_EH_HANDLED from the callout and
+ * either complete requests successfully with 0 or if abnormally completed
+ * with the error code -EINTR.
+ *
+ * On failure the driver returns BLK_EH_NOT_HANDLED, and requests may still
+ * be executing.
+ */
+int blk_reset_queue(struct request_queue *q)
+{
+	enum blk_eh_timer_return eh_rc;
+	int rc;
+
+	spin_lock_irq(q->queue_lock);
+	wait_event_lock_irq(q->reset_wq,
+			    !queue_flag_test_and_set(QUEUE_FLAG_RESETTING, q),
+			    *q->queue_lock);
+	if (blk_queue_dead(q)) {
+		rc = -ENODEV;
+		spin_unlock_irq(q->queue_lock);
+		goto done;
+	}
+	spin_unlock_irq(q->queue_lock);
+
+	if (q->mq_ops) {
+		blk_mq_stop_hw_queues(q);
+		blk_mq_freeze_queue(q);
+
+		eh_rc = q->mq_ops->reset(q);
+
+		blk_mq_unfreeze_queue(q);
+		blk_mq_start_stopped_hw_queues(q, true);
+	} else if (q->reset_fn) {
+		spin_lock_irq(q->queue_lock);
+		blk_stop_queue(q);
+		spin_unlock_irq(q->queue_lock);
+
+		while (q->request_fn_active)
+			msleep(10);
+
+		eh_rc = q->reset_fn(q);
+
+		spin_lock_irq(q->queue_lock);
+		blk_start_queue(q);
+		spin_unlock_irq(q->queue_lock);
+	} else {
+		rc = -EOPNOTSUPP;
+		goto done;
+	}
+
+	if (eh_rc == BLK_EH_HANDLED)
+		rc = 0;
+	else
+		rc = -EIO;
+
+done:
+	spin_lock_irq(q->queue_lock);
+	queue_flag_clear(QUEUE_FLAG_RESETTING, q);
+	spin_unlock_irq(q->queue_lock);
+	wake_up_all(&q->reset_wq);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(blk_reset_queue);
+
 unsigned long blk_rq_timeout(unsigned long timeout)
 {
 	unsigned long maxt;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 2498fdf..5d5e55a 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -120,6 +120,11 @@ struct blk_mq_ops {
 	timeout_fn		*timeout;
 
 	/*
+	 * Force executing IO to complete or fail.
+	 */
+	reset_fn		*reset;
+
+	/*
 	 * Called to poll for completion of a specific tag.
 	 */
 	poll_fn			*poll;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1fd8fdf..f030436 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -227,6 +227,7 @@ enum blk_eh_timer_return {
 };
 
 typedef enum blk_eh_timer_return (rq_timed_out_fn)(struct request *);
+typedef enum blk_eh_timer_return (reset_fn)(struct request_queue *);
 
 enum blk_queue_state {
 	Queue_down,
@@ -304,6 +305,7 @@ struct request_queue {
 	unprep_rq_fn		*unprep_rq_fn;
 	softirq_done_fn		*softirq_done_fn;
 	rq_timed_out_fn		*rq_timed_out_fn;
+	reset_fn		*reset_fn;
 	dma_drain_needed_fn	*dma_drain_needed;
 	lld_busy_fn		*lld_busy_fn;
 
@@ -464,6 +466,8 @@ struct request_queue {
 	struct bio_set		*bio_split;
 
 	bool			mq_sysfs_init_done;
+
+	wait_queue_head_t	reset_wq;
 };
 
 #define QUEUE_FLAG_QUEUED	1	/* uses generic tag queueing */
@@ -492,6 +496,7 @@ struct request_queue {
 #define QUEUE_FLAG_WC	       23	/* Write back caching */
 #define QUEUE_FLAG_FUA	       24	/* device supports FUA writes */
 #define QUEUE_FLAG_FLUSH_NQ    25	/* flush not queueuable */
+#define QUEUE_FLAG_RESETTING   26	/* reset callback is executing */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\
@@ -564,6 +569,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 	__clear_bit(flag, &q->queue_flags);
 }
 
+#define blk_queue_resetting(q)	test_bit(QUEUE_FLAG_RESETTING, &(q)->queue_flags)
 #define blk_queue_tagged(q)	test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
 #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
@@ -955,6 +961,7 @@ extern bool __blk_end_request_err(struct request *rq, int error);
 extern void blk_complete_request(struct request *);
 extern void __blk_complete_request(struct request *);
 extern void blk_abort_request(struct request *);
+extern int blk_reset_queue(struct request_queue *);
 extern void blk_unprep_request(struct request *);
 
 /*
@@ -1008,6 +1015,7 @@ extern void blk_queue_update_dma_alignment(struct request_queue *, int);
 extern void blk_queue_softirq_done(struct request_queue *, softirq_done_fn *);
 extern void blk_queue_rq_timed_out(struct request_queue *, rq_timed_out_fn *);
 extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
+extern void blk_queue_reset(struct request_queue *, reset_fn *);
 extern void blk_queue_flush_queueable(struct request_queue *q, bool queueable);
 extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool fua);
 extern struct backing_dev_info *blk_get_backing_dev_info(struct block_device *bdev);
-- 
2.7.2

[PATCH 3/5] target: call queue reset if supported

[mchristi]

From: Mike Christie <mchristi@redhat.com>

Instead of waiting for commands, map the lun reset operation
to a queue reset.

We do not check the result of blk_reset_queue because if
it works then we need to wait for the bio/request completions
and if it failed we might as wait and hope like we did before.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/target/target_core_file.c    | 12 ++++++++++++
 drivers/target/target_core_iblock.c  |  8 ++++++++
 drivers/target/target_core_pscsi.c   |  8 ++++++++
 drivers/target/target_core_tmr.c     |  3 +++
 include/target/target_core_backend.h |  1 +
 5 files changed, 32 insertions(+)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 75f0f08..d6af1f2 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -97,6 +97,17 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
 	return &fd_dev->dev;
 }
 
+static int fd_reset_device(struct se_device *dev)
+{
+	struct fd_dev *fd_dev = FD_DEV(dev);
+	struct inode *inode;
+
+	inode = fd_dev->fd_file->f_mapping->host;
+	if (!S_ISBLK(inode->i_mode))
+		return -EOPNOTSUPP;
+	return blk_reset_queue(bdev_get_queue(inode->i_bdev));
+}
+
 static int fd_configure_device(struct se_device *dev)
 {
 	struct fd_dev *fd_dev = FD_DEV(dev);
@@ -813,6 +824,7 @@ static const struct target_backend_ops fileio_ops = {
 	.attach_hba		= fd_attach_hba,
 	.detach_hba		= fd_detach_hba,
 	.alloc_device		= fd_alloc_device,
+	.reset_device		= fd_reset_device,
 	.configure_device	= fd_configure_device,
 	.free_device		= fd_free_device,
 	.parse_cdb		= fd_parse_cdb,
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 7c4efb4..0a7dd59 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -79,6 +79,13 @@ static struct se_device *iblock_alloc_device(struct se_hba *hba, const char *nam
 	return &ib_dev->dev;
 }
 
+static int iblock_reset_device(struct se_device *dev)
+{
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+
+	return blk_reset_queue(bdev_get_queue(ib_dev->ibd_bd));
+}
+
 static int iblock_configure_device(struct se_device *dev)
 {
 	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
@@ -848,6 +855,7 @@ static const struct target_backend_ops iblock_ops = {
 	.detach_hba		= iblock_detach_hba,
 	.alloc_device		= iblock_alloc_device,
 	.configure_device	= iblock_configure_device,
+	.reset_device		= iblock_reset_device,
 	.free_device		= iblock_free_device,
 	.parse_cdb		= iblock_parse_cdb,
 	.set_configfs_dev_params = iblock_set_configfs_dev_params,
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index de18790..fa0505b 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -455,6 +455,13 @@ static int pscsi_create_type_other(struct se_device *dev,
 	return 0;
 }
 
+static int pscsi_reset_device(struct se_device *dev)
+{
+	struct pscsi_dev_virt *pdv = PSCSI_DEV(dev);
+
+	return blk_reset_queue(pdv->pdv_sd->request_queue);
+}
+
 static int pscsi_configure_device(struct se_device *dev)
 {
 	struct se_hba *hba = dev->se_hba;
@@ -1131,6 +1138,7 @@ static const struct target_backend_ops pscsi_ops = {
 	.detach_hba		= pscsi_detach_hba,
 	.pmode_enable_hba	= pscsi_pmode_enable_hba,
 	.alloc_device		= pscsi_alloc_device,
+	.reset_device		= pscsi_reset_device,
 	.configure_device	= pscsi_configure_device,
 	.free_device		= pscsi_free_device,
 	.transport_complete	= pscsi_transport_complete,
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 4f229e7..609ec53 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -431,6 +431,9 @@ int core_tmr_lun_reset(
 		(preempt_and_abort_list) ? "Preempt" : "TMR",
 		dev->transport->name, tas);
 
+	if (dev->transport->reset_device(dev))
+		dev->transport->reset_device(dev);
+
 	core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list);
 	core_tmr_drain_state_list(dev, prout_cmd, tmr_sess, tas,
 				preempt_and_abort_list);
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 28ee5c2..a055e4f 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -17,6 +17,7 @@ struct target_backend_ops {
 
 	struct se_device *(*alloc_device)(struct se_hba *, const char *);
 	int (*configure_device)(struct se_device *);
+	int (*reset_device)(struct se_device *);
 	void (*free_device)(struct se_device *device);
 
 	ssize_t (*set_configfs_dev_params)(struct se_device *,
-- 
2.7.2

[PATCH 4/5] scsi: add new async device reset support

[mchristi]

From: Mike Christie <mchristi@redhat.com>

Currently, if the SCSI eh runs then before we do a LUN_RESET
we stop the host. This patch and the block layer one before it
begin to add infrastructure to be able to do a LUN_RESET and
eventually do a transport level recovery without having to stop the
host.

For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
LLD manages the commands that are affected.

eh_async_device_reset_handler:

The LLD should perform a LUN RESET that affects all commands
that have been accepted by its queuecommand callout for the
device passed in to the callout. While the reset handler is running,
queuecommand will not be running or called for the device.

Unlike eh_device_reset_handler, queuecommand may still be
called for other devices, and the LLD must call scsi_done for the
commands that have been affected by the reset.

If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
must be failed with DID_ABORT.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/scsi/scsi_error.c | 31 ++++++++++++++++++++++++++++---
 drivers/scsi/scsi_lib.c   |  6 ++++++
 drivers/scsi/scsi_priv.h  |  1 +
 include/scsi/scsi_host.h  | 17 +++++++++++++++++
 4 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..cec2dfb 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -853,16 +853,41 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 {
 	int rtn;
 	struct scsi_host_template *hostt = scmd->device->host->hostt;
+	struct scsi_device *sdev = scmd->device;
 
-	if (!hostt->eh_device_reset_handler)
+	if (!hostt->eh_device_reset_handler &&
+	    !hostt->eh_async_device_reset_handler)
 		return FAILED;
 
-	rtn = hostt->eh_device_reset_handler(scmd);
+	if (hostt->eh_device_reset_handler) {
+		rtn = hostt->eh_device_reset_handler(scmd);
+	} else {
+		if (!blk_reset_queue(sdev->request_queue))
+			rtn = SUCCESS;
+		else
+			rtn = FAILED;
+	}
 	if (rtn == SUCCESS)
-		__scsi_report_device_reset(scmd->device, NULL);
+		__scsi_report_device_reset(sdev, NULL);
 	return rtn;
 }
 
+enum blk_eh_timer_return scsi_reset_queue(struct request_queue *q)
+{
+	struct scsi_device *sdev = q->queuedata;
+	struct scsi_host_template *hostt = sdev->host->hostt;
+	int rtn;
+
+	if (!hostt->eh_async_device_reset_handler)
+		return -EOPNOTSUPP;
+
+	rtn = hostt->eh_async_device_reset_handler(sdev);
+	if (rtn == SUCCESS || rtn == FAST_IO_FAIL)
+		return BLK_EH_HANDLED;
+
+	return BLK_EH_NOT_HANDLED;
+}
+
 /**
  * scsi_try_to_abort_cmd - Ask host to abort a SCSI command
  * @hostt:	SCSI driver host template
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..11374dd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -779,6 +779,10 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
 		set_host_byte(cmd, DID_OK);
 		error = -ENODATA;
 		break;
+	case DID_ABORT:
+		set_host_byte(cmd, DID_OK);
+		error = -EINTR;
+		break;
 	default:
 		error = -EIO;
 		break;
@@ -2159,6 +2163,7 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 	blk_queue_softirq_done(q, scsi_softirq_done);
 	blk_queue_rq_timed_out(q, scsi_times_out);
 	blk_queue_lld_busy(q, scsi_lld_busy);
+	blk_queue_reset(q, scsi_reset_queue);
 	return q;
 }
 
@@ -2167,6 +2172,7 @@ static struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
 	.complete	= scsi_softirq_done,
 	.timeout	= scsi_timeout,
+	.reset		= scsi_reset_queue,
 	.init_request	= scsi_init_request,
 	.exit_request	= scsi_exit_request,
 };
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 27b4d0a..2e03168 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -67,6 +67,7 @@ extern void scsi_exit_devinfo(void);
 
 /* scsi_error.c */
 extern void scmd_eh_abort_handler(struct work_struct *work);
+extern enum blk_eh_timer_return scsi_reset_queue(struct request_queue *q);
 extern enum blk_eh_timer_return scsi_times_out(struct request *req);
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index fcfa3d7..532deb5 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -146,6 +146,23 @@ struct scsi_host_template {
 	 */
 	int (* eh_abort_handler)(struct scsi_cmnd *);
 	int (* eh_device_reset_handler)(struct scsi_cmnd *);
+	/*
+	 * eh_async_device_reset_handler - Perform LUN RESET
+	 * @scsi_device: scsi device to reset
+	 *
+	 * The LLD should perform a LUN RESET that affects all commands
+	 * that have been accepted by its queuecommand callout for the
+	 * device passed in. While the reset handler is running, queuecommand
+	 * will not be called for the device.
+	 *
+	 * Unlike eh_device_reset_handler, queuecommand may still be called
+	 * for other devices, and the LLD must call scsi_done for the commands
+	 * that have been affected by the reset.
+	 *
+	 * If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds for
+	 * scsi_device must be failed with DID_ABORT.
+	 */
+	int (* eh_async_device_reset_handler)(struct scsi_device *);
 	int (* eh_target_reset_handler)(struct scsi_cmnd *);
 	int (* eh_bus_reset_handler)(struct scsi_cmnd *);
 	int (* eh_host_reset_handler)(struct scsi_cmnd *);
-- 
2.7.2

[PATCH 5/5] iscsi initiator: support eh_async_device_reset_handler

[mchristi]

From: Mike Christie <mchristi@redhat.com>

Like a lot of drivers, iscsi already supported SG_SCSI_RESET_DEVICE
and did not need the host stopped, so supporting
eh_async_device_reset_handler just required some renames, and
changing the callout argument from a scsi_cmnd to a scsi_device.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |  2 +-
 drivers/scsi/be2iscsi/be_main.c          | 10 +++++-----
 drivers/scsi/bnx2i/bnx2i_iscsi.c         |  2 +-
 drivers/scsi/cxgbi/cxgb3i/cxgb3i.c       |  2 +-
 drivers/scsi/cxgbi/cxgb4i/cxgb4i.c       |  2 +-
 drivers/scsi/iscsi_tcp.c                 |  2 +-
 drivers/scsi/libiscsi.c                  | 16 ++++++++--------
 include/scsi/libiscsi.h                  |  2 +-
 8 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 80b6bed..244e330 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -992,7 +992,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.max_sectors            = ISER_DEF_MAX_SECTORS,
 	.cmd_per_lun            = ISER_DEF_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
-	.eh_device_reset_handler= iscsi_eh_device_reset,
+	.eh_async_device_reset_handler= iscsi_eh_device_reset,
 	.eh_target_reset_handler = iscsi_eh_recover_target,
 	.target_alloc		= iscsi_target_alloc,
 	.use_clustering         = ENABLE_CLUSTERING,
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index f05e773..fd1dd20 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -294,7 +294,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
 	return iscsi_eh_abort(sc);
 }
 
-static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
+static int beiscsi_eh_device_reset(struct scsi_device *sdev)
 {
 	struct iscsi_task *abrt_task;
 	struct beiscsi_io_task *abrt_io_task;
@@ -309,7 +309,7 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
 	int rc;
 
 	/* invalidate iocbs */
-	cls_session = starget_to_session(scsi_target(sc->device));
+	cls_session = starget_to_session(scsi_target(sdev));
 	session = cls_session->dd_data;
 	spin_lock_bh(&session->frwd_lock);
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN) {
@@ -329,7 +329,7 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
 		if (!abrt_task->sc || abrt_task->state == ISCSI_TASK_FREE)
 			continue;
 
-		if (sc->device->lun != abrt_task->sc->device->lun)
+		if (sdev->lun != abrt_task->sc->device->lun)
 			continue;
 
 		/* Invalidate WRB Posted for this Task */
@@ -371,7 +371,7 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
 	if (rc != -EBUSY)
 		pci_free_consistent(phba->ctrl.pdev, nonemb_cmd.size,
 				    nonemb_cmd.va, nonemb_cmd.dma);
-	return iscsi_eh_device_reset(sc);
+	return iscsi_eh_device_reset(sdev);
 }
 
 static ssize_t beiscsi_show_boot_tgt_info(void *data, int type, char *buf)
@@ -560,7 +560,7 @@ static struct scsi_host_template beiscsi_sht = {
 	.slave_configure = beiscsi_slave_configure,
 	.target_alloc = iscsi_target_alloc,
 	.eh_abort_handler = beiscsi_eh_abort,
-	.eh_device_reset_handler = beiscsi_eh_device_reset,
+	.eh_async_device_reset_handler = beiscsi_eh_device_reset,
 	.eh_target_reset_handler = iscsi_eh_session_reset,
 	.shost_attrs = beiscsi_attrs,
 	.sg_tablesize = BEISCSI_SGLIST_ELEMENTS,
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 7289437..e73ee8d 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -2260,7 +2260,7 @@ static struct scsi_host_template bnx2i_host_template = {
 	.proc_name		= "bnx2i",
 	.queuecommand		= iscsi_queuecommand,
 	.eh_abort_handler	= iscsi_eh_abort,
-	.eh_device_reset_handler = iscsi_eh_device_reset,
+	.eh_async_device_reset_handler = iscsi_eh_device_reset,
 	.eh_target_reset_handler = iscsi_eh_recover_target,
 	.change_queue_depth	= scsi_change_queue_depth,
 	.target_alloc		= iscsi_target_alloc,
diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
index e22a268..f70e9f5 100644
--- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
+++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
@@ -91,7 +91,7 @@ static struct scsi_host_template cxgb3i_host_template = {
 	.max_sectors	= 0xFFFF,
 	.cmd_per_lun	= ISCSI_DEF_CMD_PER_LUN,
 	.eh_abort_handler = iscsi_eh_abort,
-	.eh_device_reset_handler = iscsi_eh_device_reset,
+	.eh_async_device_reset_handler = iscsi_eh_device_reset,
 	.eh_target_reset_handler = iscsi_eh_recover_target,
 	.target_alloc	= iscsi_target_alloc,
 	.use_clustering	= DISABLE_CLUSTERING,
diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 339f6b7..230e7d2 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -100,7 +100,7 @@ static struct scsi_host_template cxgb4i_host_template = {
 	.max_sectors	= 0xFFFF,
 	.cmd_per_lun	= ISCSI_DEF_CMD_PER_LUN,
 	.eh_abort_handler = iscsi_eh_abort,
-	.eh_device_reset_handler = iscsi_eh_device_reset,
+	.eh_async_device_reset_handler = iscsi_eh_device_reset,
 	.eh_target_reset_handler = iscsi_eh_recover_target,
 	.target_alloc	= iscsi_target_alloc,
 	.use_clustering	= DISABLE_CLUSTERING,
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 2e4c82f..1123f98 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -968,7 +968,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht = {
 	.max_sectors		= 0xFFFF,
 	.cmd_per_lun		= ISCSI_DEF_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
-	.eh_device_reset_handler= iscsi_eh_device_reset,
+	.eh_async_device_reset_handler= iscsi_eh_device_reset,
 	.eh_target_reset_handler = iscsi_eh_recover_target,
 	.use_clustering         = DISABLE_CLUSTERING,
 	.slave_alloc            = iscsi_sw_tcp_slave_alloc,
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6bffd91..ccef7be 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2250,17 +2250,18 @@ failed_unlocked:
 }
 EXPORT_SYMBOL_GPL(iscsi_eh_abort);
 
-static void iscsi_prep_lun_reset_pdu(struct scsi_cmnd *sc, struct iscsi_tm *hdr)
+static void iscsi_prep_lun_reset_pdu(struct scsi_device *sdev,
+				     struct iscsi_tm *hdr)
 {
 	memset(hdr, 0, sizeof(*hdr));
 	hdr->opcode = ISCSI_OP_SCSI_TMFUNC | ISCSI_OP_IMMEDIATE;
 	hdr->flags = ISCSI_TM_FUNC_LOGICAL_UNIT_RESET & ISCSI_FLAG_TM_FUNC_MASK;
 	hdr->flags |= ISCSI_FLAG_CMD_FINAL;
-	int_to_scsilun(sc->device->lun, &hdr->lun);
+	int_to_scsilun(sdev->lun, &hdr->lun);
 	hdr->rtt = RESERVED_ITT;
 }
 
-int iscsi_eh_device_reset(struct scsi_cmnd *sc)
+int iscsi_eh_device_reset(struct scsi_device *sdev)
 {
 	struct iscsi_cls_session *cls_session;
 	struct iscsi_session *session;
@@ -2268,11 +2269,10 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 	struct iscsi_tm *hdr;
 	int rc = FAILED;
 
-	cls_session = starget_to_session(scsi_target(sc->device));
+	cls_session = starget_to_session(scsi_target(sdev));
 	session = cls_session->dd_data;
 
-	ISCSI_DBG_EH(session, "LU Reset [sc %p lun %llu]\n", sc,
-		     sc->device->lun);
+	ISCSI_DBG_EH(session, "LU Reset [lun %llu]\n", sdev->lun);
 
 	mutex_lock(&session->eh_mutex);
 	spin_lock_bh(&session->frwd_lock);
@@ -2290,7 +2290,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 	conn->tmf_state = TMF_QUEUED;
 
 	hdr = &conn->tmhdr;
-	iscsi_prep_lun_reset_pdu(sc, hdr);
+	iscsi_prep_lun_reset_pdu(sdev, hdr);
 
 	if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age,
 				    session->lu_reset_timeout)) {
@@ -2317,7 +2317,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 
 	spin_lock_bh(&session->frwd_lock);
 	memset(hdr, 0, sizeof(*hdr));
-	fail_scsi_tasks(conn, sc->device->lun, DID_ERROR);
+	fail_scsi_tasks(conn, sdev->lun, DID_ABORT);
 	conn->tmf_state = TMF_INITIAL;
 	spin_unlock_bh(&session->frwd_lock);
 
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 4d1c46a..8db2d22 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -381,7 +381,7 @@ struct iscsi_host {
 extern int iscsi_eh_abort(struct scsi_cmnd *sc);
 extern int iscsi_eh_recover_target(struct scsi_cmnd *sc);
 extern int iscsi_eh_session_reset(struct scsi_cmnd *sc);
-extern int iscsi_eh_device_reset(struct scsi_cmnd *sc);
+extern int iscsi_eh_device_reset(struct scsi_device *sdev);
 extern int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc);
 
 /*
-- 
2.7.2

Re: [PATCH 1/5] blk mq: take ref to q when running it

[Bart Van Assche]

On 05/25/2016 12:54 AM, mchristi@redhat.com wrote:
> If the __blk_mq_run_hw_queue is a result of a async run or retry
> then we do not have a reference to the queue. At this time,
> blk_cleanup_queue could begin tearing it down while the queue_rq
> function is being run.
>
> This patch just has us do a blk_queue_enter call.
>
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> ---
>  block/blk-mq.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 7df9c92..5958a02 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -738,8 +738,13 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
>
>  	WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask));
>
> -	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
> +	if (blk_queue_enter(q, false)) {
> +		blk_mq_run_hw_queue(hctx, true);
>  		return;
> +	}
> +
> +	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
> +		goto exit_queue;
>
>  	hctx->run++;
>
> @@ -832,6 +837,9 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
>  		 **/
>  		blk_mq_run_hw_queue(hctx, true);
>  	}
> +
> +exit_queue:
> +	blk_queue_exit(q);
>  }

Hello Mike,

blk_cleanup_queue() waits until delay_work and run_work have finished 
inside blk_sync_queue(). The code in blk_cleanup_queue() before the 
blk_sync_queue() call should be safe to execute concurrently with 
queue_rq(). Or did I perhaps miss something?

Bart.

Re: [PATCH 1/5] blk mq: take ref to q when running it

[Bart Van Assche]

On 05/25/2016 12:54 AM, mchristi@redhat.com wrote:
> If the __blk_mq_run_hw_queue is a result of a async run or retry
> then we do not have a reference to the queue. At this time,
> blk_cleanup_queue could begin tearing it down while the queue_rq
> function is being run.
>
> This patch just has us do a blk_queue_enter call.
>
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> ---
>  block/blk-mq.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 7df9c92..5958a02 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -738,8 +738,13 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
>
>  	WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask));
>
> -	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
> +	if (blk_queue_enter(q, false)) {
> +		blk_mq_run_hw_queue(hctx, true);
>  		return;
> +	}
> +
> +	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
> +		goto exit_queue;
>
>  	hctx->run++;
>
> @@ -832,6 +837,9 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
>  		 **/
>  		blk_mq_run_hw_queue(hctx, true);
>  	}
> +
> +exit_queue:
> +	blk_queue_exit(q);
>  }

Hello Mike,

blk_cleanup_queue() waits until delay_work and run_work have finished 
inside blk_sync_queue(). The code in blk_cleanup_queue() before the 
blk_sync_queue() call should be safe to execute concurrently with 
queue_rq(). Or did I perhaps miss something?

Bart.

Re: [PATCH 2/5] block: add queue reset support

[Bart Van Assche]

On 05/25/2016 12:55 AM, mchristi@redhat.com wrote:
> +	spin_lock_irq(q->queue_lock);
> +	wait_event_lock_irq(q->reset_wq,
> +			    !queue_flag_test_and_set(QUEUE_FLAG_RESETTING, q),
> +			    *q->queue_lock);
> +	if (blk_queue_dead(q)) {
> +		rc = -ENODEV;
> +		spin_unlock_irq(q->queue_lock);
> +		goto done;
> +	}
> +	spin_unlock_irq(q->queue_lock);

Is the flag QUEUE_FLAG_RESETTING used to serialize blk_reset_queue() 
calls? If so, please consider using a mutex instead. Lockdep can detect 
lock inversion for mutexes and spinlocks but not for serialization that 
is based on wait_event_lock_irq() loops.

> +	if (q->mq_ops) {
> +		blk_mq_stop_hw_queues(q);
> +		blk_mq_freeze_queue(q);
> +
> +		eh_rc = q->mq_ops->reset(q);
> +
> +		blk_mq_unfreeze_queue(q);
> +		blk_mq_start_stopped_hw_queues(q, true);
> +	} else if (q->reset_fn) {
> +		spin_lock_irq(q->queue_lock);
> +		blk_stop_queue(q);
> +		spin_unlock_irq(q->queue_lock);
> +
> +		while (q->request_fn_active)
> +			msleep(10);
> +
> +		eh_rc = q->reset_fn(q);
> +
> +		spin_lock_irq(q->queue_lock);
> +		blk_start_queue(q);
> +		spin_unlock_irq(q->queue_lock);
> +	}

Some of the above code is similar to some of the code in 
scsi_internal_device_block() and scsi_internal_device_unblock(). Have 
you considered to avoid this duplication by introducing helpers in the 
block layer for blocking and unblocking queues?

Thanks,

Bart.

Re: [PATCH 2/5] block: add queue reset support

[Bart Van Assche]

On 05/25/2016 12:55 AM, mchristi@redhat.com wrote:
> +	spin_lock_irq(q->queue_lock);
> +	wait_event_lock_irq(q->reset_wq,
> +			    !queue_flag_test_and_set(QUEUE_FLAG_RESETTING, q),
> +			    *q->queue_lock);
> +	if (blk_queue_dead(q)) {
> +		rc = -ENODEV;
> +		spin_unlock_irq(q->queue_lock);
> +		goto done;
> +	}
> +	spin_unlock_irq(q->queue_lock);

Is the flag QUEUE_FLAG_RESETTING used to serialize blk_reset_queue() 
calls? If so, please consider using a mutex instead. Lockdep can detect 
lock inversion for mutexes and spinlocks but not for serialization that 
is based on wait_event_lock_irq() loops.

> +	if (q->mq_ops) {
> +		blk_mq_stop_hw_queues(q);
> +		blk_mq_freeze_queue(q);
> +
> +		eh_rc = q->mq_ops->reset(q);
> +
> +		blk_mq_unfreeze_queue(q);
> +		blk_mq_start_stopped_hw_queues(q, true);
> +	} else if (q->reset_fn) {
> +		spin_lock_irq(q->queue_lock);
> +		blk_stop_queue(q);
> +		spin_unlock_irq(q->queue_lock);
> +
> +		while (q->request_fn_active)
> +			msleep(10);
> +
> +		eh_rc = q->reset_fn(q);
> +
> +		spin_lock_irq(q->queue_lock);
> +		blk_start_queue(q);
> +		spin_unlock_irq(q->queue_lock);
> +	}

Some of the above code is similar to some of the code in 
scsi_internal_device_block() and scsi_internal_device_unblock(). Have 
you considered to avoid this duplication by introducing helpers in the 
block layer for blocking and unblocking queues?

Thanks,

Bart.

Re: [PATCH 1/5] blk mq: take ref to q when running it

[Mike Christie]

On 05/25/2016 10:53 AM, Bart Van Assche wrote:
> On 05/25/2016 12:54 AM, mchristi@redhat.com wrote:
>> If the __blk_mq_run_hw_queue is a result of a async run or retry
>> then we do not have a reference to the queue. At this time,
>> blk_cleanup_queue could begin tearing it down while the queue_rq
>> function is being run.
>>
>> This patch just has us do a blk_queue_enter call.
>>
>> Signed-off-by: Mike Christie <mchristi@redhat.com>
>> ---
>>  block/blk-mq.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 7df9c92..5958a02 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -738,8 +738,13 @@ static void __blk_mq_run_hw_queue(struct
>> blk_mq_hw_ctx *hctx)
>>
>>      WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask));
>>
>> -    if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
>> +    if (blk_queue_enter(q, false)) {
>> +        blk_mq_run_hw_queue(hctx, true);
>>          return;
>> +    }
>> +
>> +    if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
>> +        goto exit_queue;
>>
>>      hctx->run++;
>>
>> @@ -832,6 +837,9 @@ static void __blk_mq_run_hw_queue(struct
>> blk_mq_hw_ctx *hctx)
>>           **/
>>          blk_mq_run_hw_queue(hctx, true);
>>      }
>> +
>> +exit_queue:
>> +    blk_queue_exit(q);
>>  }
> 
> Hello Mike,
> 
> blk_cleanup_queue() waits until delay_work and run_work have finished
> inside blk_sync_queue(). The code in blk_cleanup_queue() before the
> blk_sync_queue() call should be safe to execute concurrently with
> queue_rq(). Or did I perhaps miss something?
> 

My concern was when blk_mq_run_hw_queue is run with async=false then we
are not always run from the work queue or a under another
blk_queue_enter call already.

For example, in the scsi cases where we call
scsi_run_queue->blk_mq_start_stopped_hw_queues we can be in soft irq
context, some driver thread or the scsi eh thread.

Re: [PATCH 2/5] block: add queue reset support

[Mike Christie]

On 05/25/2016 11:13 AM, Bart Van Assche wrote:
> On 05/25/2016 12:55 AM, mchristi@redhat.com wrote:
>> +    spin_lock_irq(q->queue_lock);
>> +    wait_event_lock_irq(q->reset_wq,
>> +                !queue_flag_test_and_set(QUEUE_FLAG_RESETTING, q),
>> +                *q->queue_lock);
>> +    if (blk_queue_dead(q)) {
>> +        rc = -ENODEV;
>> +        spin_unlock_irq(q->queue_lock);
>> +        goto done;
>> +    }
>> +    spin_unlock_irq(q->queue_lock);
> 
> Is the flag QUEUE_FLAG_RESETTING used to serialize blk_reset_queue()
> calls? If so, please consider using a mutex instead. Lockdep can detect
> lock inversion for mutexes and spinlocks but not for serialization that
> is based on wait_event_lock_irq() loops.

Will do.

> 
>> +    if (q->mq_ops) {
>> +        blk_mq_stop_hw_queues(q);
>> +        blk_mq_freeze_queue(q);
>> +
>> +        eh_rc = q->mq_ops->reset(q);
>> +
>> +        blk_mq_unfreeze_queue(q);
>> +        blk_mq_start_stopped_hw_queues(q, true);
>> +    } else if (q->reset_fn) {
>> +        spin_lock_irq(q->queue_lock);
>> +        blk_stop_queue(q);
>> +        spin_unlock_irq(q->queue_lock);
>> +
>> +        while (q->request_fn_active)
>> +            msleep(10);
>> +
>> +        eh_rc = q->reset_fn(q);
>> +
>> +        spin_lock_irq(q->queue_lock);
>> +        blk_start_queue(q);
>> +        spin_unlock_irq(q->queue_lock);
>> +    }
> 
> Some of the above code is similar to some of the code in
> scsi_internal_device_block() and scsi_internal_device_unblock(). Have
> you considered to avoid this duplication by introducing helpers in the
> block layer for blocking and unblocking queues?
> 

Will do. Thanks.

Re: [PATCH 1/5] blk mq: take ref to q when running it

[Mike Christie]

On 05/25/2016 02:15 PM, Mike Christie wrote:
> On 05/25/2016 10:53 AM, Bart Van Assche wrote:
>> On 05/25/2016 12:54 AM, mchristi@redhat.com wrote:
>>> If the __blk_mq_run_hw_queue is a result of a async run or retry
>>> then we do not have a reference to the queue. At this time,
>>> blk_cleanup_queue could begin tearing it down while the queue_rq
>>> function is being run.
>>>
>>> This patch just has us do a blk_queue_enter call.
>>>
>>> Signed-off-by: Mike Christie <mchristi@redhat.com>
>>> ---
>>>  block/blk-mq.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 7df9c92..5958a02 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -738,8 +738,13 @@ static void __blk_mq_run_hw_queue(struct
>>> blk_mq_hw_ctx *hctx)
>>>
>>>      WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask));
>>>
>>> -    if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
>>> +    if (blk_queue_enter(q, false)) {
>>> +        blk_mq_run_hw_queue(hctx, true);
>>>          return;
>>> +    }
>>> +
>>> +    if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
>>> +        goto exit_queue;
>>>
>>>      hctx->run++;
>>>
>>> @@ -832,6 +837,9 @@ static void __blk_mq_run_hw_queue(struct
>>> blk_mq_hw_ctx *hctx)
>>>           **/
>>>          blk_mq_run_hw_queue(hctx, true);
>>>      }
>>> +
>>> +exit_queue:
>>> +    blk_queue_exit(q);
>>>  }
>>
>> Hello Mike,
>>
>> blk_cleanup_queue() waits until delay_work and run_work have finished
>> inside blk_sync_queue(). The code in blk_cleanup_queue() before the
>> blk_sync_queue() call should be safe to execute concurrently with
>> queue_rq(). Or did I perhaps miss something?
>>
> 
> My concern was when blk_mq_run_hw_queue is run with async=false then we
> are not always run from the work queue or a under another
> blk_queue_enter call already.
> 
> For example, in the scsi cases where we call
> scsi_run_queue->blk_mq_start_stopped_hw_queues we can be in soft irq
> context, some driver thread or the scsi eh thread.

Doh, ok. I guess if my analysis of the code is correct, then I should
just move the blk_queue_enter call into blk_mq_run_hw_queue's
async=false code path. I am not sure what I was thinking.

Re: [PATCH 3/5] target: call queue reset if supported

[Christoph Hellwig]

On Wed, May 25, 2016 at 02:55:01AM -0500, mchristi@redhat.com wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> Instead of waiting for commands, map the lun reset operation
> to a queue reset.
> 
> We do not check the result of blk_reset_queue because if
> it works then we need to wait for the bio/request completions
> and if it failed we might as wait and hope like we did before.

Please don't call into block device code from the file handler.

Re: [PATCH 4/5] scsi: add new async device reset support

[Christoph Hellwig]

Adding Hannes to the Cc list as he's been looking into EH improvements
in this area.

On Wed, May 25, 2016 at 02:55:02AM -0500, mchristi@redhat.com wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> Currently, if the SCSI eh runs then before we do a LUN_RESET
> we stop the host. This patch and the block layer one before it
> begin to add infrastructure to be able to do a LUN_RESET and
> eventually do a transport level recovery without having to stop the
> host.
> 
> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
> LLD manages the commands that are affected.
> 
> eh_async_device_reset_handler:
> 
> The LLD should perform a LUN RESET that affects all commands
> that have been accepted by its queuecommand callout for the
> device passed in to the callout. While the reset handler is running,
> queuecommand will not be running or called for the device.
> 
> Unlike eh_device_reset_handler, queuecommand may still be
> called for other devices, and the LLD must call scsi_done for the
> commands that have been affected by the reset.
> 
> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
> must be failed with DID_ABORT.
> 
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> ---
>  drivers/scsi/scsi_error.c | 31 ++++++++++++++++++++++++++++---
>  drivers/scsi/scsi_lib.c   |  6 ++++++
>  drivers/scsi/scsi_priv.h  |  1 +
>  include/scsi/scsi_host.h  | 17 +++++++++++++++++
>  4 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 984ddcb..cec2dfb 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -853,16 +853,41 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
>  {
>  	int rtn;
>  	struct scsi_host_template *hostt = scmd->device->host->hostt;
> +	struct scsi_device *sdev = scmd->device;
>  
> -	if (!hostt->eh_device_reset_handler)
> +	if (!hostt->eh_device_reset_handler &&
> +	    !hostt->eh_async_device_reset_handler)
>  		return FAILED;
>  
> -	rtn = hostt->eh_device_reset_handler(scmd);
> +	if (hostt->eh_device_reset_handler) {
> +		rtn = hostt->eh_device_reset_handler(scmd);
> +	} else {
> +		if (!blk_reset_queue(sdev->request_queue))
> +			rtn = SUCCESS;
> +		else
> +			rtn = FAILED;
> +	}
>  	if (rtn == SUCCESS)
> -		__scsi_report_device_reset(scmd->device, NULL);
> +		__scsi_report_device_reset(sdev, NULL);
>  	return rtn;
>  }
>  
> +enum blk_eh_timer_return scsi_reset_queue(struct request_queue *q)
> +{
> +	struct scsi_device *sdev = q->queuedata;
> +	struct scsi_host_template *hostt = sdev->host->hostt;
> +	int rtn;
> +
> +	if (!hostt->eh_async_device_reset_handler)
> +		return -EOPNOTSUPP;
> +
> +	rtn = hostt->eh_async_device_reset_handler(sdev);
> +	if (rtn == SUCCESS || rtn == FAST_IO_FAIL)
> +		return BLK_EH_HANDLED;
> +
> +	return BLK_EH_NOT_HANDLED;
> +}
> +
>  /**
>   * scsi_try_to_abort_cmd - Ask host to abort a SCSI command
>   * @hostt:	SCSI driver host template
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..11374dd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -779,6 +779,10 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
>  		set_host_byte(cmd, DID_OK);
>  		error = -ENODATA;
>  		break;
> +	case DID_ABORT:
> +		set_host_byte(cmd, DID_OK);
> +		error = -EINTR;
> +		break;
>  	default:
>  		error = -EIO;
>  		break;
> @@ -2159,6 +2163,7 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
>  	blk_queue_softirq_done(q, scsi_softirq_done);
>  	blk_queue_rq_timed_out(q, scsi_times_out);
>  	blk_queue_lld_busy(q, scsi_lld_busy);
> +	blk_queue_reset(q, scsi_reset_queue);
>  	return q;
>  }
>  
> @@ -2167,6 +2172,7 @@ static struct blk_mq_ops scsi_mq_ops = {
>  	.queue_rq	= scsi_queue_rq,
>  	.complete	= scsi_softirq_done,
>  	.timeout	= scsi_timeout,
> +	.reset		= scsi_reset_queue,
>  	.init_request	= scsi_init_request,
>  	.exit_request	= scsi_exit_request,
>  };
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 27b4d0a..2e03168 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -67,6 +67,7 @@ extern void scsi_exit_devinfo(void);
>  
>  /* scsi_error.c */
>  extern void scmd_eh_abort_handler(struct work_struct *work);
> +extern enum blk_eh_timer_return scsi_reset_queue(struct request_queue *q);
>  extern enum blk_eh_timer_return scsi_times_out(struct request *req);
>  extern int scsi_error_handler(void *host);
>  extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index fcfa3d7..532deb5 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -146,6 +146,23 @@ struct scsi_host_template {
>  	 */
>  	int (* eh_abort_handler)(struct scsi_cmnd *);
>  	int (* eh_device_reset_handler)(struct scsi_cmnd *);
> +	/*
> +	 * eh_async_device_reset_handler - Perform LUN RESET
> +	 * @scsi_device: scsi device to reset
> +	 *
> +	 * The LLD should perform a LUN RESET that affects all commands
> +	 * that have been accepted by its queuecommand callout for the
> +	 * device passed in. While the reset handler is running, queuecommand
> +	 * will not be called for the device.
> +	 *
> +	 * Unlike eh_device_reset_handler, queuecommand may still be called
> +	 * for other devices, and the LLD must call scsi_done for the commands
> +	 * that have been affected by the reset.
> +	 *
> +	 * If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds for
> +	 * scsi_device must be failed with DID_ABORT.
> +	 */
> +	int (* eh_async_device_reset_handler)(struct scsi_device *);
>  	int (* eh_target_reset_handler)(struct scsi_cmnd *);
>  	int (* eh_bus_reset_handler)(struct scsi_cmnd *);
>  	int (* eh_host_reset_handler)(struct scsi_cmnd *);
> -- 
> 2.7.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH 4/5] scsi: add new async device reset support

[Hannes Reinecke]

On 05/27/2016 10:23 AM, Christoph Hellwig wrote:
> Adding Hannes to the Cc list as he's been looking into EH improvements
> in this area.
>
> On Wed, May 25, 2016 at 02:55:02AM -0500, mchristi@redhat.com wrote:
>> From: Mike Christie <mchristi@redhat.com>
>>
>> Currently, if the SCSI eh runs then before we do a LUN_RESET
>> we stop the host. This patch and the block layer one before it
>> begin to add infrastructure to be able to do a LUN_RESET and
>> eventually do a transport level recovery without having to stop the
>> host.
>>
>> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
>> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
>> LLD manages the commands that are affected.
>>
>> eh_async_device_reset_handler:
>>
>> The LLD should perform a LUN RESET that affects all commands
>> that have been accepted by its queuecommand callout for the
>> device passed in to the callout. While the reset handler is running,
>> queuecommand will not be running or called for the device.
>>
>> Unlike eh_device_reset_handler, queuecommand may still be
>> called for other devices, and the LLD must call scsi_done for the
>> commands that have been affected by the reset.
>>
>> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
>> must be failed with DID_ABORT.
>>
>> Signed-off-by: Mike Christie <mchristi@redhat.com>
In general I like the approach.
I'll be looking into it more closely next week.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: J. Hawn, J. Guild, F. Imend�rffer, HRB 16746 (AG N�rnberg)

Re: [PATCH 4/5] scsi: add new async device reset support

[Hannes Reinecke]

On 05/27/2016 10:23 AM, Christoph Hellwig wrote:
> Adding Hannes to the Cc list as he's been looking into EH improvements
> in this area.
>
> On Wed, May 25, 2016 at 02:55:02AM -0500, mchristi@redhat.com wrote:
>> From: Mike Christie <mchristi@redhat.com>
>>
>> Currently, if the SCSI eh runs then before we do a LUN_RESET
>> we stop the host. This patch and the block layer one before it
>> begin to add infrastructure to be able to do a LUN_RESET and
>> eventually do a transport level recovery without having to stop the
>> host.
>>
>> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
>> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
>> LLD manages the commands that are affected.
>>
>> eh_async_device_reset_handler:
>>
>> The LLD should perform a LUN RESET that affects all commands
>> that have been accepted by its queuecommand callout for the
>> device passed in to the callout. While the reset handler is running,
>> queuecommand will not be running or called for the device.
>>
>> Unlike eh_device_reset_handler, queuecommand may still be
>> called for other devices, and the LLD must call scsi_done for the
>> commands that have been affected by the reset.
>>
>> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
>> must be failed with DID_ABORT.
>>
>> Signed-off-by: Mike Christie <mchristi@redhat.com>
In general I like the approach.
I'll be looking into it more closely next week.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 4/5] scsi: add new async device reset support

[Hannes Reinecke]

On 05/25/2016 09:55 AM, mchristi@redhat.com wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> Currently, if the SCSI eh runs then before we do a LUN_RESET
> we stop the host. This patch and the block layer one before it
> begin to add infrastructure to be able to do a LUN_RESET and
> eventually do a transport level recovery without having to stop the
> host.
> 
> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
> LLD manages the commands that are affected.
> 
> eh_async_device_reset_handler:
> 
> The LLD should perform a LUN RESET that affects all commands
> that have been accepted by its queuecommand callout for the
> device passed in to the callout. While the reset handler is running,
> queuecommand will not be running or called for the device.
> 
> Unlike eh_device_reset_handler, queuecommand may still be
> called for other devices, and the LLD must call scsi_done for the
> commands that have been affected by the reset.
> 
> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
> must be failed with DID_ABORT.
> 
Hmm. With this patch you essentially just replaced the existing
eh_device_reset_handler() with eh_async_device_request_handler().
So how does this differ from the original behaviour?
By the time we're calling it the SCSI host is already in EH, ie all
commands have been completed or failed, so why again do we need to
wait for the queue to be empty?

And how exactly can queuecommand be called for other devices, as the
host is already in EH?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 4/5] scsi: add new async device reset support

[Hannes Reinecke]

On 05/25/2016 09:55 AM, mchristi@redhat.com wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> Currently, if the SCSI eh runs then before we do a LUN_RESET
> we stop the host. This patch and the block layer one before it
> begin to add infrastructure to be able to do a LUN_RESET and
> eventually do a transport level recovery without having to stop the
> host.
> 
> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
> LLD manages the commands that are affected.
> 
> eh_async_device_reset_handler:
> 
> The LLD should perform a LUN RESET that affects all commands
> that have been accepted by its queuecommand callout for the
> device passed in to the callout. While the reset handler is running,
> queuecommand will not be running or called for the device.
> 
> Unlike eh_device_reset_handler, queuecommand may still be
> called for other devices, and the LLD must call scsi_done for the
> commands that have been affected by the reset.
> 
> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
> must be failed with DID_ABORT.
> 
Hmm. With this patch you essentially just replaced the existing
eh_device_reset_handler() with eh_async_device_request_handler().
So how does this differ from the original behaviour?
By the time we're calling it the SCSI host is already in EH, ie all
commands have been completed or failed, so why again do we need to
wait for the queue to be empty?

And how exactly can queuecommand be called for other devices, as the
host is already in EH?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 0/5] block/target queue/LUN reset support

[Hannes Reinecke]

On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
> Currently, for SCSI LUN_RESETs the target layer can only wait on
> bio/requests it has sent. This normally results in the LUN_RESET
> timing out on the initiator side and that SCSI error handler
> escalating to something more disruptive.
> 
> To fix this, the following patches add a block layer helper and
> callout to reset a request queue which the target layer can use
> to force drivers to complete/fail executing requests.
> 
> Patches were made over Jens's block tree's for-next branch.
> 
In general I like the approach, it just looks as if the main aim (ie
running a LUN RESET concurrent with normal I/O on other devices) is
not quite reached.

The general concept of eh_async_device_reset() is quite nice, and
renaming existing functions for doing so is okay, too.

It's just the integration with SCSI EH which is somewhat deficient
(as outlined in the comment on patch 3).
For the async device reset to work we'd need to call it _before_
SCSI EH is started, ie after the asynchronous command abort failed.

The easiest way would be to add per-device reset workqueue item,
which would be called whenever command abort failed.
As it's being per device we'd be getting an implicit serialisation,
and we could skip the lun reset from EH.

But then I'm not sure if we want to add yet another workqueue to the
scsi device ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 0/5] block/target queue/LUN reset support

[Hannes Reinecke]

On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
> Currently, for SCSI LUN_RESETs the target layer can only wait on
> bio/requests it has sent. This normally results in the LUN_RESET
> timing out on the initiator side and that SCSI error handler
> escalating to something more disruptive.
> 
> To fix this, the following patches add a block layer helper and
> callout to reset a request queue which the target layer can use
> to force drivers to complete/fail executing requests.
> 
> Patches were made over Jens's block tree's for-next branch.
> 
In general I like the approach, it just looks as if the main aim (ie
running a LUN RESET concurrent with normal I/O on other devices) is
not quite reached.

The general concept of eh_async_device_reset() is quite nice, and
renaming existing functions for doing so is okay, too.

It's just the integration with SCSI EH which is somewhat deficient
(as outlined in the comment on patch 3).
For the async device reset to work we'd need to call it _before_
SCSI EH is started, ie after the asynchronous command abort failed.

The easiest way would be to add per-device reset workqueue item,
which would be called whenever command abort failed.
As it's being per device we'd be getting an implicit serialisation,
and we could skip the lun reset from EH.

But then I'm not sure if we want to add yet another workqueue to the
scsi device ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] scsi: add new async device reset support

[Mike Christie]

On 05/30/2016 01:27 AM, Hannes Reinecke wrote:
> On 05/25/2016 09:55 AM, mchristi@redhat.com wrote:
>> From: Mike Christie <mchristi@redhat.com>
>>
>> Currently, if the SCSI eh runs then before we do a LUN_RESET
>> we stop the host. This patch and the block layer one before it
>> begin to add infrastructure to be able to do a LUN_RESET and
>> eventually do a transport level recovery without having to stop the
>> host.
>>
>> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
>> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
>> LLD manages the commands that are affected.
>>
>> eh_async_device_reset_handler:
>>
>> The LLD should perform a LUN RESET that affects all commands
>> that have been accepted by its queuecommand callout for the
>> device passed in to the callout. While the reset handler is running,
>> queuecommand will not be running or called for the device.
>>
>> Unlike eh_device_reset_handler, queuecommand may still be
>> called for other devices, and the LLD must call scsi_done for the
>> commands that have been affected by the reset.
>>
>> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
>> must be failed with DID_ABORT.
>>
> Hmm. With this patch you essentially just replaced the existing
> eh_device_reset_handler() with eh_async_device_request_handler().
> So how does this differ from the original behaviour?


1. LLD must call scsi_done and set host byte on each command affected by
the reset. This is what they have to do for the SG ioctl reset, but for
the scsi eh reset, LLDs do not have to because scsi-ml manages the
commands for them.

When doing a SG ioctl based reset or if the reset is called from the
target like in the last patch it is not possible to have scsi-ml track
the outstanding commands like we do today based on timeouts.

2. LLDs have to support commands to other luns during device resets, so
they cannot have any sort of host wide device resource lock/resource
that they can rely on. It has to be per device.

3. We can now support being able to do a lun reset without having to
stop then entire host.

> By the time we're calling it the SCSI host is already in EH, ie all
> commands have been completed or failed, so why again do we need to
> wait for the queue to be empty?

I am not sure what you mean here. The patches in this set never go into
host reset or even target level reset handling. For this set, we only
want to drivers to be able to do a device/lun reset whenever they are
asked to do so.

> 
> And how exactly can queuecommand be called for other devices, as the
> host is already in EH?
> 

Where in this patchset do we stop the host?

Re: [PATCH 0/5] block/target queue/LUN reset support

[Mike Christie]

On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>> Currently, for SCSI LUN_RESETs the target layer can only wait on
>> bio/requests it has sent. This normally results in the LUN_RESET
>> timing out on the initiator side and that SCSI error handler
>> escalating to something more disruptive.
>>
>> To fix this, the following patches add a block layer helper and
>> callout to reset a request queue which the target layer can use
>> to force drivers to complete/fail executing requests.
>>
>> Patches were made over Jens's block tree's for-next branch.
>>
> In general I like the approach, it just looks as if the main aim (ie
> running a LUN RESET concurrent with normal I/O on other devices) is
> not quite reached.
> 
> The general concept of eh_async_device_reset() is quite nice, and
> renaming existing functions for doing so is okay, too.
> 
> It's just the integration with SCSI EH which is somewhat deficient
> (as outlined in the comment on patch 3).
> For the async device reset to work we'd need to call it _before_
> SCSI EH is started, ie after the asynchronous command abort failed.

Yes that is my plan.

However, these first patches are only to allow LIO to be able to do
resets. I need the same infrastructure for both though.

> 
> The easiest way would be to add per-device reset workqueue item,
> which wold be called whenever command abort failed.

If you want to do this without stopping the entire host, you need the
patches like in this set where we stop and flush a queue.


> As it's being per device we'd be getting an implicit serialisation,
> and we could skip the lun reset from EH.

To build on my patches for a new async based scsi eh what we want to do is:

0. Add eh_async_target_reset callout which works like async device reset
one. For iscsi this maps to iscsi_eh_session_reset. FC drivers have
something similar in the code paths that call rc_remote_port_delete and
the terminate_rport_io paths. We just need wrappers.

1. scsi_times_out would kick off abort if needed and return
BLK_EH_RESET_TIMEOUT.
2. If abort fails, cancel queued aborts and call new async device reset
callout in these patches.
3. If device reset fails call new async target reset callout.
4. if target reset fails, let fail the block timeout timer and do the
old style scsi eh host reset.

It is really simple for newer drivers/classes like FC and iSCSI because
they handle the device and target/port level reset clean up already. The
difficult (not really difficult but messy) part is trying to support old
and new style EHs in a functions like scsi_times_out and scsi_abort_command.

Re: [PATCH 4/5] scsi: add new async device reset support

[Mike Christie]

On 05/31/2016 02:38 PM, Mike Christie wrote:
> On 05/30/2016 01:27 AM, Hannes Reinecke wrote:
>> On 05/25/2016 09:55 AM, mchristi@redhat.com wrote:
>>> From: Mike Christie <mchristi@redhat.com>
>>>
>>> Currently, if the SCSI eh runs then before we do a LUN_RESET
>>> we stop the host. This patch and the block layer one before it
>>> begin to add infrastructure to be able to do a LUN_RESET and
>>> eventually do a transport level recovery without having to stop the
>>> host.
>>>
>>> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
>>> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
>>> LLD manages the commands that are affected.
>>>
>>> eh_async_device_reset_handler:
>>>
>>> The LLD should perform a LUN RESET that affects all commands
>>> that have been accepted by its queuecommand callout for the
>>> device passed in to the callout. While the reset handler is running,
>>> queuecommand will not be running or called for the device.
>>>
>>> Unlike eh_device_reset_handler, queuecommand may still be
>>> called for other devices, and the LLD must call scsi_done for the
>>> commands that have been affected by the reset.
>>>
>>> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
>>> must be failed with DID_ABORT.
>>>
>> Hmm. With this patch you essentially just replaced the existing
>> eh_device_reset_handler() with eh_async_device_request_handler().
>> So how does this differ from the original behaviour?
> 
> 
> 1. LLD must call scsi_done and set host byte on each command affected by
> the reset. This is what they have to do for the SG ioctl reset, but for
> the scsi eh reset, LLDs do not have to because scsi-ml manages the
> commands for them.
> 
> When doing a SG ioctl based reset or if the reset is called from the
> target like in the last patch it is not possible to have scsi-ml track
> the outstanding commands like we do today based on timeouts.
> 
> 2. LLDs have to support commands to other luns during device resets, so
> they cannot have any sort of host wide device resource lock/resource
> that they can rely on. It has to be per device.
> 
> 3. We can now support being able to do a lun reset without having to
> stop then entire host.
> 
>> By the time we're calling it the SCSI host is already in EH, ie all
>> commands have been completed or failed, so why again do we need to
>> wait for the queue to be empty?
> 
> I am not sure what you mean here. The patches in this set never go into
> host reset or even target level reset handling. For this set, we only
> want to drivers to be able to do a device/lun reset whenever they are
> asked to do so.
> 
>>
>> And how exactly can queuecommand be called for other devices, as the
>> host is already in EH?
>>
> 
> Where in this patchset do we stop the host?
> 

Oh yeah, I mean new entry points I am adding in this set. In the 3rd
patch, we never start the scsi eh and we never stop the host. The target
layer just calls the new callout through the block layer helper added in
the 2nd patch.

Re: [PATCH 4/5] scsi: add new async device reset support

[Mike Christie]

On 05/31/2016 02:38 PM, Mike Christie wrote:
> On 05/30/2016 01:27 AM, Hannes Reinecke wrote:
>> On 05/25/2016 09:55 AM, mchristi@redhat.com wrote:
>>> From: Mike Christie <mchristi@redhat.com>
>>>
>>> Currently, if the SCSI eh runs then before we do a LUN_RESET
>>> we stop the host. This patch and the block layer one before it
>>> begin to add infrastructure to be able to do a LUN_RESET and
>>> eventually do a transport level recovery without having to stop the
>>> host.
>>>
>>> For LUn-reset, this patch adds a new callout, eh_async_device_reset_handler,
>>> which works similar to how LLDs handle SG_SCSI_RESET_DEVICE where the
>>> LLD manages the commands that are affected.
>>>
>>> eh_async_device_reset_handler:
>>>
>>> The LLD should perform a LUN RESET that affects all commands
>>> that have been accepted by its queuecommand callout for the
>>> device passed in to the callout. While the reset handler is running,
>>> queuecommand will not be running or called for the device.
>>>
>>> Unlike eh_device_reset_handler, queuecommand may still be
>>> called for other devices, and the LLD must call scsi_done for the
>>> commands that have been affected by the reset.
>>>
>>> If SUCCESS or FAST_IO_FAIL is returned, the scsi_cmnds cleaned up
>>> must be failed with DID_ABORT.
>>>
>> Hmm. With this patch you essentially just replaced the existing
>> eh_device_reset_handler() with eh_async_device_request_handler().
>> So how does this differ from the original behaviour?
> 
> 
> 1. LLD must call scsi_done and set host byte on each command affected by
> the reset. This is what they have to do for the SG ioctl reset, but for
> the scsi eh reset, LLDs do not have to because scsi-ml manages the
> commands for them.
> 
> When doing a SG ioctl based reset or if the reset is called from the
> target like in the last patch it is not possible to have scsi-ml track
> the outstanding commands like we do today based on timeouts.
> 
> 2. LLDs have to support commands to other luns during device resets, so
> they cannot have any sort of host wide device resource lock/resource
> that they can rely on. It has to be per device.
> 
> 3. We can now support being able to do a lun reset without having to
> stop then entire host.
> 

One other difference is that for the SG ioctl reset case, it is a pain
to handle the race where queuecommand and eh_device_reset_handler are
running while also trying to remove locks and atomics from the LLD and
move to mq. Probably when the SG reset code was made we had the
host_lock taken in queuecommand, so it was simple to handle. If we stop
the queue like in the second patch then the LLD at least does not have
to worry about this.

Re: [PATCH 0/5] block/target queue/LUN reset support

[Hannes Reinecke]

On 05/31/2016 09:56 PM, Mike Christie wrote:
> On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
>> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>>> Currently, for SCSI LUN_RESETs the target layer can only wait on
>>> bio/requests it has sent. This normally results in the LUN_RESET
>>> timing out on the initiator side and that SCSI error handler
>>> escalating to something more disruptive.
>>>
>>> To fix this, the following patches add a block layer helper and
>>> callout to reset a request queue which the target layer can use
>>> to force drivers to complete/fail executing requests.
>>>
>>> Patches were made over Jens's block tree's for-next branch.
>>>
>> In general I like the approach, it just looks as if the main aim (ie
>> running a LUN RESET concurrent with normal I/O on other devices) is
>> not quite reached.
>>
>> The general concept of eh_async_device_reset() is quite nice, and
>> renaming existing functions for doing so is okay, too.
>>
>> It's just the integration with SCSI EH which is somewhat deficient
>> (as outlined in the comment on patch 3).
>> For the async device reset to work we'd need to call it _before_
>> SCSI EH is started, ie after the asynchronous command abort failed.
> 
> Yes that is my plan.
> 
> However, these first patches are only to allow LIO to be able to do
> resets. I need the same infrastructure for both though.
> 
>>
>> The easiest way would be to add per-device reset workqueue item,
>> which wold be called whenever command abort failed.
> 
> If you want to do this without stopping the entire host, you need the
> patches like in this set where we stop and flush a queue.
> 
Sure.

>> As it's being per device we'd be getting an implicit serialisation,
>> and we could skip the lun reset from EH.
> 
> To build on my patches for a new async based scsi eh what we want to do is:
> 
> 0. Add eh_async_target_reset callout which works like async device reset
> one. For iscsi this maps to iscsi_eh_session_reset. FC drivers have
> something similar in the code paths that call rc_remote_port_delete and
> the terminate_rport_io paths. We just need wrappers.
> 
Actually, I was wondering whether we could layer the new async EH
infrastructure besides the original EH.

And the current 'target_reset' is completely wrong.
SAM-2 did away with the TARGET RESET TMF, so it's anyones guess if
a target reset is actually _implemented_.
What we really need, though, is a new 'eh_async_transport_reset'
function, which would reset the _transport_.
A transport failure is currently main (and I'm even tempted to say
the only) reason why EH is invoked.

> 1. scsi_times_out would kick off abort if needed and return
> BLK_EH_RESET_TIMEOUT.
> 2. If abort fails, cancel queued aborts and call new async device reset
> callout in these patches.
> 3. If device reset fails call new async target reset callout.
> 4. if target reset fails, let fail the block timeout timer and do the
> old style scsi eh host reset.
> 
I would suggest to replace 3. and 4. with:

3. If device reset fails call the new async transport reset callout
4. If transport reset fails fallback to the original SCSI EH
   (which would have abort and device reset callouts unset, so
   it'll start with a target reset)

That way we keep the existing behaviour (so we don't need to touch
the zillions of SCSI parallel drivers) _and_ will be able to model a
reasonably modern error handling.

> It is really simple for newer drivers/classes like FC and iSCSI because
> they handle the device and target/port level reset clean up already. The
> difficult (not really difficult but messy) part is trying to support old
> and new style EHs in a functions like scsi_times_out and scsi_abort_command.
> 
And indeed, that's the challenge.
But your patchset is a step into the right direction.
I see if I can make progress with it, although I'm currently busy
doing the next release so it might take some time.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 0/5] block/target queue/LUN reset support

[Hannes Reinecke]

On 05/31/2016 09:56 PM, Mike Christie wrote:
> On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
>> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>>> Currently, for SCSI LUN_RESETs the target layer can only wait on
>>> bio/requests it has sent. This normally results in the LUN_RESET
>>> timing out on the initiator side and that SCSI error handler
>>> escalating to something more disruptive.
>>>
>>> To fix this, the following patches add a block layer helper and
>>> callout to reset a request queue which the target layer can use
>>> to force drivers to complete/fail executing requests.
>>>
>>> Patches were made over Jens's block tree's for-next branch.
>>>
>> In general I like the approach, it just looks as if the main aim (ie
>> running a LUN RESET concurrent with normal I/O on other devices) is
>> not quite reached.
>>
>> The general concept of eh_async_device_reset() is quite nice, and
>> renaming existing functions for doing so is okay, too.
>>
>> It's just the integration with SCSI EH which is somewhat deficient
>> (as outlined in the comment on patch 3).
>> For the async device reset to work we'd need to call it _before_
>> SCSI EH is started, ie after the asynchronous command abort failed.
> 
> Yes that is my plan.
> 
> However, these first patches are only to allow LIO to be able to do
> resets. I need the same infrastructure for both though.
> 
>>
>> The easiest way would be to add per-device reset workqueue item,
>> which wold be called whenever command abort failed.
> 
> If you want to do this without stopping the entire host, you need the
> patches like in this set where we stop and flush a queue.
> 
Sure.

>> As it's being per device we'd be getting an implicit serialisation,
>> and we could skip the lun reset from EH.
> 
> To build on my patches for a new async based scsi eh what we want to do is:
> 
> 0. Add eh_async_target_reset callout which works like async device reset
> one. For iscsi this maps to iscsi_eh_session_reset. FC drivers have
> something similar in the code paths that call rc_remote_port_delete and
> the terminate_rport_io paths. We just need wrappers.
> 
Actually, I was wondering whether we could layer the new async EH
infrastructure besides the original EH.

And the current 'target_reset' is completely wrong.
SAM-2 did away with the TARGET RESET TMF, so it's anyones guess if
a target reset is actually _implemented_.
What we really need, though, is a new 'eh_async_transport_reset'
function, which would reset the _transport_.
A transport failure is currently main (and I'm even tempted to say
the only) reason why EH is invoked.

> 1. scsi_times_out would kick off abort if needed and return
> BLK_EH_RESET_TIMEOUT.
> 2. If abort fails, cancel queued aborts and call new async device reset
> callout in these patches.
> 3. If device reset fails call new async target reset callout.
> 4. if target reset fails, let fail the block timeout timer and do the
> old style scsi eh host reset.
> 
I would suggest to replace 3. and 4. with:

3. If device reset fails call the new async transport reset callout
4. If transport reset fails fallback to the original SCSI EH
   (which would have abort and device reset callouts unset, so
   it'll start with a target reset)

That way we keep the existing behaviour (so we don't need to touch
the zillions of SCSI parallel drivers) _and_ will be able to model a
reasonably modern error handling.

> It is really simple for newer drivers/classes like FC and iSCSI because
> they handle the device and target/port level reset clean up already. The
> difficult (not really difficult but messy) part is trying to support old
> and new style EHs in a functions like scsi_times_out and scsi_abort_command.
> 
And indeed, that's the challenge.
But your patchset is a step into the right direction.
I see if I can make progress with it, although I'm currently busy
doing the next release so it might take some time.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] block/target queue/LUN reset support

[Brian King]

On 6/1/16 1:05 AM, Hannes Reinecke wrote:
> On 05/31/2016 09:56 PM, Mike Christie wrote:
>> On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
>>> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>>>> Currently, for SCSI LUN_RESETs the target layer can only wait 
>>>> on bio/requests it has sent. This normally results in the 
>>>> LUN_RESET timing out on the initiator side and that SCSI error 
>>>> handler escalating to something more disruptive.
>>>> 
>>>> To fix this, the following patches add a block layer helper and
>>>> callout to reset a request queue which the target layer can use
>>>> to force drivers to complete/fail executing requests.
>>>> 
>>>> Patches were made over Jens's block tree's for-next branch.
>>>> 
>>> In general I like the approach, it just looks as if the main aim 
>>> (ie running a LUN RESET concurrent with normal I/O on other 
>>> devices) is not quite reached.
>>> 
>>> The general concept of eh_async_device_reset() is quite nice, and
>>> renaming existing functions for doing so is okay, too.
>>> 
>>> It's just the integration with SCSI EH which is somewhat 
>>> deficient (as outlined in the comment on patch 3). For the async 
>>> device reset to work we'd need to call it _before_ SCSI EH is 
>>> started, ie after the asynchronous command abort failed.
>> 
>> Yes that is my plan.
>> 
>> However, these first patches are only to allow LIO to be able to do
>> resets. I need the same infrastructure for both though.
>> 
>>> 
>>> The easiest way would be to add per-device reset workqueue item,
>>>  which wold be called whenever command abort failed.
>> 
>> If you want to do this without stopping the entire host, you need 
>> the patches like in this set where we stop and flush a queue.
>> 
> Sure.
> 
>>> As it's being per device we'd be getting an implicit 
>>> serialisation, and we could skip the lun reset from EH.
>> 
>> To build on my patches for a new async based scsi eh what we want 
>> to do is:
>> 
>> 0. Add eh_async_target_reset callout which works like async device 
>> reset one. For iscsi this maps to iscsi_eh_session_reset. FC 
>> drivers have something similar in the code paths that call 
>> rc_remote_port_delete and the terminate_rport_io paths. We just 
>> need wrappers.
>> 
> Actually, I was wondering whether we could layer the new async EH 
> infrastructure besides the original EH.
> 
> And the current 'target_reset' is completely wrong. SAM-2 did away 
> with the TARGET RESET TMF, so it's anyones guess if a target reset
> is actually _implemented_. What we really need, though, is a new 
> 'eh_async_transport_reset' function, which would reset the 
> _transport_. A transport failure is currently main (and I'm even 
> tempted to say the only) reason why EH is invoked.
> 
>> 1. scsi_times_out would kick off abort if needed and return 
>> BLK_EH_RESET_TIMEOUT. 2. If abort fails, cancel queued aborts and 
>> call new async device reset callout in these patches. 3. If device 
>> reset fails call new async target reset callout. 4. if target
>> reset fails, let fail the block timeout timer and do the old style
>> scsi eh host reset.
>> 
> I would suggest to replace 3. and 4. with:
> 
> 3. If device reset fails call the new async transport reset callout 
> 4. If transport reset fails fallback to the original SCSI EH (which 
> would have abort and device reset callouts unset, so it'll start
> with a target reset)
> 
> That way we keep the existing behaviour (so we don't need to touch 
> the zillions of SCSI parallel drivers) _and_ will be able to model a
>  reasonably modern error handling.
> 
>> It is really simple for newer drivers/classes like FC and iSCSI 
>> because they handle the device and target/port level reset clean
>> up already. The difficult (not really difficult but messy) part is 
>> trying to support old and new style EHs in a functions like 
>> scsi_times_out and scsi_abort_command.
>> 
> And indeed, that's the challenge. But your patchset is a step into 
> the right direction. I see if I can make progress with it, although 
> I'm currently busy doing the next release so it might take some 
> time.


Recently I've been looking at some issues we are seeing in the field with customers
that have very large storage configurations with lots and lots of SAS drives. We are seeing scenarios
where drive head failures and other issues are resulting in command aborts that then ultimately fail
and we then quiesce the HBA in order to do the LUN reset. Since this configuration has
hundreds of SAS disks under a single HBA, that results in a very noticeable I/O service time
problem for all the other disks under that HBA due to one misbehaving drive. We've so far
focused most our efforts on getting other components in the stack to behave differently
in order to mitigate the issue. However, that doesn't mean we can't do better
in the kernel. 

The direction this patch set was headed was to implement async LUN reset, something we've
discussed for years, but never fully implemented.  Is this something anyone else is still
seeing as an issue for them in other environments? Given that the last attempt at implementing
this, from what I can tell, happened now three years ago and then stalled, I'm afraid
I know the answer, but is anyone actively working on anything like this?

Thanks,

Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH 0/5] block/target queue/LUN reset support

[Brian King]

On 6/1/16 1:05 AM, Hannes Reinecke wrote:
> On 05/31/2016 09:56 PM, Mike Christie wrote:
>> On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
>>> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>>>> Currently, for SCSI LUN_RESETs the target layer can only wait 
>>>> on bio/requests it has sent. This normally results in the 
>>>> LUN_RESET timing out on the initiator side and that SCSI error 
>>>> handler escalating to something more disruptive.
>>>> 
>>>> To fix this, the following patches add a block layer helper and
>>>> callout to reset a request queue which the target layer can use
>>>> to force drivers to complete/fail executing requests.
>>>> 
>>>> Patches were made over Jens's block tree's for-next branch.
>>>> 
>>> In general I like the approach, it just looks as if the main aim 
>>> (ie running a LUN RESET concurrent with normal I/O on other 
>>> devices) is not quite reached.
>>> 
>>> The general concept of eh_async_device_reset() is quite nice, and
>>> renaming existing functions for doing so is okay, too.
>>> 
>>> It's just the integration with SCSI EH which is somewhat 
>>> deficient (as outlined in the comment on patch 3). For the async 
>>> device reset to work we'd need to call it _before_ SCSI EH is 
>>> started, ie after the asynchronous command abort failed.
>> 
>> Yes that is my plan.
>> 
>> However, these first patches are only to allow LIO to be able to do
>> resets. I need the same infrastructure for both though.
>> 
>>> 
>>> The easiest way would be to add per-device reset workqueue item,
>>>  which wold be called whenever command abort failed.
>> 
>> If you want to do this without stopping the entire host, you need 
>> the patches like in this set where we stop and flush a queue.
>> 
> Sure.
> 
>>> As it's being per device we'd be getting an implicit 
>>> serialisation, and we could skip the lun reset from EH.
>> 
>> To build on my patches for a new async based scsi eh what we want 
>> to do is:
>> 
>> 0. Add eh_async_target_reset callout which works like async device 
>> reset one. For iscsi this maps to iscsi_eh_session_reset. FC 
>> drivers have something similar in the code paths that call 
>> rc_remote_port_delete and the terminate_rport_io paths. We just 
>> need wrappers.
>> 
> Actually, I was wondering whether we could layer the new async EH 
> infrastructure besides the original EH.
> 
> And the current 'target_reset' is completely wrong. SAM-2 did away 
> with the TARGET RESET TMF, so it's anyones guess if a target reset
> is actually _implemented_. What we really need, though, is a new 
> 'eh_async_transport_reset' function, which would reset the 
> _transport_. A transport failure is currently main (and I'm even 
> tempted to say the only) reason why EH is invoked.
> 
>> 1. scsi_times_out would kick off abort if needed and return 
>> BLK_EH_RESET_TIMEOUT. 2. If abort fails, cancel queued aborts and 
>> call new async device reset callout in these patches. 3. If device 
>> reset fails call new async target reset callout. 4. if target
>> reset fails, let fail the block timeout timer and do the old style
>> scsi eh host reset.
>> 
> I would suggest to replace 3. and 4. with:
> 
> 3. If device reset fails call the new async transport reset callout 
> 4. If transport reset fails fallback to the original SCSI EH (which 
> would have abort and device reset callouts unset, so it'll start
> with a target reset)
> 
> That way we keep the existing behaviour (so we don't need to touch 
> the zillions of SCSI parallel drivers) _and_ will be able to model a
>  reasonably modern error handling.
> 
>> It is really simple for newer drivers/classes like FC and iSCSI 
>> because they handle the device and target/port level reset clean
>> up already. The difficult (not really difficult but messy) part is 
>> trying to support old and new style EHs in a functions like 
>> scsi_times_out and scsi_abort_command.
>> 
> And indeed, that's the challenge. But your patchset is a step into 
> the right direction. I see if I can make progress with it, although 
> I'm currently busy doing the next release so it might take some 
> time.


Recently I've been looking at some issues we are seeing in the field with customers
that have very large storage configurations with lots and lots of SAS drives. We are seeing scenarios
where drive head failures and other issues are resulting in command aborts that then ultimately fail
and we then quiesce the HBA in order to do the LUN reset. Since this configuration has
hundreds of SAS disks under a single HBA, that results in a very noticeable I/O service time
problem for all the other disks under that HBA due to one misbehaving drive. We've so far
focused most our efforts on getting other components in the stack to behave differently
in order to mitigate the issue. However, that doesn't mean we can't do better
in the kernel. 

The direction this patch set was headed was to implement async LUN reset, something we've
discussed for years, but never fully implemented.  Is this something anyone else is still
seeing as an issue for them in other environments? Given that the last attempt at implementing
this, from what I can tell, happened now three years ago and then stalled, I'm afraid
I know the answer, but is anyone actively working on anything like this?

Thanks,

Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH 0/5] block/target queue/LUN reset support

[Hannes Reinecke]

On 5/2/19 11:29 PM, Brian King wrote:
> On 6/1/16 1:05 AM, Hannes Reinecke wrote:
>> On 05/31/2016 09:56 PM, Mike Christie wrote:
>>> On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
>>>> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>>>>> Currently, for SCSI LUN_RESETs the target layer can only wait
>>>>> on bio/requests it has sent. This normally results in the
>>>>> LUN_RESET timing out on the initiator side and that SCSI error
>>>>> handler escalating to something more disruptive.
>>>>>
>>>>> To fix this, the following patches add a block layer helper and
>>>>> callout to reset a request queue which the target layer can use
>>>>> to force drivers to complete/fail executing requests.
>>>>>
>>>>> Patches were made over Jens's block tree's for-next branch.
>>>>>
>>>> In general I like the approach, it just looks as if the main aim
>>>> (ie running a LUN RESET concurrent with normal I/O on other
>>>> devices) is not quite reached.
>>>>
>>>> The general concept of eh_async_device_reset() is quite nice, and
>>>> renaming existing functions for doing so is okay, too.
>>>>
>>>> It's just the integration with SCSI EH which is somewhat
>>>> deficient (as outlined in the comment on patch 3). For the async
>>>> device reset to work we'd need to call it _before_ SCSI EH is
>>>> started, ie after the asynchronous command abort failed.
>>>
>>> Yes that is my plan.
>>>
>>> However, these first patches are only to allow LIO to be able to do
>>> resets. I need the same infrastructure for both though.
>>>
>>>>
>>>> The easiest way would be to add per-device reset workqueue item,
>>>>   which wold be called whenever command abort failed.
>>>
>>> If you want to do this without stopping the entire host, you need
>>> the patches like in this set where we stop and flush a queue.
>>>
>> Sure.
>>
>>>> As it's being per device we'd be getting an implicit
>>>> serialisation, and we could skip the lun reset from EH.
>>>
>>> To build on my patches for a new async based scsi eh what we want
>>> to do is:
>>>
>>> 0. Add eh_async_target_reset callout which works like async device
>>> reset one. For iscsi this maps to iscsi_eh_session_reset. FC
>>> drivers have something similar in the code paths that call
>>> rc_remote_port_delete and the terminate_rport_io paths. We just
>>> need wrappers.
>>>
>> Actually, I was wondering whether we could layer the new async EH
>> infrastructure besides the original EH.
>>
>> And the current 'target_reset' is completely wrong. SAM-2 did away
>> with the TARGET RESET TMF, so it's anyones guess if a target reset
>> is actually _implemented_. What we really need, though, is a new
>> 'eh_async_transport_reset' function, which would reset the
>> _transport_. A transport failure is currently main (and I'm even
>> tempted to say the only) reason why EH is invoked.
>>
>>> 1. scsi_times_out would kick off abort if needed and return
>>> BLK_EH_RESET_TIMEOUT. 2. If abort fails, cancel queued aborts and
>>> call new async device reset callout in these patches. 3. If device
>>> reset fails call new async target reset callout. 4. if target
>>> reset fails, let fail the block timeout timer and do the old style
>>> scsi eh host reset.
>>>
>> I would suggest to replace 3. and 4. with:
>>
>> 3. If device reset fails call the new async transport reset callout
>> 4. If transport reset fails fallback to the original SCSI EH (which
>> would have abort and device reset callouts unset, so it'll start
>> with a target reset)
>>
>> That way we keep the existing behaviour (so we don't need to touch
>> the zillions of SCSI parallel drivers) _and_ will be able to model a
>>   reasonably modern error handling.
>>
>>> It is really simple for newer drivers/classes like FC and iSCSI
>>> because they handle the device and target/port level reset clean
>>> up already. The difficult (not really difficult but messy) part is
>>> trying to support old and new style EHs in a functions like
>>> scsi_times_out and scsi_abort_command.
>>>
>> And indeed, that's the challenge. But your patchset is a step into
>> the right direction. I see if I can make progress with it, although
>> I'm currently busy doing the next release so it might take some
>> time.
> 
> 
> Recently I've been looking at some issues we are seeing in the field with customers
> that have very large storage configurations with lots and lots of SAS drives. We are seeing scenarios
> where drive head failures and other issues are resulting in command aborts that then ultimately fail
> and we then quiesce the HBA in order to do the LUN reset. Since this configuration has
> hundreds of SAS disks under a single HBA, that results in a very noticeable I/O service time
> problem for all the other disks under that HBA due to one misbehaving drive. We've so far
> focused most our efforts on getting other components in the stack to behave differently
> in order to mitigate the issue. However, that doesn't mean we can't do better
> in the kernel.
> 
> The direction this patch set was headed was to implement async LUN reset, something we've
> discussed for years, but never fully implemented.  Is this something anyone else is still
> seeing as an issue for them in other environments? Given that the last attempt at implementing
> this, from what I can tell, happened now three years ago and then stalled, I'm afraid
> I know the answer, but is anyone actively working on anything like this?
> 
Not on async LUN Reset, no.
But I'm working on a patchset to update/fix SCSI EH yet again, by 
dropping the 'scmd' argument for the various SCSI EH callbacks, and use 
the respective object (eg SCSI device for device reset, SCSI Host for 
host reset etc) as arguments instead.
This gets rid of quite some complexity in the callbacks, and allows us 
to finally handle out-of-band resets via ioctl properly.

Once that is done we could be doing async (or, rather, parallel) LUN 
Resets, as each of the reset is in principle independent, and we're not 
bound to individual SCSI commands anymore.

I really need to polish off my patchset ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 0/5] block/target queue/LUN reset support

[Hannes Reinecke]

On 5/2/19 11:29 PM, Brian King wrote:
> On 6/1/16 1:05 AM, Hannes Reinecke wrote:
>> On 05/31/2016 09:56 PM, Mike Christie wrote:
>>> On 05/30/2016 01:37 AM, Hannes Reinecke wrote:
>>>> On 05/25/2016 09:54 AM, mchristi@redhat.com wrote:
>>>>> Currently, for SCSI LUN_RESETs the target layer can only wait
>>>>> on bio/requests it has sent. This normally results in the
>>>>> LUN_RESET timing out on the initiator side and that SCSI error
>>>>> handler escalating to something more disruptive.
>>>>>
>>>>> To fix this, the following patches add a block layer helper and
>>>>> callout to reset a request queue which the target layer can use
>>>>> to force drivers to complete/fail executing requests.
>>>>>
>>>>> Patches were made over Jens's block tree's for-next branch.
>>>>>
>>>> In general I like the approach, it just looks as if the main aim
>>>> (ie running a LUN RESET concurrent with normal I/O on other
>>>> devices) is not quite reached.
>>>>
>>>> The general concept of eh_async_device_reset() is quite nice, and
>>>> renaming existing functions for doing so is okay, too.
>>>>
>>>> It's just the integration with SCSI EH which is somewhat
>>>> deficient (as outlined in the comment on patch 3). For the async
>>>> device reset to work we'd need to call it _before_ SCSI EH is
>>>> started, ie after the asynchronous command abort failed.
>>>
>>> Yes that is my plan.
>>>
>>> However, these first patches are only to allow LIO to be able to do
>>> resets. I need the same infrastructure for both though.
>>>
>>>>
>>>> The easiest way would be to add per-device reset workqueue item,
>>>>   which wold be called whenever command abort failed.
>>>
>>> If you want to do this without stopping the entire host, you need
>>> the patches like in this set where we stop and flush a queue.
>>>
>> Sure.
>>
>>>> As it's being per device we'd be getting an implicit
>>>> serialisation, and we could skip the lun reset from EH.
>>>
>>> To build on my patches for a new async based scsi eh what we want
>>> to do is:
>>>
>>> 0. Add eh_async_target_reset callout which works like async device
>>> reset one. For iscsi this maps to iscsi_eh_session_reset. FC
>>> drivers have something similar in the code paths that call
>>> rc_remote_port_delete and the terminate_rport_io paths. We just
>>> need wrappers.
>>>
>> Actually, I was wondering whether we could layer the new async EH
>> infrastructure besides the original EH.
>>
>> And the current 'target_reset' is completely wrong. SAM-2 did away
>> with the TARGET RESET TMF, so it's anyones guess if a target reset
>> is actually _implemented_. What we really need, though, is a new
>> 'eh_async_transport_reset' function, which would reset the
>> _transport_. A transport failure is currently main (and I'm even
>> tempted to say the only) reason why EH is invoked.
>>
>>> 1. scsi_times_out would kick off abort if needed and return
>>> BLK_EH_RESET_TIMEOUT. 2. If abort fails, cancel queued aborts and
>>> call new async device reset callout in these patches. 3. If device
>>> reset fails call new async target reset callout. 4. if target
>>> reset fails, let fail the block timeout timer and do the old style
>>> scsi eh host reset.
>>>
>> I would suggest to replace 3. and 4. with:
>>
>> 3. If device reset fails call the new async transport reset callout
>> 4. If transport reset fails fallback to the original SCSI EH (which
>> would have abort and device reset callouts unset, so it'll start
>> with a target reset)
>>
>> That way we keep the existing behaviour (so we don't need to touch
>> the zillions of SCSI parallel drivers) _and_ will be able to model a
>>   reasonably modern error handling.
>>
>>> It is really simple for newer drivers/classes like FC and iSCSI
>>> because they handle the device and target/port level reset clean
>>> up already. The difficult (not really difficult but messy) part is
>>> trying to support old and new style EHs in a functions like
>>> scsi_times_out and scsi_abort_command.
>>>
>> And indeed, that's the challenge. But your patchset is a step into
>> the right direction. I see if I can make progress with it, although
>> I'm currently busy doing the next release so it might take some
>> time.
> 
> 
> Recently I've been looking at some issues we are seeing in the field with customers
> that have very large storage configurations with lots and lots of SAS drives. We are seeing scenarios
> where drive head failures and other issues are resulting in command aborts that then ultimately fail
> and we then quiesce the HBA in order to do the LUN reset. Since this configuration has
> hundreds of SAS disks under a single HBA, that results in a very noticeable I/O service time
> problem for all the other disks under that HBA due to one misbehaving drive. We've so far
> focused most our efforts on getting other components in the stack to behave differently
> in order to mitigate the issue. However, that doesn't mean we can't do better
> in the kernel.
> 
> The direction this patch set was headed was to implement async LUN reset, something we've
> discussed for years, but never fully implemented.  Is this something anyone else is still
> seeing as an issue for them in other environments? Given that the last attempt at implementing
> this, from what I can tell, happened now three years ago and then stalled, I'm afraid
> I know the answer, but is anyone actively working on anything like this?
> 
Not on async LUN Reset, no.
But I'm working on a patchset to update/fix SCSI EH yet again, by 
dropping the 'scmd' argument for the various SCSI EH callbacks, and use 
the respective object (eg SCSI device for device reset, SCSI Host for 
host reset etc) as arguments instead.
This gets rid of quite some complexity in the callbacks, and allows us 
to finally handle out-of-band resets via ioctl properly.

Once that is done we could be doing async (or, rather, parallel) LUN 
Resets, as each of the reset is in principle independent, and we're not 
bound to individual SCSI commands anymore.

I really need to polish off my patchset ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)