[PATCHv2 0/7] Limit overall SCSI EH runtime

[PATCHv2 0/7] Limit overall SCSI EH runtime

[Hannes Reinecke]

This patchset implements a new 'eh_deadline' attribute to the
SCSI host. It will limit the overall SCSI EH runtime by a given
timeout. If the timeout is reached all intermediate EH steps
will be skipped and host reset will be scheduled immediately.

For this patch I've re-used the existing 'last_reset' field
of the SCSI host to store the initial time SCSI EH started.
Also the field 'resetting' has been removed as it never has
been used as intended.

As 'last_reset' might be in use by transport-specific EH
implementation I've disallowed eh_deadline setting there.

Changes from the initial version:
- Add list_splice_init() calls to avoid stale commands
- Rename function to scsi_host_eh_past_deadline

Hannes Reinecke (7):
  dpt_i2o: Remove DPTI_STATE_IOCTL
  dpt_i2o: return SCSI_MLQUEUE_HOST_BUSY when in reset
  advansys: Remove 'last_reset' references
  tmscsim: Move 'last_reset' into host structure
  dc395: Move 'last_reset' into internal host structure
  scsi: remove check for 'resetting'
  scsi: Add 'eh_deadline' to limit SCSI EH runtime

 drivers/scsi/advansys.c   |   8 +--
 drivers/scsi/dc395x.c     |  24 +++++----
 drivers/scsi/dpt_i2o.c    |  35 +++++--------
 drivers/scsi/dpti.h       |   1 -
 drivers/scsi/hosts.c      |   7 +++
 drivers/scsi/scsi.c       |  28 ----------
 drivers/scsi/scsi_error.c | 130 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/scsi_sysfs.c |  37 +++++++++++++
 drivers/scsi/tmscsim.c    |  14 ++---
 drivers/scsi/tmscsim.h    |   1 +
 include/scsi/scsi_host.h  |   4 +-
 11 files changed, 208 insertions(+), 81 deletions(-)

-- 
1.7.12.4

[PATCH 1/7] dpt_i2o: Remove DPTI_STATE_IOCTL

[Hannes Reinecke]

