Perfromance drop on SCSI hard disk

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here. :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

Quoted 9 lines

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here.  :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

Quoted 7 lines

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

Hi Jens,
I want to add more analysis about the problem to help understand the issue. This is a severe problem, hopefully we can solve it before 2.6.39 release.

Basically the offending patch has some problems:
a. more context switches
b. __blk_run_queue losts the recursive detection. In some cases, it could be called recursively. For example, blk_run_queue in scsi_run_queue() c. fairness issue. Say we have sdev1 and sdev2 in starved_list. Then run scsi_run_queue():
 1. remove both sdev1 and sdev2 from starved_list
 2. async queue dispatches sdev1's request. host becames busy again.
 3. add sdev1 into starved_list again. Since starved_list is empty, sdev1 is added at the head
 4. async queue checks sdev2, and add sdev2 into starved_list tail. In this scenario, sdev1 is serviced first next time, so sdev2 is starved. In our test, 12 disks connect to one HBA card. disk's queue depth is 64, while HBA card queue depth is 128. Our test does sync write, so block size is big, so just several requests can occurpy one disk's bandwidth. Saturate one disk but starve others will hurt total throughput.

problem a isn't a big problem in our test (we did observe higher context switch, which is about 4x more CS), but guess it will hurt high end system.

problem b is easy to fix for scsi. just replace blk_run_queue with blk_run_queue_async in scsi_run_queue

problem c is the root cause for the regression. I had a patch for it. Basically with my patch, we don't remove sdev from starved_list in scsi_run_queue, but we delay the removal in scsi_request_fn() till a starved device really dispatches a request. My patch can fully fix the regression.

But given problem a, we should revert the patch (or Alex's patch if stack overflow isn't a big deal here), so I didn't post my patch here. Problem c actually exsists even we revert the patch (we could do async execution with small chance), but not that severe. I can post a fix after the patch is reverted.

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here. :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

Quoted 9 lines · Alex,Shi

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here. :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

Quoted 7 lines

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

This is a problem and we should do something about it for 2.6.39. I knew that there would be cases where the async offload would cause a performance degredation, but not to the extent that you are reporting. Must be hitting the pathological case.

I can think of two scenarios where it could potentially recurse:

- request_fn enter, end up requeuing IO. Run queue at the end. Rinse, repeat.
- Running starved list from request_fn, two (or more) devices could alternately recurse.

The first case should be fairly easy to handle. The second one is already handled by the local list splice.

Looking at the code, is this a real scenario? Only potential recurse I see is:

scsi_request_fn()

        scsi_dispatch_cmd()
                scsi_queue_insert()
                        __scsi_queue_insert()
                                scsi_run_queue()

Why are we even re-running the queue immediately on a BUSY condition? Should only be needed if we have zero pending commands from this particular queue, and for that particular case async run is just fine since it's a rare condition (or performance would suck already).

Quoted 9 lines · Alex,Shi

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here. :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

Quoted 7 lines

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

This is a problem and we should do something about it for 2.6.39. I knew that there would be cases where the async offload would cause a performance degredation, but not to the extent that you are reporting. Must be hitting the pathological case.

I can think of two scenarios where it could potentially recurse:

- request_fn enter, end up requeuing IO. Run queue at the end. Rinse, repeat.
- Running starved list from request_fn, two (or more) devices could alternately recurse.

The first case should be fairly easy to handle. The second one is already handled by the local list splice.

Looking at the code, is this a real scenario? Only potential recurse I see is:

scsi_request_fn()

        scsi_dispatch_cmd()
                scsi_queue_insert()
                        __scsi_queue_insert()
                                scsi_run_queue()

Why are we even re-running the queue immediately on a BUSY condition? Should only be needed if we have zero pending commands from this particular queue, and for that particular case async run is just fine since it's a rare condition (or performance would suck already).

And it should only really be needed for the 'q' being passed in, not the others. Something like the below.

drivers/scsi/scsi_lib.c +1 -1

