Fix race between starved list processing and device removal

PATCH Superseded Bart Van Assche 2 participants 6 messages Aug 9, 2012 – Oct 7, 2012 2 messages not in the archive SCSI

Share & tools

Bart Van Assche

author

2012-08-09 18:59 UTC #0 lore raw

PATCH 1/1 Fix race between starved list processing and device removal

Avoid that the sdev reference count can drop to zero before the queue is run by scsi_run_queue(). Also avoid that the sdev reference count can drop to zero in the same function by invoking __blk_run_queue().

Reported-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

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

Reviewed-by Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc <[email protected]>

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

drivers/scsi/scsi_lib.c +5 -0

index ffd7773..bd7daec 100644
@@ -452,10 +452,15 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
+		get_device(&sdev->sdev_gendev);
 		spin_unlock(shost->host_lock);
+
 		spin_lock(sdev->request_queue->queue_lock);
 		__blk_run_queue(sdev->request_queue);
 		spin_unlock(sdev->request_queue->queue_lock);
+
+		put_device(&sdev->sdev_gendev);
+
 		spin_lock(shost->host_lock);
 	}
 	/* put any unprocessed entries back */

drivers/scsi/scsi_sysfs.c +6 -1

index 093d4f6..44f232e 100644
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);

Signature · 1 line

1.7.7

A message that is not in the archive (sent off-list or not yet received): 001901cd8281$d49132d0$7db39870$@[email protected]

Bart Van Assche

author

2012-08-25 06:44 UTC #2 lore raw

Quoted 3 lines · Chanho Min

I still have this race issue in our platform.
If this patch is not accepted, I have to find another approach. Any comment will be appreciated.

Which kernel version are you using in your tests, and what is the oops message the kernel reports ?

Bart.

A message that is not in the archive (sent off-list or not yet received): 001a01cd828e$944acf30$bce06d90$@[email protected]

Bart Van Assche

author

2012-08-25 07:31 UTC #4 lore raw

Quoted 10 lines · Chanho Min

Quoted 7 lines · Chanho Min
Quoted 3 lines
I still have this race issue in our platform.
If this patch is not accepted, I have to find another approach. Any comment will be appreciated.
Which kernel version are you using in your tests, and what is the oops message the kernel reports ?
I mean it's solved if your patch is applied. I hope that this patch is accepted.

Ah, that's great. Thanks for testing this patch !

Bart.

Bart Van Assche

author

2012-09-24 13:14 UTC #5 lore raw

PATCH RESEND 1/1 Fix race between starved list processing and device removal

In reply to Bart Van Assche

Reported-by Chanho Min <[email protected]>

Tested-by Chanho Min <[email protected]>

Reviewed-by Mike Christie <[email protected]>

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

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

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

drivers/scsi/scsi_lib.c +5 -0

index ffd7773..bd7daec 100644
@@ -452,10 +452,15 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
+		get_device(&sdev->sdev_gendev);
 		spin_unlock(shost->host_lock);
+
 		spin_lock(sdev->request_queue->queue_lock);
 		__blk_run_queue(sdev->request_queue);
 		spin_unlock(sdev->request_queue->queue_lock);
+
+		put_device(&sdev->sdev_gendev);
+
 		spin_lock(shost->host_lock);
 	}
 	/* put any unprocessed entries back */

drivers/scsi/scsi_sysfs.c +6 -1

index 093d4f6..44f232e 100644
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);

Signature · 1 line

1.7.7

James Bottomley

maintainer

2012-10-07 10:47 UTC #6 lore raw

Quoted 4 lines · Bart Van Assche

Avoid that the sdev reference count can drop to zero before the queue is run by scsi_run_queue(). Also avoid that the sdev reference count can drop to zero in the same function by invoking __blk_run_queue().

[...] if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 984

Quoted 8 lines

drivers/scsi/scsi_sysfs.c +4 -0

@@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+

This hunk doesn't make much sense. It seems to be orthogonal to the problem listed in the changelog and this action is done on last put anyway.

James

Bart Van Assche

author

2012-10-07 18:24 UTC #7 lore raw

Quoted 10 lines · James Bottomley

Quoted 4 lines · Bart Van Assche
Avoid that the sdev reference count can drop to zero before the queue is run by scsi_run_queue(). Also avoid that the sdev reference count can drop to zero in the same function by invoking __blk_run_queue().
[...] if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
Quoted 4 lines
drivers/scsi/scsi_sysfs.c +0 -0
@@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
  	blk_cleanup_queue(sdev->request_queue);
  	cancel_work_sync(&sdev->requeue_work);
 

Quoted 8 lines

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 984
Quoted 4 lines
drivers/scsi/scsi_sysfs.c +4 -0
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
This hunk doesn't make much sense. It seems to be orthogonal to the problem listed in the changelog and this action is done on last put anyway.

Removing an sdev from the starved list in __scsi_remove_device() has the advantage that it is guaranteed that the get_device() call added in scsi_run_queue() will succeed. A possible alternative is to leave the starved list removal code in scsi_device_dev_release_usercontext() and to invoke __blk_run_queue() in scsi_run_queue() only if the get_device() call in that function succeeded. Does this mean that you prefer the second option - something like the (untested) code below ?

		if (get_device(&sdev->sdev_gendev)) {
			spin_unlock(shost->host_lock);

			spin_lock(sdev->request_queue->queue_lock);
			__blk_run_queue(sdev->request_queue);
			spin_unlock(sdev->request_queue->queue_lock);

			put_device(&sdev->sdev_gendev);
			spin_lock(shost->host_lock);
		}

Bart.