scsi_block_host/scsi_unlock_host provides the required
functionality.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dpt_i2o.c | 30 ++++++++++++------------------
 drivers/scsi/dpti.h    |  1 -
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 19e1b42..106ff1a 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -448,15 +448,7 @@ static int adpt_queue_lck(struct scsi_cmnd * cmd, void (*done) (struct scsi_cmnd
 	}
 
 	rmb();
-	/*
-	 * TODO: I need to block here if I am processing ioctl cmds
-	 * but if the outstanding cmds all finish before the ioctl,
-	 * the scsi-core will not know to start sending cmds to me again.
-	 * I need to a way to restart the scsi-cores queues or should I block
-	 * calling scsi_done on the outstanding cmds instead
-	 * for now we don't set the IOCTL state
-	 */
-	if(((pHba->state) & DPTI_STATE_IOCTL) || ((pHba->state) & DPTI_STATE_RESET)) {
+	if((pHba->state) & DPTI_STATE_RESET) {
 		pHba->host->last_reset = jiffies;
 		pHba->host->resetting = 1;
 		return 1;
@@ -1811,21 +1803,23 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user *arg)
 	}
 
 	do {
-		if(pHba->host)
+		/*
+		 * Stop any new commands from enterring the
+		 * controller while processing the ioctl
+		 */
+		if(pHba->host) {
+			scsi_block_requests(pHba->host);
 			spin_lock_irqsave(pHba->host->host_lock, flags);
-		// This state stops any new commands from enterring the
-		// controller while processing the ioctl
-//		pHba->state |= DPTI_STATE_IOCTL;
-//		We can't set this now - The scsi subsystem sets host_blocked and
-//		the queue empties and stops.  We need a way to restart the queue
+		}
 		rcode = adpt_i2o_post_wait(pHba, msg, size, FOREVER);
 		if (rcode != 0)
 			printk("adpt_i2o_passthru: post wait failed %d %p\n",
 					rcode, reply);
-//		pHba->state &= ~DPTI_STATE_IOCTL;
-		if(pHba->host)
+		if(pHba->host) {
 			spin_unlock_irqrestore(pHba->host->host_lock, flags);
-	} while(rcode == -ETIMEDOUT);  
+			scsi_unblock_requests(pHba->host);
+		}
+	} while(rcode == -ETIMEDOUT);
 
 	if(rcode){
 		goto cleanup;
diff --git a/drivers/scsi/dpti.h b/drivers/scsi/dpti.h
index beded71..aeb0461 100644
--- a/drivers/scsi/dpti.h
+++ b/drivers/scsi/dpti.h
@@ -202,7 +202,6 @@ struct adpt_channel {
 
 // HBA state flags
 #define DPTI_STATE_RESET	(0x01)
-#define DPTI_STATE_IOCTL	(0x02)
 
 typedef struct _adpt_hba {
 	struct _adpt_hba *next;
-- 
1.7.12.4

[PATCH 2/7] dpt_i2o: return SCSI_MLQUEUE_HOST_BUSY when in reset

[Hannes Reinecke]

When the HBA is in reset we should be returning 'busy' and not
rely on the obscure 'last_reset' feature.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dpt_i2o.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 106ff1a..0a2d801 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -448,11 +448,8 @@ static int adpt_queue_lck(struct scsi_cmnd * cmd, void (*done) (struct scsi_cmnd
 	}
 
 	rmb();
-	if((pHba->state) & DPTI_STATE_RESET) {
-		pHba->host->last_reset = jiffies;
-		pHba->host->resetting = 1;
-		return 1;
-	}
+	if((pHba->state) & DPTI_STATE_RESET)
+		return SCSI_MLQUEUE_HOST_BUSY;
 
 	// TODO if the cmd->device if offline then I may need to issue a bus rescan
 	// followed by a get_lct to see if the device is there anymore
-- 
1.7.12.4

[PATCH 3/7] advansys: Remove 'last_reset' references

[Hannes Reinecke]

Serves no purpose whatsoever.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/advansys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index c67e401..d814588 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -2511,8 +2511,8 @@ static void asc_prt_scsi_host(struct Scsi_Host *s)
 	struct asc_board *boardp = shost_priv(s);
 
 	printk("Scsi_Host at addr 0x%p, device %s\n", s, dev_name(boardp->dev));
-	printk(" host_busy %u, host_no %d, last_reset %d,\n",
-	       s->host_busy, s->host_no, (unsigned)s->last_reset);
+	printk(" host_busy %u, host_no %d,\n",
+	       s->host_busy, s->host_no);
 
 	printk(" base 0x%lx, io_port 0x%lx, irq %d,\n",
 	       (ulong)s->base, (ulong)s->io_port, boardp->irq);
@@ -3345,8 +3345,8 @@ static void asc_prt_driver_conf(struct seq_file *m, struct Scsi_Host *shost)
 		shost->host_no);
 
 	seq_printf(m,
-		   " host_busy %u, last_reset %lu, max_id %u, max_lun %u, max_channel %u\n",
-		   shost->host_busy, shost->last_reset, shost->max_id,
+		   " host_busy %u, max_id %u, max_lun %u, max_channel %u\n",
+		   shost->host_busy, shost->max_id,
 		   shost->max_lun, shost->max_channel);
 
 	seq_printf(m,
-- 
1.7.12.4

[PATCH 4/7] tmscsim: Move 'last_reset' into host structure

[Hannes Reinecke]

The 'last_reset' value is only used internally, so move it into
the internal host structure.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/tmscsim.c | 14 +++++++-------
 drivers/scsi/tmscsim.h |  1 +
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index 9327f5f..1142361 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -521,7 +521,7 @@ dc390_StartSCSI( struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_sr
 	pACB->SelConn++;
 	return 1;
     }
-    if (time_before (jiffies, pACB->pScsiHost->last_reset))
+    if (time_before (jiffies, pACB->last_reset))
     {
 	DEBUG0(printk ("DC390: We were just reset and don't accept commands yet!\n"));
 	return 1;
@@ -1863,7 +1863,7 @@ dc390_ScsiRstDetect( struct dc390_acb* pACB )
     /* delay half a second */
     udelay (1000);
     DC390_write8 (ScsiCmd, CLEAR_FIFO_CMD);
-    pACB->pScsiHost->last_reset = jiffies + 5*HZ/2
+    pACB->last_reset = jiffies + 5*HZ/2
 		    + HZ * dc390_eepromBuf[pACB->AdapterIndex][EE_DELAY];
     pACB->Connected = 0;
 
@@ -2048,9 +2048,9 @@ static int DC390_bus_reset (struct scsi_cmnd *cmd)
 
 	dc390_ResetDevParam(pACB);
 	mdelay(1);
-	pACB->pScsiHost->last_reset = jiffies + 3*HZ/2 
+	pACB->last_reset = jiffies + 3*HZ/2
 		+ HZ * dc390_eepromBuf[pACB->AdapterIndex][EE_DELAY];
-    
+
 	DC390_write8(ScsiCmd, CLEAR_FIFO_CMD);
 	DC390_read8(INT_Status);		/* Reset Pending INT */
 
@@ -2383,7 +2383,7 @@ static void dc390_init_hw(struct dc390_acb *pACB, u8 index)
 	if (pACB->Gmode2 & RST_SCSI_BUS) {
 		dc390_ResetSCSIBus(pACB);
 		udelay(1000);
-		shost->last_reset = jiffies + HZ/2 +
+		pACB->last_reset = jiffies + HZ/2 +
 			HZ * dc390_eepromBuf[pACB->AdapterIndex][EE_DELAY];
 	}
 
@@ -2455,8 +2455,8 @@ static int dc390_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	shost->irq = pdev->irq;
 	shost->base = io_port;
 	shost->unique_id = io_port;
-	shost->last_reset = jiffies;
-	
+
+	pACB->last_reset = jiffies;
 	pACB->pScsiHost = shost;
 	pACB->IOPortBase = (u16) io_port;
 	pACB->IRQLevel = pdev->irq;
diff --git a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
index 77adc54..3d1bb4a 100644
--- a/drivers/scsi/tmscsim.h
+++ b/drivers/scsi/tmscsim.h
@@ -143,6 +143,7 @@ u8		Ignore_IRQ;	/* Not used */
 
 struct pci_dev	*pdev;
 
+unsigned long   last_reset;
 unsigned long	Cmds;
 u32		SelLost;
 u32		SelConn;
-- 
1.7.12.4

[PATCH 5/7] dc395: Move 'last_reset' into internal host structure

[Hannes Reinecke]

'last_reset' is only used internally, so move it into the
internal host structure.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dc395x.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/dc395x.c b/drivers/scsi/dc395x.c
index 694e13c..42e8624 100644
--- a/drivers/scsi/dc395x.c
+++ b/drivers/scsi/dc395x.c
@@ -308,6 +308,8 @@ struct AdapterCtlBlk {
 	struct timer_list waiting_timer;
 	struct timer_list selto_timer;
 
+	unsigned long last_reset;
+
 	u16 srb_count;
 
 	u8 sel_timeout;
@@ -860,9 +862,9 @@ static void waiting_set_timer(struct AdapterCtlBlk *acb, unsigned long to)
 	init_timer(&acb->waiting_timer);
 	acb->waiting_timer.function = waiting_timeout;
 	acb->waiting_timer.data = (unsigned long) acb;
-	if (time_before(jiffies + to, acb->scsi_host->last_reset - HZ / 2))
+	if (time_before(jiffies + to, acb->last_reset - HZ / 2))
 		acb->waiting_timer.expires =
-		    acb->scsi_host->last_reset - HZ / 2 + 1;
+		    acb->last_reset - HZ / 2 + 1;
 	else
 		acb->waiting_timer.expires = jiffies + to + 1;
 	add_timer(&acb->waiting_timer);
@@ -1319,7 +1321,7 @@ static int __dc395x_eh_bus_reset(struct scsi_cmnd *cmd)
 	udelay(500);
 
 	/* We may be in serious trouble. Wait some seconds */
-	acb->scsi_host->last_reset =
+	acb->last_reset =
 	    jiffies + 3 * HZ / 2 +
 	    HZ * acb->eeprom.delay_time;
 
@@ -1462,9 +1464,9 @@ static void selto_timer(struct AdapterCtlBlk *acb)
 	acb->selto_timer.function = selection_timeout_missed;
 	acb->selto_timer.data = (unsigned long) acb;
 	if (time_before
-	    (jiffies + HZ, acb->scsi_host->last_reset + HZ / 2))
+	    (jiffies + HZ, acb->last_reset + HZ / 2))
 		acb->selto_timer.expires =
-		    acb->scsi_host->last_reset + HZ / 2 + 1;
+		    acb->last_reset + HZ / 2 + 1;
 	else
 		acb->selto_timer.expires = jiffies + HZ + 1;
 	add_timer(&acb->selto_timer);
@@ -1535,7 +1537,7 @@ static u8 start_scsi(struct AdapterCtlBlk* acb, struct DeviceCtlBlk* dcb,
 	}
 	/* Allow starting of SCSI commands half a second before we allow the mid-level
 	 * to queue them again after a reset */
-	if (time_before(jiffies, acb->scsi_host->last_reset - HZ / 2)) {
+	if (time_before(jiffies, acb->last_reset - HZ / 2)) {
 		dprintkdbg(DBG_KG, "start_scsi: Refuse cmds (reset wait)\n");
 		return 1;
 	}
@@ -3031,7 +3033,7 @@ static void disconnect(struct AdapterCtlBlk *acb)
 		dprintkl(KERN_ERR, "disconnect: No such device\n");
 		udelay(500);
 		/* Suspend queue for a while */
-		acb->scsi_host->last_reset =
+		acb->last_reset =
 		    jiffies + HZ / 2 +
 		    HZ * acb->eeprom.delay_time;
 		clear_fifo(acb, "disconnectEx");
@@ -3053,7 +3055,7 @@ static void disconnect(struct AdapterCtlBlk *acb)
 		waiting_process_next(acb);
 	} else if (srb->state & SRB_ABORT_SENT) {
 		dcb->flag &= ~ABORT_DEV_;
-		acb->scsi_host->last_reset = jiffies + HZ / 2 + 1;
+		acb->last_reset = jiffies + HZ / 2 + 1;
 		dprintkl(KERN_ERR, "disconnect: SRB_ABORT_SENT\n");
 		doing_srb_done(acb, DID_ABORT, srb->cmd, 1);
 		waiting_process_next(acb);
@@ -3649,7 +3651,7 @@ static void scsi_reset_detect(struct AdapterCtlBlk *acb)
 	/*DC395x_write8(acb, TRM_S1040_DMA_CONTROL,STOPDMAXFER); */
 	udelay(500);
 	/* Maybe we locked up the bus? Then lets wait even longer ... */
-	acb->scsi_host->last_reset =
+	acb->last_reset =
 	    jiffies + 5 * HZ / 2 +
 	    HZ * acb->eeprom.delay_time;
 
@@ -4426,7 +4428,7 @@ static void adapter_init_scsi_host(struct Scsi_Host *host)
 	host->dma_channel = -1;
 	host->unique_id = acb->io_port_base;
 	host->irq = acb->irq_level;
-	host->last_reset = jiffies;
+	acb->last_reset = jiffies;
 
 	host->max_id = 16;
 	if (host->max_id - 1 == eeprom->scsi_id)
@@ -4484,7 +4486,7 @@ static void adapter_init_chip(struct AdapterCtlBlk *acb)
 		/*spin_unlock_irq (&io_request_lock); */
 		udelay(500);
 
-		acb->scsi_host->last_reset =
+		acb->last_reset =
 		    jiffies + HZ / 2 +
 		    HZ * acb->eeprom.delay_time;
 
-- 
1.7.12.4

[PATCH 6/7] scsi: remove check for 'resetting'

[Hannes Reinecke]

Field is now unused, so this is dead code.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2c0d0ec..ebe3b0a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -78,11 +78,6 @@ static void scsi_done(struct scsi_cmnd *cmd);
  * Definitions and constants.
  */
 
-#define MIN_RESET_DELAY (2*HZ)
-
-/* Do not call reset on error if we just did a reset within 15 sec. */
-#define MIN_RESET_PERIOD (15*HZ)
-
 /*
  * Note - the initial logging level can be set here to log events at boot time.
  * After the system is up, you may enable logging via the /proc interface.
@@ -658,7 +653,6 @@ EXPORT_SYMBOL(scsi_cmd_get_serial);
 int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 {
 	struct Scsi_Host *host = cmd->device->host;
-	unsigned long timeout;
 	int rtn = 0;
 
 	atomic_inc(&cmd->device->iorequest_cnt);
@@ -704,28 +698,6 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 			       (cmd->device->lun << 5 & 0xe0);
 	}
 
-	/*
-	 * We will wait MIN_RESET_DELAY clock ticks after the last reset so
-	 * we can avoid the drive not being ready.
-	 */
-	timeout = host->last_reset + MIN_RESET_DELAY;
-
-	if (host->resetting && time_before(jiffies, timeout)) {
-		int ticks_remaining = timeout - jiffies;
-		/*
-		 * NOTE: This may be executed from within an interrupt
-		 * handler!  This is bad, but for now, it'll do.  The irq
-		 * level of the interrupt handler has been masked out by the
-		 * platform dependent interrupt handling code already, so the
-		 * sti() here will not cause another call to the SCSI host's
-		 * interrupt handler (assuming there is one irq-level per
-		 * host).
-		 */
-		while (--ticks_remaining >= 0)
-			mdelay(1 + 999 / HZ);
-		host->resetting = 0;
-	}
-
 	scsi_log_send(cmd);
 
 	/*
-- 
1.7.12.4

[PATCH 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[Hannes Reinecke]

This patchs adds an 'eh_deadline' sysfs attribute to the scsi
host which limits the overall runtime of the SCSI EH.
The 'eh_deadline' value is stored in the now obsolete field
'resetting'.
When a command is failed the start time of the EH is stored
in 'last_reset'. If the overall runtime of the SCSI EH is longer
than last_reset + eh_deadline, the EH is short-circuited and
falls through to issue a host reset only.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c      |   7 +++
 drivers/scsi/scsi_error.c | 130 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/scsi_sysfs.c |  37 +++++++++++++
 include/scsi/scsi_host.h  |   4 +-
 4 files changed, 170 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index df0c3c7..f334859 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -316,6 +316,12 @@ static void scsi_host_dev_release(struct device *dev)
 	kfree(shost);
 }
 
+static unsigned int shost_eh_deadline;
+
+module_param_named(eh_deadline, shost_eh_deadline, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(eh_deadline,
+		 "SCSI EH timeout in seconds (should be between 1 and 2^32-1)");
+
 static struct device_type scsi_host_type = {
 	.name =		"scsi_host",
 	.release =	scsi_host_dev_release,
@@ -388,6 +394,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
+	shost->eh_deadline = shost_eh_deadline * HZ;
 
 	if (sht->supported_mode == MODE_UNKNOWN)
 		/* means we didn't set it ... default to INITIATOR */
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f43de1e..84369f2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -89,6 +89,18 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL_GPL(scsi_schedule_eh);
 
+static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
+{
+	if (!shost->last_reset || !shost->eh_deadline)
+		return 0;
+
+	if (time_before(jiffies,
+			shost->last_reset + shost->eh_deadline))
+		return 0;
+
+	return 1;
+}
+
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
@@ -111,6 +123,9 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
 			goto out_unlock;
 
+	if (shost->eh_deadline && !shost->last_reset)
+		shost->last_reset = jiffies;
+
 	ret = 1;
 	scmd->eh_eflags |= eh_flag;
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
@@ -140,6 +155,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 	trace_scsi_dispatch_cmd_timeout(scmd);
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
 
+	if (host->eh_deadline && !host->last_reset)
+		host->last_reset = jiffies;
+
 	if (host->transportt->eh_timed_out)
 		rtn = host->transportt->eh_timed_out(scmd);
 	else if (host->hostt->eh_timed_out)
@@ -928,13 +946,26 @@ int scsi_eh_get_sense(struct list_head *work_q,
 		      struct list_head *done_q)
 {
 	struct scsi_cmnd *scmd, *next;
+	struct Scsi_Host *shost;
 	int rtn;
+	unsigned long flags;
 
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
 		if ((scmd->eh_eflags & SCSI_EH_CANCEL_CMD) ||
 		    SCSI_SENSE_VALID(scmd))
 			continue;
 
+		shost = scmd->device->host;
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_eh_past_deadline(shost)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				shost_printk(KERN_INFO, shost,
+					    "skip %s, past eh deadline\n",
+					     __func__));
+			break;
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		SCSI_LOG_ERROR_RECOVERY(2, scmd_printk(KERN_INFO, scmd,
 						  "%s: requesting sense\n",
 						  current->comm));
@@ -1019,11 +1050,28 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 	struct scsi_cmnd *scmd, *next;
 	struct scsi_device *sdev;
 	int finish_cmds;
+	unsigned long flags;
 
 	while (!list_empty(cmd_list)) {
 		scmd = list_entry(cmd_list->next, struct scsi_cmnd, eh_entry);
 		sdev = scmd->device;
 
+		if (!try_stu) {
+			spin_lock_irqsave(sdev->host->host_lock, flags);
+			if (scsi_host_eh_past_deadline(sdev->host)) {
+				/* Push items back onto work_q */
+				list_splice_init(cmd_list, work_q);
+				spin_unlock_irqrestore(sdev->host->host_lock,
+						       flags);
+				SCSI_LOG_ERROR_RECOVERY(3,
+					shost_printk(KERN_INFO, sdev->host,
+						     "skip %s, past eh deadline",
+						     __func__));
+				break;
+			}
+			spin_unlock_irqrestore(sdev->host->host_lock, flags);
+		}
+
 		finish_cmds = !scsi_device_online(scmd->device) ||
 			(try_stu && !scsi_eh_try_stu(scmd) &&
 			 !scsi_eh_tur(scmd)) ||
@@ -1059,14 +1107,28 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
 	struct scsi_cmnd *scmd, *next;
 	LIST_HEAD(check_list);
 	int rtn;
+	struct Scsi_Host *shost;
+	unsigned long flags;
 
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
 		if (!(scmd->eh_eflags & SCSI_EH_CANCEL_CMD))
 			continue;
+		shost = scmd->device->host;
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_eh_past_deadline(shost)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			list_splice_init(&check_list, work_q);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				shost_printk(KERN_INFO, shost,
+					    "skip %s, past eh deadline\n",
+					     __func__));
+			return list_empty(work_q);
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting cmd:"
 						  "0x%p\n", current->comm,
 						  scmd));
-		rtn = scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd);
+		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
 		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
 			scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
 			if (rtn == FAST_IO_FAIL)
@@ -1124,8 +1186,19 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
 {
 	struct scsi_cmnd *scmd, *stu_scmd, *next;
 	struct scsi_device *sdev;
+	unsigned long flags;
 
 	shost_for_each_device(sdev, shost) {
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_eh_past_deadline(shost)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				shost_printk(KERN_INFO, shost,
+					    "skip %s, past eh deadline\n",
+					     __func__));
+			break;
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		stu_scmd = NULL;
 		list_for_each_entry(scmd, work_q, eh_entry)
 			if (scmd->device == sdev && SCSI_SENSE_VALID(scmd) &&
@@ -1178,9 +1251,20 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
 {
 	struct scsi_cmnd *scmd, *bdr_scmd, *next;
 	struct scsi_device *sdev;
+	unsigned long flags;
 	int rtn;
 
 	shost_for_each_device(sdev, shost) {
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_eh_past_deadline(shost)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				shost_printk(KERN_INFO, shost,
+					    "skip %s, past eh deadline\n",
+					     __func__));
+			break;
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		bdr_scmd = NULL;
 		list_for_each_entry(scmd, work_q, eh_entry)
 			if (scmd->device == sdev) {
@@ -1240,6 +1324,21 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
 		struct scsi_cmnd *next, *scmd;
 		int rtn;
 		unsigned int id;
+		unsigned long flags;
+
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_eh_past_deadline(shost)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			/* push back on work queue for further processing */
+			list_splice_init(&check_list, work_q);
+			list_splice_init(&tmp_list, work_q);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				shost_printk(KERN_INFO, shost,
+					    "skip %s, past eh deadline\n",
+					     __func__));
+			return list_empty(work_q);
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
 
 		scmd = list_entry(tmp_list.next, struct scsi_cmnd, eh_entry);
 		id = scmd_id(scmd);
@@ -1284,6 +1383,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
 	LIST_HEAD(check_list);
 	unsigned int channel;
 	int rtn;
+	unsigned long flags;
 
 	/*
 	 * we really want to loop over the various channels, and do this on
@@ -1293,6 +1393,18 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
 	 */
 
 	for (channel = 0; channel <= shost->max_channel; channel++) {
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_eh_past_deadline(shost)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			list_splice_init(&check_list, work_q);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				shost_printk(KERN_INFO, shost,
+					    "skip %s, past eh deadline\n",
+					     __func__));
+			return list_empty(work_q);
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
+
 		chan_scmd = NULL;
 		list_for_each_entry(scmd, work_q, eh_entry) {
 			if (channel == scmd_channel(scmd)) {
@@ -1698,8 +1810,9 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
 	 * will be requests for character device operations, and also for
 	 * ioctls to queued block devices.
 	 */
-	SCSI_LOG_ERROR_RECOVERY(3, printk("%s: waking up host to restart\n",
-					  __func__));
+	SCSI_LOG_ERROR_RECOVERY(3,
+		printk("scsi_eh_%d waking up host to restart\n",
+		       shost->host_no));
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (scsi_host_set_state(shost, SHOST_RUNNING))
@@ -1826,6 +1939,10 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
 		if (!scsi_eh_abort_cmds(&eh_work_q, &eh_done_q))
 			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
 
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (shost->eh_deadline)
+		shost->last_reset = 0;
+	spin_unlock_irqrestore(shost->host_lock, flags);
 	scsi_eh_flush_done_q(&eh_done_q);
 }
 
@@ -1852,7 +1969,7 @@ int scsi_error_handler(void *data)
 		if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
 		    shost->host_failed != shost->host_busy) {
 			SCSI_LOG_ERROR_RECOVERY(1,
-				printk("Error handler scsi_eh_%d sleeping\n",
+				printk("scsi_eh_%d: sleeping\n",
 					shost->host_no));
 			schedule();
 			continue;
@@ -1860,8 +1977,9 @@ int scsi_error_handler(void *data)
 
 		__set_current_state(TASK_RUNNING);
 		SCSI_LOG_ERROR_RECOVERY(1,
-			printk("Error handler scsi_eh_%d waking up\n",
-				shost->host_no));
+			printk("scsi_eh_%d: waking up %d/%d/%d\n",
+			       shost->host_no, shost->host_eh_scheduled,
+			       shost->host_failed, shost->host_busy));
 
 		/*
 		 * We have a host that is failing for some reason.  Figure out
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 931a7d9..1c597ab 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -281,6 +281,42 @@ exit_store_host_reset:
 
 static DEVICE_ATTR(host_reset, S_IWUSR, NULL, store_host_reset);
 
+static ssize_t
+show_shost_eh_deadline(struct device *dev,
+		      struct device_attribute *attr, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+
+	return sprintf(buf, "%d\n", shost->eh_deadline / HZ);
+}
+
+static ssize_t
+store_shost_eh_deadline(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	int ret = -EINVAL;
+	int deadline;
+	unsigned long flags;
+
+	if (shost->transportt && shost->transportt->eh_strategy_handler)
+		return ret;
+
+	if (sscanf(buf, "%d\n", &deadline) == 1) {
+		spin_lock_irqsave(shost->host_lock, flags);
+		if (scsi_host_in_recovery(shost))
+			ret = -EBUSY;
+		else {
+			shost->eh_deadline = deadline * HZ;
+			ret = count;
+		}
+		spin_unlock_irqrestore(shost->host_lock, flags);
+	}
+	return ret;
+}
+
+static DEVICE_ATTR(eh_deadline, S_IRUGO | S_IWUSR, show_shost_eh_deadline, store_shost_eh_deadline);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -308,6 +344,7 @@ static struct attribute *scsi_sysfs_shost_attrs[] = {
 	&dev_attr_prot_capabilities.attr,
 	&dev_attr_prot_guard_type.attr,
 	&dev_attr_host_reset.attr,
+	&dev_attr_eh_deadline.attr,
 	NULL
 };
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 7552435..896bb05 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -596,9 +596,9 @@ struct Scsi_Host {
 	unsigned int host_busy;		   /* commands actually active on low-level */
 	unsigned int host_failed;	   /* commands that failed. */
 	unsigned int host_eh_scheduled;    /* EH scheduled without command */
-    
+
 	unsigned int host_no;  /* Used for IOCTL_GET_IDLUN, /proc/scsi et al. */
-	int resetting; /* if set, it means that last_reset is a valid value */
+	int eh_deadline;
 	unsigned long last_reset;
 
 	/*
-- 
1.7.12.4

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Jörn Engel]

On Mon, 1 July 2013 08:50:48 +0200, Hannes Reinecke wrote:
> 
> This patchset implements a new 'eh_deadline' attribute to the
> SCSI host. It will limit the overall SCSI EH runtime by a given
> timeout. If the timeout is reached all intermediate EH steps
> will be skipped and host reset will be scheduled immediately.

I have mixed opinions about the concept.

Having a command timeout is of limited use if you can still spend
several minutes after the timeout in random processing.  Userspace
either needs -EIO reasonably quickly after a command timeout or will
have to implement it's own timeout mechanism.  I prefer having a
single implementation in the kernel, so your patches are a step in the
right direction.

Host reset is an expensive and harmful operation.  You lose access to
all devices behind the host.  At best this is a performance blip, at
worst someone actually cared about some realtime properties.  My main
grump is that a single bad device can trigger this behaviour,
essentially doing a DoS on the rest of the system.  While that problem
is somewhat orthogonal, your patchset can only make matters worse.

Ideally we would have a way to detect the system geometry and next the
error location.  If a single device is bad, don't ever do a host
reset.  If you have redundant paths, never do a host reset on both
controllers at the same time.  Etc, etc.

Getting there will be a lot of work and the result may be too
error-prone to maintain without constantly breaking one exotic setup
or another.  But if someone could pull it off, it would be really nice
to have.

That said, now I should actually read your patches. ;)

Jörn

--
Measure. Don't tune for speed until you've measured, and even then
don't unless one part of the code overwhelms the rest.
-- Rob Pike
--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[James Bottomley]

On Mon, 2013-07-01 at 13:44 -0400, Jörn Engel wrote:
> If a single device is bad, don't ever do a host
> reset.

This isn't a tenable position.  Sometimes a device looks bad because the
host state for it has gone insane.  At that point, the only safe action
is a reset of the host to sane state.

I could be persuaded that you should never do the transport equivalent
of a bus reset (on non-SPI transports, at least), which is actually hard
to do on some of the modern transports, but I don't think you can get
away without having a host reset in the eh arsenal.

James

--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Jörn Engel]

On Mon, 1 July 2013 19:23:25 +0000, James Bottomley wrote:
> On Mon, 2013-07-01 at 13:44 -0400, Jörn Engel wrote:
> > If a single device is bad, don't ever do a host
> > reset.
> 
> This isn't a tenable position.  Sometimes a device looks bad because the
> host state for it has gone insane.  At that point, the only safe action
> is a reset of the host to sane state.
> 
> I could be persuaded that you should never do the transport equivalent
> of a bus reset (on non-SPI transports, at least), which is actually hard
> to do on some of the modern transports, but I don't think you can get
> away without having a host reset in the eh arsenal.

Fair enough.  Hardware being hardware and hardware bugs being hard to
fix, I see your point.

However, we shouldn't screw the poor user who has paid a premium for a
second HBA to get some redundancy and reset both of them at the same
time.  That would, you know, defeat the redundancy. ;)

Jörn

--
A victorious army first wins and then seeks battle.
-- Sun Tzu
--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Hannes Reinecke]

On 07/01/2013 10:55 PM, Jörn Engel wrote:
> On Mon, 1 July 2013 19:23:25 +0000, James Bottomley wrote:
>> On Mon, 2013-07-01 at 13:44 -0400, Jörn Engel wrote:
>>> If a single device is bad, don't ever do a host
>>> reset.
>>
>> This isn't a tenable position.  Sometimes a device looks bad because the
>> host state for it has gone insane.  At that point, the only safe action
>> is a reset of the host to sane state.
>>
>> I could be persuaded that you should never do the transport equivalent
>> of a bus reset (on non-SPI transports, at least), which is actually hard
>> to do on some of the modern transports, but I don't think you can get
>> away without having a host reset in the eh arsenal.
> 
> Fair enough.  Hardware being hardware and hardware bugs being hard to
> fix, I see your point.
> 
> However, we shouldn't screw the poor user who has paid a premium for a
> second HBA to get some redundancy and reset both of them at the same
> time.  That would, you know, defeat the redundancy. ;)
> 
Which would arguably a setup issue.

We've had SAN issues where the HBA lost track of the remote port
state (RSCNs being eaten by the switch firmware), so the only chance
of recovery was indeed a host reset.

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)
--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[James Bottomley]

On Mon, 2013-07-01 at 16:55 -0400, Jörn Engel wrote:
> On Mon, 1 July 2013 19:23:25 +0000, James Bottomley wrote:
> > On Mon, 2013-07-01 at 13:44 -0400, Jörn Engel wrote:
> > > If a single device is bad, don't ever do a host
> > > reset.
> > 
> > This isn't a tenable position.  Sometimes a device looks bad because the
> > host state for it has gone insane.  At that point, the only safe action
> > is a reset of the host to sane state.
> > 
> > I could be persuaded that you should never do the transport equivalent
> > of a bus reset (on non-SPI transports, at least), which is actually hard
> > to do on some of the modern transports, but I don't think you can get
> > away without having a host reset in the eh arsenal.
> 
> Fair enough.  Hardware being hardware and hardware bugs being hard to
> fix, I see your point.
> 
> However, we shouldn't screw the poor user who has paid a premium for a
> second HBA to get some redundancy and reset both of them at the same
> time.  That would, you know, defeat the redundancy. ;)

I don't understand what you're getting at.  In a dual HBA situation,
whether the second HBA is implicated or not depends on configuration and
what the first HBA is doing. If it's just passively lost device state,
then the second HBA should continue just fine.  If the insane HBA is
injecting rogue data on the bus then, in a properly isolated
configuration, it shouldn't be able to affect the second HBA, but if
there's some leak and it does, chances are error handling will occur on
both simultaneously.  I don't see any way to avoid this other than
having the user buy better hardware and properly configure it.

James

--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Jörn Engel]

On Tue, 2 July 2013 06:37:05 +0000, James Bottomley wrote:
> 
> I don't understand what you're getting at.  In a dual HBA situation,
> whether the second HBA is implicated or not depends on configuration and
> what the first HBA is doing. If it's just passively lost device state,
> then the second HBA should continue just fine.  If the insane HBA is

If the problem is an insane drive instead of an insane HBA, both HBAs
will be in roughly the same state at roughly the same time - assuming
they both send commands to the insane drive.  If they now go into
error handling and effectively shut off all the sane drives at roughly
the same time, the user is ****ed.

And we shouldn't require the user to buy better hardware.  The whole
point of a redundant setup is that your plane doesn't crash to the
ground when one of your two engines fails.  If regulations required
perfect engines, you wouldn't be flying to conferences.  They require
decent engines and enough redundancy that any one can fail at any
moment.

Computer systems are no different.  We can construct a robust system
from individually less robust components.  Requiring perfect
components would be ludicrous.  Having a system design where one
faulty component will reliably bring the system down is equally
ludicrous.  Sadly that is also the state of today's scsi stack.

This is not a theoretical problem, btw.  We currently carry some
patches to solve it for us.  They are not applicable for mainline in
their current state - we support a lot less hardware diversity.  But
trust me, we didn't create them on a whim. ;)

Jörn

--
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack
--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Jörn Engel]

On Tue, 2 July 2013 16:33:40 +0000, James Bottomley wrote:
> On Tue, 2013-07-02 at 10:58 -0400, Jörn Engel wrote:
> > On Tue, 2 July 2013 06:37:05 +0000, James Bottomley wrote:
> > > 
> > > I don't understand what you're getting at.  In a dual HBA situation,
> > > whether the second HBA is implicated or not depends on configuration and
> > > what the first HBA is doing. If it's just passively lost device state,
> > > then the second HBA should continue just fine.  If the insane HBA is
> > 
> > If the problem is an insane drive instead of an insane HBA, both HBAs
> > will be in roughly the same state at roughly the same time - assuming
> > they both send commands to the insane drive.  If they now go into
> > error handling and effectively shut off all the sane drives at roughly
> > the same time, the user is ****ed.
> 
> That's handled in device reset, so I don't understand your point.

Doesn't a device reset require all IO to the entire HBA to stop?
Hannes patches fixed that for aborts, but not for device reset yet,
afaics.

Jörn

--
In America you can have either a flimsy box banged together out of two
by fours and drywall, or a McMansion -- a flimsy box banged together
out of two by fours and drywall, but larger, more dramatic-looking,
and full of expensive fittings.
-- Paul Graham
--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[James Bottomley]

On Tue, 2013-07-02 at 10:58 -0400, Jörn Engel wrote:
> On Tue, 2 July 2013 06:37:05 +0000, James Bottomley wrote:
> > 
> > I don't understand what you're getting at.  In a dual HBA situation,
> > whether the second HBA is implicated or not depends on configuration and
> > what the first HBA is doing. If it's just passively lost device state,
> > then the second HBA should continue just fine.  If the insane HBA is
> 
> If the problem is an insane drive instead of an insane HBA, both HBAs
> will be in roughly the same state at roughly the same time - assuming
> they both send commands to the insane drive.  If they now go into
> error handling and effectively shut off all the sane drives at roughly
> the same time, the user is ****ed.

That's handled in device reset, so I don't understand your point.

James

> And we shouldn't require the user to buy better hardware.  The whole
> point of a redundant setup is that your plane doesn't crash to the
> ground when one of your two engines fails.  If regulations required
> perfect engines, you wouldn't be flying to conferences.  They require
> decent engines and enough redundancy that any one can fail at any
> moment.
> 
> Computer systems are no different.  We can construct a robust system
> from individually less robust components.  Requiring perfect
> components would be ludicrous.  Having a system design where one
> faulty component will reliably bring the system down is equally
> ludicrous.  Sadly that is also the state of today's scsi stack.
> 
> This is not a theoretical problem, btw.  We currently carry some
> patches to solve it for us.  They are not applicable for mainline in
> their current state - we support a lot less hardware diversity.  But
> trust me, we didn't create them on a whim. ;)


--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ewan Milne]

On Mon, 2013-07-01 at 08:50 +0200, Hannes Reinecke wrote:
> This patchset implements a new 'eh_deadline' attribute to the
> SCSI host. It will limit the overall SCSI EH runtime by a given
> timeout. If the timeout is reached all intermediate EH steps
> will be skipped and host reset will be scheduled immediately.
> 
> For this patch I've re-used the existing 'last_reset' field
> of the SCSI host to store the initial time SCSI EH started.
> Also the field 'resetting' has been removed as it never has
> been used as intended.
> 
> As 'last_reset' might be in use by transport-specific EH
> implementation I've disallowed eh_deadline setting there.
> 
> Changes from the initial version:
> - Add list_splice_init() calls to avoid stale commands
> - Rename function to scsi_host_eh_past_deadline
> 
> Hannes Reinecke (7):
>   dpt_i2o: Remove DPTI_STATE_IOCTL
>   dpt_i2o: return SCSI_MLQUEUE_HOST_BUSY when in reset
>   advansys: Remove 'last_reset' references
>   tmscsim: Move 'last_reset' into host structure
>   dc395: Move 'last_reset' into internal host structure
>   scsi: remove check for 'resetting'
>   scsi: Add 'eh_deadline' to limit SCSI EH runtime
> 
>  drivers/scsi/advansys.c   |   8 +--
>  drivers/scsi/dc395x.c     |  24 +++++----
>  drivers/scsi/dpt_i2o.c    |  35 +++++--------
>  drivers/scsi/dpti.h       |   1 -
>  drivers/scsi/hosts.c      |   7 +++
>  drivers/scsi/scsi.c       |  28 ----------
>  drivers/scsi/scsi_error.c | 130 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/scsi/scsi_sysfs.c |  37 +++++++++++++
>  drivers/scsi/tmscsim.c    |  14 ++---
>  drivers/scsi/tmscsim.h    |   1 +
>  include/scsi/scsi_host.h  |   4 +-
>  11 files changed, 208 insertions(+), 81 deletions(-)
> 

Looks good.  We have been testing this extensively.

Acked-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ren Mingxin]

Hi, Ewan:

On 07/11/2013 04:35 AM, Ewan Milne wrote:
> On Mon, 2013-07-01 at 08:50 +0200, Hannes Reinecke wrote:
>> This patchset implements a new 'eh_deadline' attribute to the
>> SCSI host. It will limit the overall SCSI EH runtime by a given
>> timeout. If the timeout is reached all intermediate EH steps
>> will be skipped and host reset will be scheduled immediately.
>>
>> For this patch I've re-used the existing 'last_reset' field
>> of the SCSI host to store the initial time SCSI EH started.
>> Also the field 'resetting' has been removed as it never has
>> been used as intended.
>>
>> As 'last_reset' might be in use by transport-specific EH
>> implementation I've disallowed eh_deadline setting there.
>>
>> Changes from the initial version:
>> - Add list_splice_init() calls to avoid stale commands
>> - Rename function to scsi_host_eh_past_deadline
>>
>> Hannes Reinecke (7):
>>    dpt_i2o: Remove DPTI_STATE_IOCTL
>>    dpt_i2o: return SCSI_MLQUEUE_HOST_BUSY when in reset
>>    advansys: Remove 'last_reset' references
>>    tmscsim: Move 'last_reset' into host structure
>>    dc395: Move 'last_reset' into internal host structure
>>    scsi: remove check for 'resetting'
>>    scsi: Add 'eh_deadline' to limit SCSI EH runtime
>>
>>   drivers/scsi/advansys.c   |   8 +--
>>   drivers/scsi/dc395x.c     |  24 +++++----
>>   drivers/scsi/dpt_i2o.c    |  35 +++++--------
>>   drivers/scsi/dpti.h       |   1 -
>>   drivers/scsi/hosts.c      |   7 +++
>>   drivers/scsi/scsi.c       |  28 ----------
>>   drivers/scsi/scsi_error.c | 130 +++++++++++++++++++++++++++++++++++++++++++---
>>   drivers/scsi/scsi_sysfs.c |  37 +++++++++++++
>>   drivers/scsi/tmscsim.c    |  14 ++---
>>   drivers/scsi/tmscsim.h    |   1 +
>>   include/scsi/scsi_host.h  |   4 +-
>>   11 files changed, 208 insertions(+), 81 deletions(-)
>>
> Looks good.  We have been testing this extensively.

I'm wondering how do you test, with a special hardware or self-made
module?Would you mind pasting your test method() and result?

Thanks,
Ren

>
> Acked-by: Ewan D. Milne<emilne@redhat.com>

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ewan Milne]

On Fri, 2013-07-12 at 13:54 +0800, Ren Mingxin wrote:
> Hi, Ewan:
> 
> I'm wondering how do you test, with a special hardware or self-made
> module?Would you mind pasting your test method() and result?

Hi Rex-

This was tested in a SAN environment with an EMC Symmetrix and
Brocade FC switches.  The error was injected by the following
commands:

portcfg rscnsupr <port> --enable
portdisable <port>

Where <port> is the FC port of the Symmetrix target.

Multipath is used and the test records how long I/O from userspace
takes to complete after the error handling stops and the I/O is
retried on another path.

What happens is that the target never responds to anything the
HBA sends, so commands and TMFs just timeout.  The HBA doesn't
see link down (since it is the target port) and doesn't get an
RSCN.  When the HBA is finally reset, however, it can't login
to the target port and so further I/O gets an immediate error.

Unfortunately, not all SAN environments will exhibit the failing
behavior -- it appears as if in some cases the HBA detects the
problem regardless of the switch portcfg setting.  But this has
been verified to solve the problem of seemingly endless EH
activity in testing at a large customer site.

Also, to be clear, we tested with the "Limit overall SCSI EH
runtime" patchset but not the "New EH command timeout handler".
I think the changes to issue the abort in the timeout handler
are a good idea, though, because there really is no need to
wait for all activity on the host to cease before issuing the
abort as far as I can see.

-Ewan

> 
> Thanks,
> Ren
> 
> >
> > Acked-by: Ewan D. Milne<emilne@redhat.com>
> 

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ren Mingxin]

Hi, Ewan:

On 07/12/2013 09:30 PM, Ewan Milne wrote:
> On Fri, 2013-07-12 at 13:54 +0800, Ren Mingxin wrote:
>> I'm wondering how do you test, with a special hardware or self-made
>> module?Would you mind pasting your test method() and result?
> This was tested in a SAN environment with an EMC Symmetrix and
> Brocade FC switches.  The error was injected by the following
> commands:
>
> portcfg rscnsupr<port>  --enable
> portdisable<port>
>
> Where<port>  is the FC port of the Symmetrix target.
>
> Multipath is used and the test records how long I/O from userspace
> takes to complete after the error handling stops and the I/O is
> retried on another path.
>
> What happens is that the target never responds to anything the
> HBA sends, so commands and TMFs just timeout.  The HBA doesn't
> see link down (since it is the target port) and doesn't get an
> RSCN.  When the HBA is finally reset, however, it can't login
> to the target port and so further I/O gets an immediate error.
>
> Unfortunately, not all SAN environments will exhibit the failing
> behavior -- it appears as if in some cases the HBA detects the
> problem regardless of the switch portcfg setting.  But this has
> been verified to solve the problem of seemingly endless EH
> activity in testing at a large customer site.

Thanks in advance for your explanations in detail. I've been able to
reproduce only with this patchset.

> Also, to be clear, we tested with the "Limit overall SCSI EH
> runtime" patchset but not the "New EH command timeout handler".
> I think the changes to issue the abort in the timeout handler
> are a good idea, though, because there really is no need to
> wait for all activity on the host to cease before issuing the
> abort as far as I can see.

Hmm, agree with you. It is much better to issue aborts without
waiting, which can shorten the timeout handling time.

>>> Acked-by: Ewan D. Milne<emilne@redhat.com>
>>>

Hi, Hannes:

I noticed that the dd time had been reduced from 6m+ to 2m+ when the
'eh_deadline' was set as 30s, but the dd time was 6m+(nearly the same
as default - 'eh_deadline' was 0) when the 'eh_deadline' was set as
10s. I havn't been able to dig further, but I guess there is some
restriction when setting this 'eh_deadline' interface. Maybe should
not less than some timeout, otherwise 'eh_deadline' setting will not
work?

Thanks,
Ren

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ren Mingxin]

Hi, Hannes:

On 07/15/2013 06:33 PM, Ren Mingxin wrote:
> I noticed that the dd time had been reduced from 6m+ to 2m+ when the
> 'eh_deadline' was set as 30s, but the dd time was 6m+(nearly the same
> as default - 'eh_deadline' was 0) when the 'eh_deadline' was set as
> 10s. I havn't been able to dig further, but I guess there is some
> restriction when setting this 'eh_deadline' interface. Maybe should
> not less than some timeout, otherwise 'eh_deadline' setting will not
> work?

I've retried and confirmed that the exception above is caused by
misoperation - for I had two fc hosts to build a failover multipath,
but I just set 'eh_deadline' on one host. When I tested with 10s,
the 'eh_deadline' on the host of the active path wasn't set :-(

Sorry for my mistake. So:

Tested-by: Ren Mingxin <renmx@cn.fujitsu.com>

Thanks,
Ren

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ren Mingxin]

Hi, James:

On 07/11/2013 04:35 AM, Ewan Milne wrote:
> Looks good.  We have been testing this extensively.
>
> Acked-by: Ewan D. Milne<emilne@redhat.com>

Do you think this patchset can be applied? If so, When? Perhaps you
are waiting for someone's feedback?

We've also tested and got the duration could be shortened from 6m26s
to 44s when 'eh_deadline' was set as 1s(the minimum value of timeout)
and 16M data were written(I/O processing time can be ignored - 0.7s).

As Ewan said, this is efficient to fast failover policy for redundant
environments.

Thanks,
Ren

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Hannes Reinecke]

Hi James,

On 08/07/2013 08:43 AM, Ren Mingxin wrote:
> Hi, James:
> 
> On 07/11/2013 04:35 AM, Ewan Milne wrote:
>> Looks good.  We have been testing this extensively.
>>
>> Acked-by: Ewan D. Milne<emilne@redhat.com>
> 
> Do you think this patchset can be applied? If so, When? Perhaps you
> are waiting for someone's feedback?
> 
> We've also tested and got the duration could be shortened from 6m26s
> to 44s when 'eh_deadline' was set as 1s(the minimum value of timeout)
> and 16M data were written(I/O processing time can be ignored - 0.7s).
> 
> As Ewan said, this is efficient to fast failover policy for redundant
> environments.
> 
> Thanks,
> Ren

Any objections to this patchset?
I have other patchsets pending (eg asynchronous command aborts)
which are based on this one.
So it would be good to have a status here.

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)
--
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 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[Ren Mingxin]

Hi, Hannes:

On 07/01/2013 02:50 PM, Hannes Reinecke wrote:
> This patchs adds an 'eh_deadline' sysfs attribute to the scsi
> host which limits the overall runtime of the SCSI EH.
> The 'eh_deadline' value is stored in the now obsolete field
> 'resetting'.
> When a command is failed the start time of the EH is stored
> in 'last_reset'. If the overall runtime of the SCSI EH is longer
> than last_reset + eh_deadline, the EH is short-circuited and
> falls through to issue a host reset only.

There is one thing during my test: if I want to stop EH ASAP, I can
only set the 'eh_deadline' as the minimum value: 1 second. But on my
box, since scsi command times out, it takes less than 1 second before
the first check point - comparingthe overall runtime of the SCSI EH
with last_reset + eh_deadline as you said. So, the EH could only be
stopped once it spends more than 1 second before the check point
rather than stopping at the first time.

This problem is also existed in your second patchset "New EH command
timeout handler" - it spends less than 1 second before the check
point in scsi_abort_command().

So, should a special handling be considered for 1 second? E.g., we
just past eh deadline when 1 second is set even if 1 second hasn't
been reached. Or, should 0 second mean stopping EH ASAP rather than
disabling eh_deadline?

> Signed-off-by: Hannes Reinecke<hare@suse.de>
<snip>
> @@ -1059,14 +1107,28 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
>   	struct scsi_cmnd *scmd, *next;
>   	LIST_HEAD(check_list);
>   	int rtn;
> +	struct Scsi_Host *shost;
> +	unsigned long flags;
>
>   	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
>   		if (!(scmd->eh_eflags&  SCSI_EH_CANCEL_CMD))
>   			continue;
> +		shost = scmd->device->host;
> +		spin_lock_irqsave(shost->host_lock, flags);
> +		if (scsi_host_eh_past_deadline(shost)) {

Especially speaking: could we remove this check point? In other
words, could we keep aborting? According to my test,
scsi_try_to_abort_cmd() takes so little time that we can ignore it.
So, keeping aborting won't reduce the performance of stopping EH,
and it is worth trying.

Also, I'd like removing the check point in your new added
scmd_eh_abort_handler() in your second patchset.

Thanks,
Ren

>
> +			spin_unlock_irqrestore(shost->host_lock, flags);
> +			list_splice_init(&check_list, work_q);
> +			SCSI_LOG_ERROR_RECOVERY(3,
> +				shost_printk(KERN_INFO, shost,
> +					    "skip %s, past eh deadline\n",
> +					     __func__));
> +			return list_empty(work_q);
> +		}
> +		spin_unlock_irqrestore(shost->host_lock, flags);
>   		SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting cmd:"
>   						  "0x%p\n", current->comm,
>   						  scmd));
> -		rtn = scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd);
> +		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
>   		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
>   			scmd->eh_eflags&= ~SCSI_EH_CANCEL_CMD;
>   			if (rtn == FAST_IO_FAIL)

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Ric Wheeler]

On 08/29/2013 09:06 AM, Hannes Reinecke wrote:
> Hi James,
>
> On 08/07/2013 08:43 AM, Ren Mingxin wrote:
>> Hi, James:
>>
>> On 07/11/2013 04:35 AM, Ewan Milne wrote:
>>> Looks good.  We have been testing this extensively.
>>>
>>> Acked-by: Ewan D. Milne<emilne@redhat.com>
>> Do you think this patchset can be applied? If so, When? Perhaps you
>> are waiting for someone's feedback?
>>
>> We've also tested and got the duration could be shortened from 6m26s
>> to 44s when 'eh_deadline' was set as 1s(the minimum value of timeout)
>> and 16M data were written(I/O processing time can be ignored - 0.7s).
>>
>> As Ewan said, this is efficient to fast failover policy for redundant
>> environments.
>>
>> Thanks,
>> Ren
> Any objections to this patchset?
> I have other patchsets pending (eg asynchronous command aborts)
> which are based on this one.
> So it would be good to have a status here.
>
> Cheers,
>
> Hannes

Any progress on this patchset? Seems to have gone dormant?

thanks!

Ric

Re: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Hannes Reinecke]

On 09/24/2013 10:51 PM, Ric Wheeler wrote:
> On 08/29/2013 09:06 AM, Hannes Reinecke wrote:
>> Hi James,
>>
>> On 08/07/2013 08:43 AM, Ren Mingxin wrote:
>>> Hi, James:
>>>
>>> On 07/11/2013 04:35 AM, Ewan Milne wrote:
>>>> Looks good.  We have been testing this extensively.
>>>>
>>>> Acked-by: Ewan D. Milne<emilne@redhat.com>
>>> Do you think this patchset can be applied? If so, When? Perhaps you
>>> are waiting for someone's feedback?
>>>
>>> We've also tested and got the duration could be shortened from 6m26s
>>> to 44s when 'eh_deadline' was set as 1s(the minimum value of
>>> timeout)
>>> and 16M data were written(I/O processing time can be ignored -
>>> 0.7s).
>>>
>>> As Ewan said, this is efficient to fast failover policy for
>>> redundant
>>> environments.
>>>
>>> Thanks,
>>> Ren
>> Any objections to this patchset?
>> I have other patchsets pending (eg asynchronous command aborts)
>> which are based on this one.
>> So it would be good to have a status here.
>>
>> Cheers,
>>
>> Hannes
> 
> Any progress on this patchset? Seems to have gone dormant?
> 
Not for the lack of trying on my side ...

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)
--
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: [PATCHv2 0/7] Limit overall SCSI EH runtime

[Hannes Reinecke]

On 09/25/2013 07:48 AM, Hannes Reinecke wrote:
> On 09/24/2013 10:51 PM, Ric Wheeler wrote:
>> On 08/29/2013 09:06 AM, Hannes Reinecke wrote:
>>> Hi James,
>>>
>>> On 08/07/2013 08:43 AM, Ren Mingxin wrote:
>>>> Hi, James:
>>>>
>>>> On 07/11/2013 04:35 AM, Ewan Milne wrote:
>>>>> Looks good.  We have been testing this extensively.
>>>>>
>>>>> Acked-by: Ewan D. Milne<emilne@redhat.com>
>>>> Do you think this patchset can be applied? If so, When? Perhaps you
>>>> are waiting for someone's feedback?
>>>>
>>>> We've also tested and got the duration could be shortened from 6m26s
>>>> to 44s when 'eh_deadline' was set as 1s(the minimum value of
>>>> timeout)
>>>> and 16M data were written(I/O processing time can be ignored -
>>>> 0.7s).
>>>>
>>>> As Ewan said, this is efficient to fast failover policy for
>>>> redundant
>>>> environments.
>>>>
>>>> Thanks,
>>>> Ren
>>> Any objections to this patchset?
>>> I have other patchsets pending (eg asynchronous command aborts)
>>> which are based on this one.
>>> So it would be good to have a status here.
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>> Any progress on this patchset? Seems to have gone dormant?
>>
> Not for the lack of trying on my side ...
>
So, James, could please give a short statement about the patchset?
Anything missing from my side?

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)
--
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 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[James Bottomley]

On Mon, 2013-07-01 at 08:50 +0200, Hannes Reinecke wrote:
> This patchs adds an 'eh_deadline' sysfs attribute to the scsi
> host which limits the overall runtime of the SCSI EH.
> The 'eh_deadline' value is stored in the now obsolete field
> 'resetting'.
> When a command is failed the start time of the EH is stored
> in 'last_reset'. If the overall runtime of the SCSI EH is longer
> than last_reset + eh_deadline, the EH is short-circuited and
> falls through to issue a host reset only.

OK, so the specific problem with this one is that potentially it will
spend all its time mucking about with aborts (which most often time out
on non FC kit because of the issue problems) and then proceed to host
reset, which mostly does nothing for failing devices.

If you want to impose a deadline, then we need to spend only 50% of the
time attempting aborts and the rest of the time escalating the resets.

[...]
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index f43de1e..84369f2 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -89,6 +89,18 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
>  }
>  EXPORT_SYMBOL_GPL(scsi_schedule_eh);
>  
> +static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
> +{
> +	if (!shost->last_reset || !shost->eh_deadline)
> +		return 0;
> +
> +	if (time_before(jiffies,
> +			shost->last_reset + shost->eh_deadline))
> +		return 0;
> +
> +	return 1;
> +}
> +

What about instead:

static int scsi_host_eh_past_deadline(struct Scsi_Host *shost, int percent) {
	if (!shost->last_reset || !shost->eh_deadline)
		return 0;

	if (time_before(jiffies,
			shost->last_reset + shost->eh_deadline * percent/100))
		return 0;

	return 1;
}

which allows us to have

if (scsi_host_eh_past_deadline(shost, 50)) {

in scsi_eh_abort_cmds()

if (scsi_host_eh_past_deadline(shost, 66) {

in scsi_eh_bus_device_reset()

say 83 in target reset, and 100 in bus reset.

Thus ensuring we at least get a crack at the reset chain?

James

Re: [PATCH 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[Ewan Milne]

On Wed, 2013-10-16 at 19:22 +0000, James Bottomley wrote:
> What about instead:
> 
> static int scsi_host_eh_past_deadline(struct Scsi_Host *shost, int percent) {
> 	if (!shost->last_reset || !shost->eh_deadline)
> 		return 0;
> 
> 	if (time_before(jiffies,
> 			shost->last_reset + shost->eh_deadline * percent/100))
> 		return 0;
> 
> 	return 1;
> }
> 
> which allows us to have
> 
> if (scsi_host_eh_past_deadline(shost, 50)) {
> 
> in scsi_eh_abort_cmds()
> 
> if (scsi_host_eh_past_deadline(shost, 66) {
> 
> in scsi_eh_bus_device_reset()
> 
> say 83 in target reset, and 100 in bus reset.
> 
> Thus ensuring we at least get a crack at the reset chain?
> 
> James
> 

Well, conceptually that seems like a good idea, since if there
is limited time available it is probably wiser to spend it on
higher-level recovery instead of timing out trying to deal with
individual devices, but we didn't do any testing on how long
the bus device reset/target reset/bus reset take.  The host
reset was about 10 seconds for lpfc, and the maximum time was
(command timeout) + (eh deadline) + (host reset time).

However...

With this enhancement, the maximum time could be much longer if
we attempt to e.g. perform a bus reset right before the
eh_deadline expires, because drivers like lpfc iterate over the
targets and send a target reset to each one (with a timeout).

The original problem that prompted this change was that a
target became inaccessible, and nothing the EH did was ever going
to do anything except timeout, until the host reset was performed,
at which point the FC login would fail and the HBA would start
failing commands immediately instead of them timing out.

I guess the main thing is that there should be some way to
explain to people what value to use for eh_deadline in order
for I/O to complete within a specified amount of time (e.g. before
some other node in a cluster shoots us because we are unresponsive).

-Ewan

Re: [PATCH 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[James Bottomley]

On Wed, 2013-10-23 at 11:25 +0200, Hannes Reinecke wrote:
> On 10/16/2013 09:22 PM, James Bottomley wrote:
> > On Mon, 2013-07-01 at 08:50 +0200, Hannes Reinecke wrote:
> >> This patchs adds an 'eh_deadline' sysfs attribute to the scsi
> >> host which limits the overall runtime of the SCSI EH.
> >> The 'eh_deadline' value is stored in the now obsolete field
> >> 'resetting'.
> >> When a command is failed the start time of the EH is stored
> >> in 'last_reset'. If the overall runtime of the SCSI EH is longer
> >> than last_reset + eh_deadline, the EH is short-circuited and
> >> falls through to issue a host reset only.
> >
> > OK, so the specific problem with this one is that potentially it will
> > spend all its time mucking about with aborts (which most often time out
> > on non FC kit because of the issue problems) and then proceed to host
> > reset, which mostly does nothing for failing devices.
> >
> > If you want to impose a deadline, then we need to spend only 50% of the
> > time attempting aborts and the rest of the time escalating the resets.
> >
> > [...]
> >> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> >> index f43de1e..84369f2 100644
> >> --- a/drivers/scsi/scsi_error.c
> >> +++ b/drivers/scsi/scsi_error.c
> >> @@ -89,6 +89,18 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
> >>   }
> >>   EXPORT_SYMBOL_GPL(scsi_schedule_eh);
> >>
> >> +static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
> >> +{
> >> +	if (!shost->last_reset || !shost->eh_deadline)
> >> +		return 0;
> >> +
> >> +	if (time_before(jiffies,
> >> +			shost->last_reset + shost->eh_deadline))
> >> +		return 0;
> >> +
> >> +	return 1;
> >> +}
> >> +
> >
> > What about instead:
> >
> > static int scsi_host_eh_past_deadline(struct Scsi_Host *shost, int percent) {
> > 	if (!shost->last_reset || !shost->eh_deadline)
> > 		return 0;
> >
> > 	if (time_before(jiffies,
> > 			shost->last_reset + shost->eh_deadline * percent/100))
> > 		return 0;
> >
> > 	return 1;
> > }
> >
> > which allows us to have
> >
> > if (scsi_host_eh_past_deadline(shost, 50)) {
> >
> > in scsi_eh_abort_cmds()
> >
> > if (scsi_host_eh_past_deadline(shost, 66) {
> >
> > in scsi_eh_bus_device_reset()
> >
> > say 83 in target reset, and 100 in bus reset.
> >
> > Thus ensuring we at least get a crack at the reset chain?
> >
> Alternatively we could just escalate directly to LUN reset once the 
> first abort fails. That looks like a cleaner solution here.

I'm OK with that ... if you want this in the current merge window, you
have about a week to code it up ...

James

Re: [PATCH 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[Hannes Reinecke]

On 10/16/2013 09:22 PM, James Bottomley wrote:
> On Mon, 2013-07-01 at 08:50 +0200, Hannes Reinecke wrote:
>> This patchs adds an 'eh_deadline' sysfs attribute to the scsi
>> host which limits the overall runtime of the SCSI EH.
>> The 'eh_deadline' value is stored in the now obsolete field
>> 'resetting'.
>> When a command is failed the start time of the EH is stored
>> in 'last_reset'. If the overall runtime of the SCSI EH is longer
>> than last_reset + eh_deadline, the EH is short-circuited and
>> falls through to issue a host reset only.
>
> OK, so the specific problem with this one is that potentially it will
> spend all its time mucking about with aborts (which most often time out
> on non FC kit because of the issue problems) and then proceed to host
> reset, which mostly does nothing for failing devices.
>
> If you want to impose a deadline, then we need to spend only 50% of the
> time attempting aborts and the rest of the time escalating the resets.
>
> [...]
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index f43de1e..84369f2 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -89,6 +89,18 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
>>   }
>>   EXPORT_SYMBOL_GPL(scsi_schedule_eh);
>>
>> +static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
>> +{
>> +	if (!shost->last_reset || !shost->eh_deadline)
>> +		return 0;
>> +
>> +	if (time_before(jiffies,
>> +			shost->last_reset + shost->eh_deadline))
>> +		return 0;
>> +
>> +	return 1;
>> +}
>> +
>
> What about instead:
>
> static int scsi_host_eh_past_deadline(struct Scsi_Host *shost, int percent) {
> 	if (!shost->last_reset || !shost->eh_deadline)
> 		return 0;
>
> 	if (time_before(jiffies,
> 			shost->last_reset + shost->eh_deadline * percent/100))
> 		return 0;
>
> 	return 1;
> }
>
> which allows us to have
>
> if (scsi_host_eh_past_deadline(shost, 50)) {
>
> in scsi_eh_abort_cmds()
>
> if (scsi_host_eh_past_deadline(shost, 66) {
>
> in scsi_eh_bus_device_reset()
>
> say 83 in target reset, and 100 in bus reset.
>
> Thus ensuring we at least get a crack at the reset chain?
>
Alternatively we could just escalate directly to LUN reset once the 
first abort fails. That looks like a cleaner solution here.

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)
--
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 7/7] scsi: Add 'eh_deadline' to limit SCSI EH runtime

[Hannes Reinecke]

On 10/23/2013 09:46 AM, James Bottomley wrote:
> On Wed, 2013-10-23 at 11:25 +0200, Hannes Reinecke wrote:
>> On 10/16/2013 09:22 PM, James Bottomley wrote:
>>> On Mon, 2013-07-01 at 08:50 +0200, Hannes Reinecke wrote:
>>>> This patchs adds an 'eh_deadline' sysfs attribute to the scsi
>>>> host which limits the overall runtime of the SCSI EH.
>>>> The 'eh_deadline' value is stored in the now obsolete field
>>>> 'resetting'.
>>>> When a command is failed the start time of the EH is stored
>>>> in 'last_reset'. If the overall runtime of the SCSI EH is longer
>>>> than last_reset + eh_deadline, the EH is short-circuited and
>>>> falls through to issue a host reset only.
>>>
>>> OK, so the specific problem with this one is that potentially it will
>>> spend all its time mucking about with aborts (which most often time out
>>> on non FC kit because of the issue problems) and then proceed to host
>>> reset, which mostly does nothing for failing devices.
>>>
>>> If you want to impose a deadline, then we need to spend only 50% of the
>>> time attempting aborts and the rest of the time escalating the resets.
>>>
>>> [...]
>>>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>>>> index f43de1e..84369f2 100644
>>>> --- a/drivers/scsi/scsi_error.c
>>>> +++ b/drivers/scsi/scsi_error.c
>>>> @@ -89,6 +89,18 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(scsi_schedule_eh);
>>>>
>>>> +static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
>>>> +{
>>>> +	if (!shost->last_reset || !shost->eh_deadline)
>>>> +		return 0;
>>>> +
>>>> +	if (time_before(jiffies,
>>>> +			shost->last_reset + shost->eh_deadline))
>>>> +		return 0;
>>>> +
>>>> +	return 1;
>>>> +}
>>>> +
>>>
>>> What about instead:
>>>
>>> static int scsi_host_eh_past_deadline(struct Scsi_Host *shost, int percent) {
>>> 	if (!shost->last_reset || !shost->eh_deadline)
>>> 		return 0;
>>>
>>> 	if (time_before(jiffies,
>>> 			shost->last_reset + shost->eh_deadline * percent/100))
>>> 		return 0;
>>>
>>> 	return 1;
>>> }
>>>
>>> which allows us to have
>>>
>>> if (scsi_host_eh_past_deadline(shost, 50)) {
>>>
>>> in scsi_eh_abort_cmds()
>>>
>>> if (scsi_host_eh_past_deadline(shost, 66) {
>>>
>>> in scsi_eh_bus_device_reset()
>>>
>>> say 83 in target reset, and 100 in bus reset.
>>>
>>> Thus ensuring we at least get a crack at the reset chain?
>>>
>> Alternatively we could just escalate directly to LUN reset once the
>> first abort fails. That looks like a cleaner solution here.
>
> I'm OK with that ... if you want this in the current merge window, you
> have about a week to code it up ...
>
Patch is already compiling :-)

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