index 0bac91e..0b01c1f 100644
@@ -74,7 +74,7 @@ struct kmem_cache *scsi_sdb_cache;
  */
 #define SCSI_QUEUE_DELAY	3
 
-static void scsi_run_queue(struct request_queue *q);
+static void scsi_run_queue_async(struct request_queue *q);
 
 /*
  * Function:	scsi_unprep_request()

Quoted 18 lines · Jens Axboe

Quoted 9 lines · Alex,Shi

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here. :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

Quoted 7 lines

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

Quoted 4 lines

This is a problem and we should do something about it for 2.6.39. I knew that there would be cases where the async offload would cause a performance degredation, but not to the extent that you are reporting. Must be hitting the pathological case.

async offload is expected to increase context switch. But the real root cause of the issue is fairness issue. Please see my previous email.

Quoted 9 lines

I can think of two scenarios where it could potentially recurse:

- request_fn enter, end up requeuing IO. Run queue at the end. Rinse, repeat.
- Running starved list from request_fn, two (or more) devices could alternately recurse.

The first case should be fairly easy to handle. The second one is already handled by the local list splice.

this isn't true to me. if you unlock host_lock in scsi_run_queue, other cpus can add sdev to the starved device list again. In the recursive call of scsi_run_queue, the starved device list might not be empty. So the local list_splice doesn't help.

Quoted 35 lines

Looking at the code, is this a real scenario? Only potential recurse I see is:

scsi_request_fn()

        scsi_dispatch_cmd()
                scsi_queue_insert()
                        __scsi_queue_insert()
                                scsi_run_queue()

Why are we even re-running the queue immediately on a BUSY condition? Should only be needed if we have zero pending commands from this particular queue, and for that particular case async run is just fine since it's a rare condition (or performance would suck already).

And it should only really be needed for the 'q' being passed in, not the others. Something like the below.

drivers/scsi/scsi_lib.c +1 -1

index 0bac91e..0b01c1f 100644
@@ -74,7 +74,7 @@ struct kmem_cache *scsi_sdb_cache;
  */
 #define SCSI_QUEUE_DELAY	3
 
-static void scsi_run_queue(struct request_queue *q);
+static void scsi_run_queue_async(struct request_queue *q);
 
 /*
  * Function:	scsi_unprep_request()

@@ -161,7 +161,7 @@ static int __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	blk_requeue_request(q, cmd->request);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 

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

Quoted 2 lines

drivers/scsi/scsi_lib.c +1 -1

-	scsi_run_queue(q);
+	scsi_run_queue_async(q);

so you could still recursivly run into starved list. Do you want to put the whole __scsi_run_queue into workqueue?

Quoted 75 lines · Shaohua Li

Quoted 18 lines · Jens Axboe

Quoted 9 lines · Alex,Shi

commit c21e6beba8835d09bb80e34961 removed the REENTER flag and changed scsi_run_queue() to punt all requests on starved_list devices to kblockd. Yes, like Jens mentioned, the performance on slow SCSI disk was hurt here. :) (Intel SSD isn't effected here)

In our testing on 12 SAS disk JBD, the fio write with sync ioengine drop about 30~40% throughput, fio randread/randwrite with aio ioengine drop about 20%/50% throughput. and fio mmap testing was hurt also.

Quoted 7 lines

With the following debug patch, the performance can be totally recovered in our testing. But without REENTER flag here, in some corner case, like a device is keeping blocked and then unblocked repeatedly, __blk_run_queue() may recursively call scsi_run_queue() and then cause kernel stack overflow.
I don't know details of block device driver, just wondering why on scsi need the REENTER flag here. :)

Quoted 4 lines

This is a problem and we should do something about it for 2.6.39. I knew that there would be cases where the async offload would cause a performance degredation, but not to the extent that you are reporting. Must be hitting the pathological case.

async offload is expected to increase context switch. But the real root cause of the issue is fairness issue. Please see my previous email.

Quoted 9 lines

I can think of two scenarios where it could potentially recurse:

- request_fn enter, end up requeuing IO. Run queue at the end. Rinse, repeat.
- Running starved list from request_fn, two (or more) devices could alternately recurse.

The first case should be fairly easy to handle. The second one is already handled by the local list splice.

this isn't true to me. if you unlock host_lock in scsi_run_queue, other cpus can add sdev to the starved device list again. In the recursive call of scsi_run_queue, the starved device list might not be empty. So the local list_splice doesn't help.

Quoted 35 lines

Looking at the code, is this a real scenario? Only potential recurse I see is:

scsi_request_fn()

        scsi_dispatch_cmd()
                scsi_queue_insert()
                        __scsi_queue_insert()
                                scsi_run_queue()

Why are we even re-running the queue immediately on a BUSY condition? Should only be needed if we have zero pending commands from this particular queue, and for that particular case async run is just fine since it's a rare condition (or performance would suck already).

And it should only really be needed for the 'q' being passed in, not the others. Something like the below.

drivers/scsi/scsi_lib.c +1 -1

index 0bac91e..0b01c1f 100644
@@ -74,7 +74,7 @@ struct kmem_cache *scsi_sdb_cache;
  */
 #define SCSI_QUEUE_DELAY	3
 
