New EH command timeout handler

drivers/scsi/scsi_error.c +44 -0

+void
+scmd_eh_abort_handler(struct work_struct *work)
+{
+	struct scsi_cmnd *scmd =
+		container_of(work, struct scsi_cmnd, abort_work.work);
+	struct scsi_device *sdev = scmd->device;
+	unsigned long flags;
+	int rtn;
+
+	spin_lock_irqsave(sdev->host->host_lock, flags);
+	if (scsi_host_eh_past_deadline(sdev->host)) {
+		spin_unlock_irqrestore(sdev->host->host_lock, flags);
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "scmd %p eh timeout, not aborting\n", scmd));
+	} else {
+		spin_unlock_irqrestore(sdev->host->host_lock, flags);
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "aborting command %p\n", scmd));
+		rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
+		if (rtn == SUCCESS) {
+			scmd->result |= DID_TIME_OUT << 16;
+			if (!scsi_noretry_cmd(scmd) &&
+			    (++scmd->retries <= scmd->allowed)) {
+				SCSI_LOG_ERROR_RECOVERY(3,
+					scmd_printk(KERN_WARNING, scmd,
+						    "scmd %p retry "
+						    "aborted command\n", scmd));
+				scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
+			} else {
+				SCSI_LOG_ERROR_RECOVERY(3,
+					scmd_printk(KERN_WARNING, scmd,
+						    "scmd %p finish "
+						    "aborted command\n", scmd));
+				scsi_finish_command(scmd);
+			}
+			return;
+		}
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "scmd %p abort failed, rtn %d\n",
+				    scmd, rtn));
+	}

Quoted 4 lines · Hannes Reinecke

drivers/scsi/scsi.c +0 -0

@@ -353,6 +354,8 @@ void scsi_put_command(struct scsi_cmnd *cmd)
      list_del_init(&cmd->list);
      spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 

drivers/scsi/scsi.c void scsi_put_command(struct scsi_cmnd *cmd) · line 361

Quoted 5 lines

drivers/scsi/scsi.c +2 -0

+    cancel_delayed_work(&cmd->abort_work);
+
      __scsi_put_command(cmd->device->host, cmd, &sdev->sdev_gendev);
  }
  EXPORT_SYMBOL(scsi_put_command);

Is this approach safe ? Is it e.g. possible that the abort work starts just before the cancel_delayed_work() call and continues to run after scsi_put_command() has finished ? In drivers/scsi/libfc/fc_exch.c a similar issue is solved by holding an additional reference as long as delayed work (fc_exch.timeout_work) is queued.

Quoted 52 lines

drivers/scsi/scsi_error.c +52 -0

+void
+scmd_eh_abort_handler(struct work_struct *work)
+{
+    struct scsi_cmnd *scmd =
+        container_of(work, struct scsi_cmnd, abort_work.work);
+    struct scsi_device *sdev = scmd->device;
+    unsigned long flags;
+    int rtn;
+
+    spin_lock_irqsave(sdev->host->host_lock, flags);
+    if (scsi_host_eh_past_deadline(sdev->host)) {
+        spin_unlock_irqrestore(sdev->host->host_lock, flags);
+        SCSI_LOG_ERROR_RECOVERY(3,
+            scmd_printk(KERN_INFO, scmd,
+                    "scmd %p eh timeout, not aborting\n", scmd));
+    } else {
+        spin_unlock_irqrestore(sdev->host->host_lock, flags);
+        SCSI_LOG_ERROR_RECOVERY(3,
+            scmd_printk(KERN_INFO, scmd,
+                    "aborting command %p\n", scmd));
+        rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
+        if (rtn == SUCCESS) {
+            scmd->result |= DID_TIME_OUT << 16;
+            if (!scsi_noretry_cmd(scmd) &&
+                (++scmd->retries <= scmd->allowed)) {
+                SCSI_LOG_ERROR_RECOVERY(3,
+                    scmd_printk(KERN_WARNING, scmd,
+                            "scmd %p retry "
+                            "aborted command\n", scmd));
+                scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
+            } else {
+                SCSI_LOG_ERROR_RECOVERY(3,
+                    scmd_printk(KERN_WARNING, scmd,
+                            "scmd %p finish "
+                            "aborted command\n", scmd));
+                scsi_finish_command(scmd);
+            }
+            return;
+        }
+        SCSI_LOG_ERROR_RECOVERY(3,
+            scmd_printk(KERN_INFO, scmd,
+                    "scmd %p abort failed, rtn %d\n",
+                    scmd, rtn));
+    }
+
+    if (scsi_eh_scmd_add(scmd, 0)) {
+        SCSI_LOG_ERROR_RECOVERY(3,
+            scmd_printk(KERN_WARNING, scmd,
+                    "scmd %p terminate "
+                    "aborted command\n", scmd));
+        scmd->result |= DID_TIME_OUT << 16;
+        scsi_finish_command(scmd);

Review

Quoted 2 lines

(quoted diff) +2 -0

+    }
+}

This patch adds several new calls to LLD EH handlers. Is it guaranteed that these will only be invoked before scsi_remove_host() has finished ? For more background information, see also "[PATCH] Make scsi_remove_host() wait until error handling finished"
(http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Hannes Reinecke

drivers/scsi/scsi.c +0 -0

@@ -353,6 +354,8 @@ void scsi_put_command(struct scsi_cmnd *cmd)
       list_del_init(&cmd->list);
       spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 

Quoted 13 lines

drivers/scsi/scsi.c void scsi_put_command(struct scsi_cmnd *cmd) · line 361

Quoted 5 lines

drivers/scsi/scsi.c +2 -0

+    cancel_delayed_work(&cmd->abort_work);
+
       __scsi_put_command(cmd->device->host, cmd, &sdev->sdev_gendev);
   }
   EXPORT_SYMBOL(scsi_put_command);

