Remove the cancel_delayed_work() call from scsi_put_command()

drivers/scsi/scsi_error.c +22 -0

+static bool scmd_being_handled_in_other_context(struct scsi_cmnd *scmd)
+{
+	struct Scsi_Host *shost = scmd->device->host;
+	struct scsi_cmnd *c;
+	unsigned long flags;
+	bool ret = false;
+
+	if (!blk_rq_completed(scmd->request))
+		return true;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry(c, &shost->eh_cmd_q, eh_entry) {
+		if (c == scmd) {
+			ret = true;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	return ret;
+}
+
 /**
  * scmd_eh_abort_handler - Handle command aborts
  * @work:	command to be aborted.

Il 21/05/2014 15:30, Bart Van Assche ha scritto:

Quoted 29 lines

drivers/scsi/scsi_error.c +22 -0

+static bool scmd_being_handled_in_other_context(struct scsi_cmnd *scmd)
+{
+    struct Scsi_Host *shost = scmd->device->host;
+    struct scsi_cmnd *c;
+    unsigned long flags;
+    bool ret = false;
+
+    if (!blk_rq_completed(scmd->request))
+        return true;
+
+    spin_lock_irqsave(shost->host_lock, flags);
+    list_for_each_entry(c, &shost->eh_cmd_q, eh_entry) {
+        if (c == scmd) {
+            ret = true;
+            break;
+        }
+    }
+    spin_unlock_irqrestore(shost->host_lock, flags);
+
+    return ret;
+}
+
 /**
  * scmd_eh_abort_handler - Handle command aborts
  * @work:    command to be aborted.

@@ -120,6 +142,8 @@ scmd_eh_abort_handler(struct work_struct *work)
     struct scsi_device *sdev = scmd->device;
     int rtn;
 

Review scmd_eh_abort_handler(struct work_struct *work) · line 145

Quoted 1 line

(quoted diff) +1 -0

+    WARN_ON_ONCE(scmd_being_handled_in_other_context(scmd));

What about a simpler, though less accuracte

    WARN_ON(!blk_rq_completed(scmd->request));

that doesn't need the host_lock?

scmd->abort_work is only scheduled after the block layer has marked the request associated with a command as complete and for commands that are not on the eh_cmd_q list. A SCSI command is only requeued after the scmd->abort_work handler has started (requeueing clears the "complete" flag). This means that the cancel_delayed_work() statement in scsi_put_command() is a no-op. Hence remove it.

Additionally, document how it is avoided that scsi_finish_command() and the SCSI error handler code are invoked concurrently for the same command via WARN_ON_ONCE() statements. This should avoid that the scsi error handler code confuses its readers.

Signed-off-by Bart Van Assche <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

---
  block/blk-softirq.c       |  6 ++++++
  drivers/scsi/scsi.c       |  2 --
  drivers/scsi/scsi_error.c | 28 ++++++++++++++++++++++++++++
  include/linux/blkdev.h    |  1 +
  4 files changed, 35 insertions(+), 2 deletions(-)

block/blk-softirq.c +6 -0

index 53b1737..59bb52d 100644
@@ -172,6 +172,12 @@ void blk_complete_request(struct request *req)
  }
  EXPORT_SYMBOL(blk_complete_request);
 
+bool blk_rq_completed(struct request *rq)
+{
+	return test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
+}
+EXPORT_SYMBOL(blk_rq_completed);
+
  static __init int blk_softirq_init(void)
  {
  	int i;

drivers/scsi/scsi.c +0 -2

index 88d46fe..04a282a 100644
@@ -334,8 +334,6 @@ void scsi_put_command(struct scsi_cmnd *cmd)
  	list_del_init(&cmd->list);
  	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-	cancel_delayed_work(&cmd->abort_work);
-
  	__scsi_put_command(cmd->device->host, cmd);
  }
  EXPORT_SYMBOL(scsi_put_command);

drivers/scsi/scsi_error.c +15 -0

index 14ce3b4..32a8cd1 100644
@@ -108,6 +108,28 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
  	return 1;
  }
 
+static bool scmd_being_handled_in_other_context(struct scsi_cmnd *scmd)
+{
+	struct Scsi_Host *shost = scmd->device->host;
+	struct scsi_cmnd *c;
+	unsigned long flags;
+	bool ret = false;
+
+	if (!blk_rq_completed(scmd->request))
+		return true;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry(c, &shost->eh_cmd_q, eh_entry) {
+		if (c == scmd) {
+			ret = true;
+			break;

And when freeing a command we absolutely need to make sure that the workqueue is empty.
So calling cancel_delayed_work() was the obvious thing to do.

I'd be fine with adding a WARN_ON(!list_empty(&cmd->abort_work)) here, however. This will clear up the intent of this statement.

Il 23/05/2014 08:09, Hannes Reinecke ha scritto:

Quoted 4 lines

And when freeing a command we absolutely need to make sure that the workqueue is empty.
So calling cancel_delayed_work() was the obvious thing to do.

You would need cancel_delayed_work_sync, but if it really happened that the work item is running, it would cause a double free.

Quoted 2 lines

I'd be fine with adding a WARN_ON(!list_empty(&cmd->abort_work)) here, however. This will clear up the intent of this statement.

BUG_ON even, since you'd get badness from the double free anyway.

Quoted 12 lines

Il 23/05/2014 08:09, Hannes Reinecke ha scritto:

Quoted 4 lines

And when freeing a command we absolutely need to make sure that the workqueue is empty.
So calling cancel_delayed_work() was the obvious thing to do.

You would need cancel_delayed_work_sync, but if it really happened that the work item is running, it would cause a double free.

Quoted 2 lines

I'd be fine with adding a WARN_ON(!list_empty(&cmd->abort_work)) here, however. This will clear up the intent of this statement.

Quoted 1 line

BUG_ON even, since you'd get badness from the double free anyway.

Hello Paolo,

Are you aware that Linus strongly prefers WARN_ON_ONCE() over BUG_ON() ? See e.g. https://lkml.org/lkml/2012/9/27/461 or
https://lkml.org/lkml/2014/4/28/657.

Il 23/05/2014 12:37, Bart Van Assche ha scritto:

Quoted 14 lines · Paolo Bonzini

Quoted 13 lines

Il 23/05/2014 08:09, Hannes Reinecke ha scritto:

Quoted 4 lines

And when freeing a command we absolutely need to make sure that the workqueue is empty.
So calling cancel_delayed_work() was the obvious thing to do.

You would need cancel_delayed_work_sync, but if it really happened that
the work item is running, it would cause a double free.

Quoted 2 lines

I'd be fine with adding a WARN_ON(!list_empty(&cmd->abort_work)) here, however. This will clear up the intent of this statement.

Quoted 8 lines

Quoted 1 line

BUG_ON even, since you'd get badness from the double free anyway.

Hello Paolo,

Are you aware that Linus strongly prefers WARN_ON_ONCE() over BUG_ON() ?
See e.g. https://lkml.org/lkml/2012/9/27/461 or
https://lkml.org/lkml/2014/4/28/657.

Yes, I am and I even downgraded some KVM BUG_ONs recently.

But in this case I think that memory corruption is going to happen anyway unless you consciously leak the Scsi_Cmnd * (because if you use WARN_ON, you also need to return early as Linus suggested in the second email).

So the WARN_ON/BUG_ON choice here should not just consider what makes the problem easier to debug; hanging the machine before guaranteed badness seems to me like a good use for BUG_ON.