-static void scsi_run_queue(struct request_queue *q);
+static void scsi_run_queue_async(struct request_queue *q);
 
 /*
  * Function:	scsi_unprep_request()

@@ -161,7 +161,7 @@ static int __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	blk_requeue_request(q, cmd->request);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 

Quoted 4 lines

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

Quoted 2 lines

drivers/scsi/scsi_lib.c +1 -1

-	scsi_run_queue(q);
+	scsi_run_queue_async(q);

so you could still recursivly run into starved list. Do you want to put the whole __scsi_run_queue into workqueue?

what I mean is current sdev (other devices too) can still be added into starved list, so only does async execute for current q isn't enough, we'd better put whole __scsi_run_queue into workqueue. something like below on top of yours, untested. Not sure if there are other recursive cases.

Quoted 6 lines

what I mean is current sdev (other devices too) can still be added into starved list, so only does async execute for current q isn't enough, we'd better put whole __scsi_run_queue into workqueue. something like below on top of yours, untested. Not sure if there are other recursive cases.

verified the regression can be fully fixed by your patch (with my suggested fix to avoid race). Can we put a formal patch upstream?

Thanks,
Shaohua

Quoted 5 lines

Quoted 5 lines

what I mean is current sdev (other devices too) can still be added into starved list, so only does async execute for current q isn't enough, we'd better put whole __scsi_run_queue into workqueue. something like below on top of yours, untested. Not sure if there are other recursive cases.

Quoted 2 lines

verified the regression can be fully fixed by your patch (with my suggested fix to avoid race). Can we put a formal patch upstream?

Yes, we tested Jens patch alone and plus Shaohua's patch too. Both of them recovered SAS disk performance too. Now I am testing them on SSD disk with kbuild and fio cases, In theory, they will both work.

Quoted 4 lines

Thanks,
Shaohua

Quoted 8 lines · Alex,Shi

Quoted 7 lines

Quoted 5 lines

what I mean is current sdev (other devices too) can still be added into starved list, so only does async execute for current q isn't enough, we'd better put whole __scsi_run_queue into workqueue. something like below on top of yours, untested. Not sure if there are other recursive cases.

verified the regression can be fully fixed by your patch (with my suggested fix to avoid race). Can we put a formal patch upstream?

Quoted 4 lines

Yes, we tested Jens patch alone and plus Shaohua's patch too. Both of them recovered SAS disk performance too. Now I am testing them on SSD disk with kbuild and fio cases, In theory, they will both work.

As expected, both patches has no effect on SSD disks in JBD.

I will queue up the combined patch, it looks fine from here as well.

Quoted 2 lines

I will queue up the combined patch, it looks fine from here as well.

When I have some time to study Jens and shaohua's patch today. I find a simpler way to resolved the re-enter issue on starved_list. Following Jens' idea, we can just put the starved_list device into kblockd if it come from __scsi_queue_insert().
It can resolve the re-enter issue and recover performance totally, and need not a work_struct in every scsi_device. The logic/code also looks a bit simpler.
What's your opinion of this?

Signed-off-by Alex Shi <[email protected]>

---

drivers/scsi/scsi_lib.c +23 -16

index ec1803a..de7c569 100644
@@ -74,6 +74,8 @@ struct kmem_cache *scsi_sdb_cache;
  */
 #define SCSI_QUEUE_DELAY	3
 
