New SCSI command timeout handler

The 'eh_entry' list might be used even before scsi_softirq_done() is called. Hence we should rather initialize it together with the other eh-related variables.

When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. If the TMF fails the command will be pushed back onto the list of failed commands and the SCSI EH handler will be called immediately for all timed-out commands.

Quoted 10 lines · Hannes Reinecke

When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.

Quoted 7 lines

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. If the TMF fails the command will be pushed back onto the list of failed commands and the SCSI EH handler will be called immediately for all timed-out commands.

Why can't we use a work item per command? Linking things into a list just to queue it up to workqueues missed half of the point of the workqueue infrastructure.

Quoted 5 lines · Christoph Hellwig

Why can't we use a work item per command? Linking things into a list just to queue it up to workqueues missed half of the point of the workqueue infrastructure.

Hmm. I felt that using a per command workqueue might be a bit excessive. Also the current semantics call for a synchronous command abort. So even using a per command workqueue won't buy us anything as the workqueue item would have to wait for the command abort to complete, which again is quite a waste.

I don't care too much whether we use per-command work items or a single system-global thread.

Quoted 3 lines · Jörn Engel

I don't care too much whether we use per-command work items or a single system-global thread.

Actually, I do care. We have to abort the commands in parallel, as a fairly large number of abort can queue up and individual aborts can take 20s on hardware I care about.

20s for an abort is pretty bad, but given today's reality there is no need to make things worse by serializing.

Quoted 9 lines · Jörn Engel

Quoted 3 lines · Jörn Engel

I don't care too much whether we use per-command work items or a single system-global thread.

Actually, I do care. We have to abort the commands in parallel, as a fairly large number of abort can queue up and individual aborts can take 20s on hardware I care about.

Quoted 3 lines

20s for an abort is pretty bad, but given today's reality there is no need to make things worse by serializing.

We're only serializing aborts per LUN, so this is a _big_ improvement as the original, where we would be serializing per _host_.

Also, upon the first abort failure EH will be escalating to LUN reset, so we won't have to wait for all aborts to time out.

More importantly, the current synchronous implementation of command aborts does not allow for complete de-serialisation:
- There is no way to abort a running command abort, so we have to wait for it to complete, with the chance of running into a timeout.
- We will have to sent command aborts in parallel, and can only stop sending aborts once the first returns an error.
- After we've received an error we have to wait for the outstanding aborts to complete.
-> So the max wait-time will be 2 times the abort timeout.
Not much of a gain here :-)

The _correct_ way of handling asynchronous aborts would be to mandate that the LLDD has to send a command completion on the original command once an abort has been issued. Then we could just kick off the TMF and rearm the request timer. Everything else would then be handled via normal I/O paths.

However, this would mean to implement new callouts into each and every driver. And the actual gain would be dubious, as the several IHVs indicated that a command abort might be handled lazily, ie the target will return a good status, but abort the command only at a later time. Other vendors treat a command abort as a best bet, and rely on the LUN reset to clear up things.

So overall I doubt we'd be gaining much from a fully asynchronous command abort. I'd rather concentrate on getting the remaining bits like LUN reset working correctly.

Quoted 10 lines · Hannes Reinecke

When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.

Quoted 7 lines

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. If the TMF fails the command will be pushed back onto the list of failed commands and the SCSI EH handler will be called immediately for all timed-out commands.

Why can't we use a work item per command? Linking things into a list just to queue it up to workqueues missed half of the point of the workqueue infrastructure.

Quoted 19 lines · Christoph Hellwig

Quoted 10 lines · Hannes Reinecke

When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.

Quoted 7 lines

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. If the TMF fails the command will be pushed back onto the list of failed commands and the SCSI EH handler will be called immediately for all timed-out commands.

Quoted 3 lines

Why can't we use a work item per command? Linking things into a list just to queue it up to workqueues missed half of the point of the workqueue infrastructure.

Actually, I think we can dump the workqueue altogether. The only reason we need it is because the current abort handlers wait for the command and return the completion state. However, all LLDs are capable of emitting TMFs at interrupt level, so if we separated the emit from the wait, we could simply do this sequence:

on timeout, fire the abort from interrupt and mark the command as having an abort issued (possibly by adding a pointer to the abort task), return BLK_EH_RESET_TIMER.

Now if the timeout fires again, assume the abort was unsucessful and escalate to LUN reset.

This is fully asynchronous, fully tracked and doesn't rely on work queues.

The necessary additions for something like this are the from interrupt issue abort and LUN reset, which could just be additional callbacks in the host template.

James

Quoted 30 lines · James Bottomley

Quoted 19 lines · Christoph Hellwig

Quoted 10 lines · Hannes Reinecke

When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.

Quoted 7 lines

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. If the TMF fails the command will be pushed back onto the list of failed commands and the SCSI EH handler will be called immediately for all timed-out commands.

