Fixes for SCSI device removal

Some request_queue.end_io implementations can be called safely without the queue lock held while several other implementations assume that the queue lock is held. So let's play it safe and make sure that the queue lock is held around end_io invocations. Found this through source code review.

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

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc <[email protected]>

Quoted 10 lines · Bart Van Assche

Some request_queue.end_io implementations can be called safely without the queue lock held while several other implementations assume that the queue lock is held. So let's play it safe and make sure that the queue lock is held around end_io invocations. Found this through source code review.

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

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc <[email protected]>

Acked-by Tejun Heo <[email protected]>

Not sure about stable@ tho. This doesn't fix any visible issue at the moment and any change carries some risk.

Some request_queue.end_io implementations can be called safely without the queue lock held while several other implementations assume that the queue lock is held. So let's play it safe and make sure that the queue lock is held around end_io invocations. Found this through source code review.

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

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc <[email protected]>

---
 block/blk-exec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

block/blk-exec.c +0 -0

index fb2cbd5..6724fab 100644

Quoted 21 lines · Bart Van Assche

Some request_queue.end_io implementations can be called safely without the queue lock held while several other implementations assume that the queue lock is held. So let's play it safe and make sure that the queue lock is held around end_io invocations. Found this through source code review.

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

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc <[email protected]>

---
 block/blk-exec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

block/blk-exec.c +0 -0

index fb2cbd5..6724fab 100644
@@ -54,10 +54,10 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	spin_lock_irq(q->queue_lock);
 

block/blk-exec.c void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk, · line 62

Quoted 8 lines

block/blk-exec.c +1 -1

 	if (unlikely(blk_queue_dead(q))) {
-		spin_unlock_irq(q->queue_lock);
 		rq->errors = -ENXIO;
 		if (rq->end_io)
 			rq->end_io(rq, rq->errors);
+		spin_unlock_irq(q->queue_lock);
 		return;
 	}

I'm assuming you checked any in-kernel users of rq->end_io to ensure that it is fine? If so, patch looks fine to me. And I agree, it's not stable material.

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +0 -0

index 082c1e5..b722a8b 100644

Quoted 20 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 13 lines

drivers/scsi/scsi_lib.c +4 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);
+		kblockd_schedule_work(q, &device->requeue_work);
+	}

If we do not requeue what eventually frees the request?

Quoted 21 lines · Mike Christie

Quoted 20 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

Quoted 15 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 13 lines

drivers/scsi/scsi_lib.c +4 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);
+		kblockd_schedule_work(q, &device->requeue_work);
+	}

If we do not requeue what eventually frees the request?

As far as I can see any request passed to __scsi_queue_insert() has already been started. So if it isn't requeued it's timer remains active and hence will fire eventually.

Quoted 39 lines · Bart Van Assche

Quoted 36 lines · Mike Christie

Quoted 25 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +0 -0

index 082c1e5..b722a8b 100644

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 8 lines

drivers/scsi/scsi_lib.c +4 -1

@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);
+		kblockd_schedule_work(q, &device->requeue_work);
+	}

If we do not requeue what eventually frees the request?

Quoted 3 lines

As far as I can see any request passed to __scsi_queue_insert() has already been started. So if it isn't requeued it's timer remains active and hence will fire eventually.

This is true in the scsi_dispatch_cmd path, but not others.

If we were requeueing from the error handler scsi_eh_flush_done_q then the timer would have been stopped because that is how we got into the eh.

Quoted 44 lines · Mike Christie

Quoted 42 lines · Bart Van Assche

Quoted 21 lines · Mike Christie

Quoted 20 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

Quoted 15 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 13 lines

drivers/scsi/scsi_lib.c +4 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);
+		kblockd_schedule_work(q, &device->requeue_work);
+	}

If we do not requeue what eventually frees the request?

As far as I can see any request passed to __scsi_queue_insert() has already been started. So if it isn't requeued it's timer remains active and hence will fire eventually.

Quoted 4 lines

This is true in the scsi_dispatch_cmd path, but not others.

If we were requeueing from the error handler scsi_eh_flush_done_q then the timer would have been stopped because that is how we got into the eh.

Given the above I propose to add a __blk_end_request_all(req, -ENXIO) call if the queue is dead.

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +0 -0

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.

drivers/scsi/scsi_lib.c +0 -0

 }
 
 /*

drivers/scsi/scsi_sysfs.c +0 -0

index 42c35ff..efffc92 100644
@@ -966,13 +966,20 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		device_del(dev);
 	} else

drivers/scsi/scsi_sysfs.c +1 -2

+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);
 
-	/* Freeing the queue signals to block that we're done */
-	blk_cleanup_queue(sdev->request_queue);
 	put_device(dev);
 }
 

Quoted 31 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +2 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 5 lines

drivers/scsi/scsi_lib.c +2 -2

+		kblockd_schedule_work(q, &device->requeue_work);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	kblockd_schedule_work(q, &device->requeue_work);

If we do not have the part of the patch above, but have your other patches and the code below, will we be ok?

Quoted 32 lines · Mike Christie

Quoted 31 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +2 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);

Quoted 8 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 5 lines

drivers/scsi/scsi_lib.c +2 -2

+		kblockd_schedule_work(q, &device->requeue_work);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	kblockd_schedule_work(q, &device->requeue_work);

If we do not have the part of the patch above, but have your other patches and the code below, will we be ok?