+static void scsi_run_queue_async(struct request_queue *q);
+
 /*
  * Function:	scsi_unprep_request()
  *

@@ -159,7 +161,7 @@ static int __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 	blk_requeue_request(q, cmd->request);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-	kblockd_schedule_work(q, &device->requeue_work);
+	scsi_run_queue_async(q);
 
 	return 0;
 }

@@ -395,7 +397,7 @@ static inline int scsi_host_is_busy(struct Scsi_Host *shost)
  * Notes:	The previous command was completely finished, start
  *		a new one if possible.
  */
-static void scsi_run_queue(struct request_queue *q)
+static void __scsi_run_queue(struct request_queue *q, bool async)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;

@@ -435,30 +437,35 @@ static void scsi_run_queue(struct request_queue *q)
 				       &shost->starved_list);
 			continue;
 		}
-
-		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);
-		spin_lock(shost->host_lock);
+		if (async)
+			blk_run_queue_async(sdev->request_queue);
+		else {
+			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);
+			spin_lock(shost->host_lock);
+		}
 	}
 	/* put any unprocessed entries back */
 	list_splice(&starved_list, &shost->starved_list);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	blk_run_queue(q);
+	if (async)
+		blk_run_queue_async(q);
+	else
+		blk_run_queue(q);
 }
 
-void scsi_requeue_run_queue(struct work_struct *work)
+static void scsi_run_queue(struct request_queue *q)
 {
-	struct scsi_device *sdev;
-	struct request_queue *q;
-
-	sdev = container_of(work, struct scsi_device, requeue_work);
-	q = sdev->request_queue;
-	scsi_run_queue(q);
+	__scsi_run_queue(q, false);
 }
 
+static void scsi_run_queue_async(struct request_queue *q)
+{
+	__scsi_run_queue(q, true);
+}
 /*
  * Function:	scsi_requeue_command()
  *

drivers/scsi/scsi_scan.c +0 -2

index 58584dc..087821f 100644
@@ -242,7 +242,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	int display_failure_msg = 1, ret;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	extern void scsi_evt_thread(struct work_struct *work);
-	extern void scsi_requeue_run_queue(struct work_struct *work);
 
 	sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
 		       GFP_ATOMIC);

@@ -265,7 +264,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
 	INIT_WORK(&sdev->event_work, scsi_evt_thread);
-	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
 	sdev->sdev_target = starget;

include/scsi/scsi_device.h +0 -1

index dd82e02..2d3ec50 100644
@@ -169,7 +169,6 @@ struct scsi_device {
 				sdev_dev;
 
 	struct execute_work	ew; /* used to get process context on put */
-	struct work_struct	requeue_work;
 
 	struct scsi_dh_data	*scsi_dh_data;
 	enum scsi_device_state sdev_state;

Quoted 3 lines

I will queue up the combined patch, it looks fine from here as well.

When I have some time to study Jens and shaohua's patch today. I find a simpler way to resolved the re-enter issue on starved_list. Following Jens' idea, we can just put the starved_list device into kblockd if it come from __scsi_queue_insert().
It can resolve the re-enter issue and recover performance totally, and need not a work_struct in every scsi_device. The logic/code also looks a bit simpler.
What's your opinion of this?

Quoted 4 lines · Alex,Shi