Quoted 3 lines

Why can't we use a work item per command? Linking things into a list just to queue it up to workqueues missed half of the point of the workqueue infrastructure.

Actually, I think we can dump the workqueue altogether. The only reason we need it is because the current abort handlers wait for the command and return the completion state. However, all LLDs are capable of emitting TMFs at interrupt level, so if we separated the emit from the wait, we could simply do this sequence:

Quoted 3 lines

on timeout, fire the abort from interrupt and mark the command as having an abort issued (possibly by adding a pointer to the abort task), return BLK_EH_RESET_TIMER.

Doesn't this cause blk_rq_timed_out to reset the timer on the req to the original timeout value again? It seems like this would increase the time before any further attempted error handling. The default timeout is 30 seconds for sd, but it could be much longer (e.g. WRITE SAME, which was 120 seconds last I looked).

Quoted 19 lines · Christoph Hellwig

Quoted 10 lines · Hannes Reinecke

When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.

Quoted 7 lines

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. If the TMF fails the command will be pushed back onto the list of failed commands and the SCSI EH handler will be called immediately for all timed-out commands.

Quoted 3 lines

Why can't we use a work item per command? Linking things into a list just to queue it up to workqueues missed half of the point of the workqueue infrastructure.

Actually, I think we can dump the workqueue altogether. The only reason we need it is because the current abort handlers wait for the command and return the completion state. However, all LLDs are capable of emitting TMFs at interrupt level, so if we separated the emit from the wait, we could simply do this sequence:

on timeout, fire the abort from interrupt and mark the command as having an abort issued (possibly by adding a pointer to the abort task), return BLK_EH_RESET_TIMER.

Now if the timeout fires again, assume the abort was unsucessful and escalate to LUN reset.

This is fully asynchronous, fully tracked and doesn't rely on work queues.

The necessary additions for something like this are the from interrupt issue abort and LUN reset, which could just be additional callbacks in the host template.

Actually, I think we can dump the workqueue altogether. The only reason we need it is because the current abort handlers wait for the command and return the completion state. However, all LLDs are capable of emitting TMFs at interrupt level, so if we separated the emit from the wait, we could simply do this sequence:

on timeout, fire the abort from interrupt and mark the command as having an abort issued (possibly by adding a pointer to the abort task), return BLK_EH_RESET_TIMER.

Now if the timeout fires again, assume the abort was unsuccessful and escalate to LUN reset.

This is fully asynchronous, fully tracked and doesn't rely on work queues.

The necessary additions for something like this are the from interrupt issue abort and LUN reset, which could just be additional callbacks in the host template.

Quoted 17 lines · James Bottomley

Actually, I think we can dump the workqueue altogether. The only reason
we need it is because the current abort handlers wait for the command and return the completion state. However, all LLDs are capable of emitting TMFs at interrupt level, so if we separated the emit from the
wait, we could simply do this sequence:

on timeout, fire the abort from interrupt and mark the command as having
an abort issued (possibly by adding a pointer to the abort task), return
BLK_EH_RESET_TIMER.

Now if the timeout fires again, assume the abort was unsuccessful and escalate to LUN reset.

Quoted 8 lines

This is fully asynchronous, fully tracked and doesn't rely on work queues.

The necessary additions for something like this are the from interrupt
issue abort and LUN reset, which could just be additional callbacks in
the host template.

Do we really need a new callback in the host template for a command abort that does not wait ? Several LLDs already have their own internal data structures for keeping track of the request state and can use these data structures to set a flag "command has been aborted". If aborting a command fails and the command completes that flag can then be used to avoid a second invocation of scsi_done(). At least, that's what the SRP initiator already does today in srp_abort().

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/virtio_scsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/scsi/virtio_scsi.c +6 -0

index 2168258..1efd219 100644
@@ -654,6 +654,11 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
 	return virtscsi_tmf(vscsi, cmd);
 }
 
+static enum blk_eh_timer_return virtscsi_timedout(struct scsi_cmnd *scmd)
+{
+	return scsi_abort_command(scmd);
+}
+
 static int virtscsi_target_alloc(struct scsi_target *starget)
 {
 	struct virtio_scsi_target_state *tgt =

@@ -683,6 +688,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
 	.queuecommand = virtscsi_queuecommand_single,
 	.eh_abort_handler = virtscsi_abort,
 	.eh_device_reset_handler = virtscsi_device_reset,
+	.eh_timed_out = virtscsi_timedout,
 
 	.can_queue = 1024,
 	.dma_boundary = UINT_MAX,

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/virtio_scsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/scsi/virtio_scsi.c +7 -0

index 2168258..1efd219 100644
@@ -654,6 +654,11 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
 	return virtscsi_tmf(vscsi, cmd);
 }
 