I'm not sure. Without the above part the request could get killed after the blk_requeue_request() call finished but before the requeue_work is scheduled, e.g. because the request timer fired or due to a blk_abort_queue() call.

Quoted 44 lines · Bart Van Assche

Quoted 32 lines · Mike Christie

Quoted 31 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +2 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);

Quoted 8 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 5 lines

drivers/scsi/scsi_lib.c +2 -2

+		kblockd_schedule_work(q, &device->requeue_work);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	kblockd_schedule_work(q, &device->requeue_work);

If we do not have the part of the patch above, but have your other patches and the code below, will we be ok?

Quoted 5 lines

I'm not sure. Without the above part the request could get killed after the blk_requeue_request() call finished but before the requeue_work is scheduled, e.g. because the request timer fired or due to a blk_abort_queue() call.

You are right.

What if we moved the requeue work struct to the request queue, then have blk_cleanup_queue or blk_drain_queue call cancel_work_sync before the queue is freed. That way that code could make sure the queue and work is flushed and drained, and it can make sure it is flushed and drained before freeing the queue?

Quoted 44 lines · Bart Van Assche

Quoted 32 lines · Mike Christie

Quoted 31 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +2 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);

Quoted 8 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 5 lines

drivers/scsi/scsi_lib.c +2 -2

+		kblockd_schedule_work(q, &device->requeue_work);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	kblockd_schedule_work(q, &device->requeue_work);

If we do not have the part of the patch above, but have your other patches and the code below, will we be ok?

Quoted 5 lines

I'm not sure. Without the above part the request could get killed after the blk_requeue_request() call finished but before the requeue_work is scheduled, e.g. because the request timer fired or due to a blk_abort_queue() call.

You are right.

What if we moved the requeue work struct to the request queue, then have blk_cleanup_queue or blk_drain_queue call cancel_work_sync before the queue is freed. That way that code could make sure the queue and work is flushed and drained, and it can make sure it is flushed and drained before freeing the queue?

Quoted 51 lines · Mike Christie

Quoted 44 lines · Bart Van Assche

Quoted 32 lines · Mike Christie

Quoted 31 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +2 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);

Quoted 8 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 5 lines

drivers/scsi/scsi_lib.c +2 -2

+		kblockd_schedule_work(q, &device->requeue_work);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	kblockd_schedule_work(q, &device->requeue_work);

If we do not have the part of the patch above, but have your other patches and the code below, will we be ok?

Quoted 5 lines

I'm not sure. Without the above part the request could get killed after the blk_requeue_request() call finished but before the requeue_work is scheduled, e.g. because the request timer fired or due to a blk_abort_queue() call.

Quoted 7 lines

You are right.

What if we moved the requeue work struct to the request queue, then have blk_cleanup_queue or blk_drain_queue call cancel_work_sync before the queue is freed. That way that code could make sure the queue and work is flushed and drained, and it can make sure it is flushed and drained before freeing the queue?

Or, in scsi_requeue_run_queue could we just add a check for the scsi_device being in the SDEV_DEL state. That combined with your cancel call in __scsi_remove_device would prevent us from running a cleaned up queue, right?

Quoted 60 lines · Mike Christie

Quoted 51 lines · Mike Christie

Quoted 44 lines · Bart Van Assche

Quoted 32 lines · Mike Christie

Quoted 31 lines · Bart Van Assche

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +2 -1

index 082c1e5..b722a8b 100644
@@ -158,10 +158,11 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	 * that are already in the queue.
 	 */
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	if (!blk_queue_dead(q)) {
+		blk_requeue_request(q, cmd->request);

Quoted 8 lines

drivers/scsi/scsi_lib.c static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy) · line 164

Quoted 5 lines

drivers/scsi/scsi_lib.c +2 -2

+		kblockd_schedule_work(q, &device->requeue_work);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	kblockd_schedule_work(q, &device->requeue_work);

If we do not have the part of the patch above, but have your other patches and the code below, will we be ok?

Quoted 5 lines

I'm not sure. Without the above part the request could get killed after the blk_requeue_request() call finished but before the requeue_work is scheduled, e.g. because the request timer fired or due to a blk_abort_queue() call.

Quoted 7 lines

You are right.

What if we moved the requeue work struct to the request queue, then have blk_cleanup_queue or blk_drain_queue call cancel_work_sync before the queue is freed. That way that code could make sure the queue and work is flushed and drained, and it can make sure it is flushed and drained before freeing the queue?

Quoted 4 lines

Or, in scsi_requeue_run_queue could we just add a check for the scsi_device being in the SDEV_DEL state. That combined with your cancel call in __scsi_remove_device would prevent us from running a cleaned up queue, right?

I'm not sure. If a requeued request times out before blk_cleanup_queue() is invoked then it's possible that the requeue_work is started after the struct scsi_device has already been deleted.

Quoted 3 lines · Bart Van Assche

I'm not sure. If a requeued request times out before blk_cleanup_queue() is invoked then it's possible that the requeue_work is started after the struct scsi_device has already been deleted.

Won't the cancel_work_sync call you are adding prevent that? After blk_cleanup_queue has returned we know that no IO is running or in the eh, and we know no new IO will be started. And then, after the cancel_work_sync call you are adding has returned we know that there will not be any workstruct queued or running.

What if we moved the requeue work struct to the request queue, then have blk_cleanup_queue or blk_drain_queue call cancel_work_sync before the queue is freed. That way that code could make sure the queue and work is flushed and drained, and it can make sure it is flushed and drained before freeing the queue?