Quoted 3 lines

I will queue up the combined patch, it looks fine from here as well.

Quoted 8 lines

When I have some time to study Jens and shaohua's patch today. I find a simpler way to resolved the re-enter issue on starved_list. Following Jens' idea, we can just put the starved_list device into kblockd if it come from __scsi_queue_insert().
It can resolve the re-enter issue and recover performance totally, and need not a work_struct in every scsi_device. The logic/code also looks a bit simpler.
What's your opinion of this?

Isn't this _identical_ to my original patch, with the added async run of the queue passed in (which is important, an oversight)?

Quoted 14 lines · Alex,Shi

Quoted 4 lines

Quoted 3 lines

I will queue up the combined patch, it looks fine from here as well.

Quoted 8 lines

When I have some time to study Jens and shaohua's patch today. I find a simpler way to resolved the re-enter issue on starved_list. Following Jens' idea, we can just put the starved_list device into kblockd if it come from __scsi_queue_insert().
It can resolve the re-enter issue and recover performance totally, and need not a work_struct in every scsi_device. The logic/code also looks a bit simpler.
What's your opinion of this?

Quoted 2 lines

Isn't this _identical_ to my original patch, with the added async run of the queue passed in (which is important, an oversight)?

Not exactly same. It bases on your patch, but added a bypass way for starved_list device. If a starved_list device come from __scsi_queue_insert(), that may caused by our talking recursion, kblockd with take over the process.  Maybe you oversight this point in original patch. :)

The different part from yours is below:
---
static void __scsi_run_queue(struct request_queue *q, bool async) {
       struct scsi_device *sdev = q->queuedata;
       struct Scsi_Host *shost;

drivers/scsi/scsi_lib.c +8 -6

@@ -435,30 +437,35 @@ static void scsi_run_queue(struct request_queue
 *q)
                                       &shost->starved_list);
                        continue;
                }
-
-               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);
-               spin_lock(shost->host_lock);
+               if (async)
+                       blk_run_queue_async(sdev->request_queue);
+               else {
+                       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);
+                       spin_lock(shost->host_lock);

Quoted 1 line

2011/5/20 Alex,Shi <[email protected]>:

Quoted 49 lines · Jens Axboe

Quoted 14 lines · Alex,Shi

Quoted 4 lines

Quoted 3 lines

I will queue up the combined patch, it looks fine from here as well.

Quoted 8 lines

When I have some time to study Jens and shaohua's patch today. I find a simpler way to resolved the re-enter issue on starved_list. Following Jens' idea, we can just put the starved_list device into kblockd if it come from __scsi_queue_insert().
It can resolve the re-enter issue and recover performance totally, and need not a work_struct in every scsi_device. The logic/code also looks a bit simpler.
What's your opinion of this?

Quoted 2 lines

Isn't this _identical_ to my original patch, with the added async run of the queue passed in (which is important, an oversight)?

Not exactly same. It bases on your patch, but added a bypass way for starved_list device. If a starved_list device come from __scsi_queue_insert(), that may caused by our talking recursion, kblockd with take over the process. Maybe you oversight this point in original patch. :)

The different part from yours is below:
---
static void __scsi_run_queue(struct request_queue *q, bool async)

 {
       struct scsi_device *sdev = q->queuedata;
       struct Scsi_Host *shost;

drivers/scsi/scsi_lib.c +8 -6

@@ -435,30 +437,35 @@ static void scsi_run_queue(struct request_queue
 *q)
                                      &shost->starved_list);
                       continue;
               }
-
-               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);
-               spin_lock(shost->host_lock);
+               if (async)
+                       blk_run_queue_async(sdev->request_queue);
+               else {
+                       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);
+                       spin_lock(shost->host_lock);

Quoted 1 line

Quoted 1 line

I don't quite like this approach. blk_run_queue_async() could introduce fairness issue as I said in previous mail, because we drop the sdev from starved list but didn't run its queue immediately. The issue exists before, but it's a bug to me.