+static enum blk_eh_timer_return virtscsi_timedout(struct scsi_cmnd *scmd)
+{
+	return scsi_abort_command(scmd);
+}
+
 static int virtscsi_target_alloc(struct scsi_target *starget)
 {
 	struct virtio_scsi_target_state *tgt =

@@ -683,6 +688,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
 	.queuecommand = virtscsi_queuecommand_single,
 	.eh_abort_handler = virtscsi_abort,
 	.eh_device_reset_handler = virtscsi_device_reset,
+	.eh_timed_out = virtscsi_timedout,
 
 	.can_queue = 1024,
 	.dma_boundary = UINT_MAX,

@@ -699,6 +705,7 @@ static struct scsi_host_template virtscsi_host_template_multi = {
 	.queuecommand = virtscsi_queuecommand_multi,
 	.eh_abort_handler = virtscsi_abort,
 	.eh_device_reset_handler = virtscsi_device_reset,
+	.eh_timed_out = virtscsi_timedout,
 
 	.can_queue = 1024,
 	.dma_boundary = UINT_MAX,

Signature · 6 lines

1.7.12.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Any TMF might be take longer as expected, or not return at all. So we need to use 'wait_for_completion_timeout' when sending a TMF to protect against these cases.

Cc Paolo Bonzini <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/virtio_scsi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/scsi/virtio_scsi.c +1 -0

index 1efd219..abfc684 100644
@@ -30,6 +30,7 @@
 #define VIRTIO_SCSI_MEMPOOL_SZ 64
 #define VIRTIO_SCSI_EVENT_LEN 8
 #define VIRTIO_SCSI_VQ_BASE 2
+#define VIRTIO_SCSI_TMF_TIMEOUT 30
 
 /* Command queue element */
 struct virtio_scsi_cmd {

@@ -597,8 +598,10 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
 			      GFP_NOIO) < 0)
 		goto out;

Any TMF might be take longer as expected, or not return at all. So we need to use 'wait_for_completion_timeout' when sending a TMF to protect against these cases.

Cc Paolo Bonzini <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/virtio_scsi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/scsi/virtio_scsi.c +5 -2

index 1efd219..abfc684 100644
@@ -30,6 +30,7 @@
 #define VIRTIO_SCSI_MEMPOOL_SZ 64
 #define VIRTIO_SCSI_EVENT_LEN 8
 #define VIRTIO_SCSI_VQ_BASE 2
+#define VIRTIO_SCSI_TMF_TIMEOUT 30
 
 /* Command queue element */
 struct virtio_scsi_cmd {

@@ -597,8 +598,10 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
 			      GFP_NOIO) < 0)
 		goto out;
 
-	wait_for_completion(&comp);
-	if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
+	if (wait_for_completion_timeout(&comp,
+					VIRTIO_SCSI_TMF_TIMEOUT * HZ) == 0)
+		ret = FAILED;
+	else if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
 	    cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
 		ret = SUCCESS;
 

Signature · 6 lines

1.7.12.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt2sas/mpt2sas_scsih.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/scsi/mpt2sas/mpt2sas_scsih.c +0 -0

index c6bdc92..47fc66c 100644
@@ -2565,6 +2565,16 @@ _scsih_tm_display_info(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd)
 }

drivers/scsi/mpt2sas/mpt2sas_scsih.c +2 -0

+/**
  * _scsih_abort - eh threads main abort routine
  * @scmd: pointer to scsi command object
  *

@@ -7515,6 +7525,7 @@ static struct scsi_host_template scsih_driver_template = {
 	.scan_start			= _scsih_scan_start,
 	.change_queue_depth 		= _scsih_change_queue_depth,
 	.change_queue_type		= _scsih_change_queue_type,
+	.eh_timed_out			= _scsih_timed_out,
 	.eh_abort_handler		= _scsih_abort,
 	.eh_device_reset_handler	= _scsih_dev_reset,
 	.eh_target_reset_handler	= _scsih_target_reset,

Signature · 1 line

1.7.12.4

Quoted 12 lines · Hannes Reinecke

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt2sas/mpt2sas_scsih.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/scsi/mpt2sas/mpt2sas_scsih.c +0 -0

index c6bdc92..47fc66c 100644
@@ -2565,6 +2565,16 @@ _scsih_tm_display_info(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd)
 }

drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_tm_display_info(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd) · line 2575

Quoted 10 lines

drivers/scsi/mpt2sas/mpt2sas_scsih.c +8 -0

 
 /**
+ * _scsih_timed_out - eh timeout handler
+ * @scmd: pointer to scsi command object
+ */
+static enum blk_eh_timer_return
+_scsih_timed_out(struct scsi_cmnd *scmd)
+{
+	return scsi_abort_command(scmd);
+}

Why did you create this function and not do the following?