Remove two cancel_delayed_work() calls from the mid-layer

PATCH In review Bart Van Assche 3 participants 3 messages Jun 2, 2014 SCSI

Share & tools

Bart Van Assche

author

2014-06-02 09:50 UTC #0 lore raw

PATCH 1/1 Remove two cancel_delayed_work() calls from the mid-layer

scsi_put_command() is either invoked before blk_start_request() or after block layer processing has completed. scsi_cmnd.abort_work is scheduled from inside the SCSI timeout handler. The block layer guarantees that either the regular completion handler (softirq_done_fn()) or the timeout handler (rq_timed_out_fn()) is invoked but not both. This means that scsi_put_command() is never invoked while abort_work is scheduled. Hence remove the cancel_delayed_work() call from scsi_put_command().

Similarly, scsi_abort_command() is only invoked from the SCSI timeout handler. If scsi_abort_command() is invoked for a SCSI command with the SCSI_EH_ABORT_SCHEDULED flag set this means that scmd_eh_abort_handler() has already invoked scsi_queue_insert() and hence that scsi_cmnd.abort_work is no longer pending. Hence also remove the cancel_delayed_work() call from scsi_abort_command().

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

---
 drivers/scsi/scsi.c       | 2 +-
 drivers/scsi/scsi_error.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +1 -1

index 88d46fe..53b8b94 100644
@@ -334,7 +334,7 @@ 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);
+	BUG_ON(delayed_work_pending(&cmd->abort_work));
 
 	__scsi_put_command(cmd->device->host, cmd);
 }

drivers/scsi/scsi_error.c +1 -1

index f17aa7a..919ade2 100644
@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_INFO, scmd,
 				    "scmd %p previous abort failed\n", scmd));
-		cancel_delayed_work(&scmd->abort_work);
+		BUG_ON(delayed_work_pending(&scmd->abort_work));
 		return FAILED;
 	}

Signature · 1 line

1.8.4.5

Hannes Reinecke

Reviewed-by on v1 1/1

2014-06-02 09:51 UTC #1 lore raw

Quoted 46 lines · Bart Van Assche

scsi_put_command() is either invoked before blk_start_request() or after block layer processing has completed. scsi_cmnd.abort_work is scheduled from inside the SCSI timeout handler. The block layer guarantees that either the regular completion handler (softirq_done_fn()) or the timeout handler (rq_timed_out_fn()) is invoked but not both. This means that scsi_put_command() is never invoked while abort_work is scheduled. Hence remove the cancel_delayed_work() call from scsi_put_command().
Similarly, scsi_abort_command() is only invoked from the SCSI timeout handler. If scsi_abort_command() is invoked for a SCSI command with the SCSI_EH_ABORT_SCHEDULED flag set this means that scmd_eh_abort_handler() has already invoked scsi_queue_insert() and hence that scsi_cmnd.abort_work is no longer pending. Hence also remove the cancel_delayed_work() call from scsi_abort_command().
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]>
---
  drivers/scsi/scsi.c       | 2 +-
  drivers/scsi/scsi_error.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/scsi.c +1 -1
index 88d46fe..53b8b94 100644
@@ -334,7 +334,7 @@ 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);
+	BUG_ON(delayed_work_pending(&cmd->abort_work));
 
  	__scsi_put_command(cmd->device->host, cmd);
  }
drivers/scsi/scsi_error.c +0 -0
index f17aa7a..919ade2 100644

drivers/scsi/scsi_error.c scsi_abort_command(struct scsi_cmnd *scmd) · line 199

Quoted 8 lines

drivers/scsi/scsi_error.c +1 -1

@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
  		SCSI_LOG_ERROR_RECOVERY(3,
  			scmd_printk(KERN_INFO, scmd,
  				    "scmd %p previous abort failed\n", scmd));
-		cancel_delayed_work(&scmd->abort_work);
+		BUG_ON(delayed_work_pending(&scmd->abort_work));
  		return FAILED;
  	}

Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig

Reviewed-by on v1 1/1

2014-06-02 11:32 UTC #2 lore raw

In reply to Bart Van Assche

Quoted 9 lines · Bart Van Assche

scsi_put_command() is either invoked before blk_start_request() or after block layer processing has completed. scsi_cmnd.abort_work is scheduled from inside the SCSI timeout handler. The block layer guarantees that either the regular completion handler (softirq_done_fn()) or the timeout handler (rq_timed_out_fn()) is invoked but not both. This means that scsi_put_command() is never invoked while abort_work is scheduled. Hence remove the cancel_delayed_work() call from scsi_put_command().

Quoted 8 lines

Similarly, scsi_abort_command() is only invoked from the SCSI timeout handler. If scsi_abort_command() is invoked for a SCSI command with the SCSI_EH_ABORT_SCHEDULED flag set this means that scmd_eh_abort_handler() has already invoked scsi_queue_insert() and hence that scsi_cmnd.abort_work is no longer pending. Hence also remove the cancel_delayed_work() call from scsi_abort_command().
Signed-off-by Bart Van Assche <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>