Is this approach safe ? Is it e.g. possible that the abort work starts just before the cancel_delayed_work() call and continues to run after scsi_put_command() has finished ? In drivers/scsi/libfc/fc_exch.c a similar issue is solved by holding an additional reference as long as delayed work (fc_exch.timeout_work) is queued.

I have been thinking of this, and in fact my original approach had 'cancel_delayed_work_sync' here. However, this didn't work as scsi_put_command() might end up being called from an softirq context.

Quoted 1 line

From my understanding the workqueue stuff guarantees that either

a) the workqueue item is still queued
-> cancel_delayed_work will be in fact synchronous, as it'll
cancel queue item from the queue
b) the workqueue item is running
-> cancel_delayed_work is essentially a no-op, as the function
is running and will be terminated anyway.

IE from the API perspective the transition between 'queued' and 'running' is atomic, and no in-between states are visible.

So case a) is obviously safe, and for case b) the abort function is already running. But then the abort function has been called from the block timeout handler, which did a blk_mark_rq_complete() prior to calling the handler. So any completion coming in after that will be ignored, and scsi_put_command() won't be called.

Hence we should be safe here.

Quoted 6 lines · Bart Van Assche

This patch adds several new calls to LLD EH handlers. Is it guaranteed that these will only be invoked before scsi_remove_host() has finished ? For more background information, see also "[PATCH] Make scsi_remove_host() wait until error handling finished"
(http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).

Well, that depends how scsi_remove_host() handles outstanding commands. What happens if you call scsi_remove_host() and there is still I/O in flight? I would assume that any HBA would have to kill any outstanding I/O prior to calling scsi_remove_host() (FC most certainly does this).
Which would mean that it'll have to wait for scsi_put_command() to be called for all outstanding commands. And as scsi_put_command() will be called only _after_ our routine runs (see the reasoning above) we should be safe.

Quoted 7 lines · Hannes Reinecke

Quoted 6 lines · Bart Van Assche

This patch adds several new calls to LLD EH handlers. Is it guaranteed that these will only be invoked before scsi_remove_host() has finished ? For more background information, see also "[PATCH] Make scsi_remove_host() wait until error handling finished"
(http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).

Quoted 9 lines

Well, that depends how scsi_remove_host() handles outstanding commands. What happens if you call scsi_remove_host() and there is still I/O in flight? I would assume that any HBA would have to kill any outstanding I/O prior to calling scsi_remove_host() (FC most certainly does this).
Which would mean that it'll have to wait for scsi_put_command() to be called for all outstanding commands. And as scsi_put_command() will be called only _after_ our routine runs (see the reasoning above) we should be safe.

Hello Hannes,

Since fc_remove_host() has to be invoked before scsi_remove_host() and since fc_remove_host() changes the port state into FC_PORTSTATE_DELETED this means that fc_remote_port_chkready() will return DID_NO_CONNECT while scsi_remove_host() is in progress. I think this prevents that the SYNCHRONIZE CACHE command submitted by sd_remove() reaches SCSI disks over FC since sd_remove() is invoked from inside scsi_remove_host(). The SRP transport patch I had posted recently makes sure that the SYNCHRONIZE CACHE command submitted by sd_remove() reaches the target SCSI disk. This makes me wonder what the correct behavior is for SCSI transport drivers - disabling I/O before scsi_remove_host() starts or ensuring that I/O is still possible while scsi_remove_host() is in progress ? I think the call chain is as follows: scsi_remove_host() -> device_del() -> bus_remove_device() -> device_release_driver() -> __device_release_driver() -> sd_remove() -> sd_shutdown() -> sd_sync_cache().

One thing I'm still wondering is why we can't enable this globally, and if there is a reason it should be documented.

As far as I can tell the actual calling context of the eh_abort_handler doesn't change, so an LLDD would have to rely on some very specific side effects of being in EH to break, and we never guaranteed such specifics.

I don't mind. Having talked to the various SCSI folks everyone agreed that calling abort asynchronously shouldn't do any harm. At least as far as the SCSI spec goes.

As for documentation: I didn't document it it currently as with my implementation it's pretty much an optional thing.
But if we were to enable it globally it surely should be documented.

So if there is a consensus I surely can enable it globally and update the documentation.

Quoted 10 lines · Hannes Reinecke

I don't mind. Having talked to the various SCSI folks everyone agreed that calling abort asynchronously shouldn't do any harm. At least as far as the SCSI spec goes.

As for documentation: I didn't document it it currently as with my implementation it's pretty much an optional thing.
But if we were to enable it globally it surely should be documented.

So if there is a consensus I surely can enable it globally and update the documentation.

I would defintively love enabling it globally. As a fallback we should probably keep the old code for a while and prepare a way to disable the async code by a flag in the host template. If no reason to enable it shows up after a year ore two we can remove the old code entirely.

I.e change the tail of scsi_times_out into something like:

	if (rtn == BLK_EH_NOT_HANDLED && !shost->hostt->no_async_abort) {
		if (scsi_abort_command(scmd))
			return BLK_EH_NOT_HANDLED;
	}

	scmd->result |= DID_TIME_OUT << 16;
	...

That way we don't even need the new BLK_EH_SCHEDULED return value and have all the handling in scsi_error.c

I would defintively love enabling it globally.