* [PATCH] SCSI: add check for host busy in scsi_run_queue()
@ 2010-09-19 12:53 Hillf Danton
2010-09-20 17:56 ` James Bottomley
0 siblings, 1 reply; 3+ messages in thread
From: Hillf Danton @ 2010-09-19 12:53 UTC (permalink / raw)
To: linux-scsi, Mike Christie, FUJITA Tomonori, James E.J. Bottomley
It seems that check for host busy is deserved before scanning the starved list.
And list operation is simplified.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
--- o/linux-2.6.36-rc4/drivers/scsi/scsi_lib.c 2010-09-13
07:07:38.000000000 +0800
+++ m/linux-2.6.36-rc4/drivers/scsi/scsi_lib.c 2010-09-19
20:32:34.000000000 +0800
@@ -399,18 +399,20 @@ static inline int scsi_host_is_busy(stru
*/
static void scsi_run_queue(struct request_queue *q)
{
- struct scsi_device *sdev = q->queuedata;
+ struct scsi_device *sdev = q->queuedata, *safe;
struct Scsi_Host *shost = sdev->host;
- LIST_HEAD(starved_list);
unsigned long flags;
if (scsi_target(sdev)->single_lun)
scsi_single_lun_run(sdev);
spin_lock_irqsave(shost->host_lock, flags);
- list_splice_init(&shost->starved_list, &starved_list);
-
- while (!list_empty(&starved_list)) {
+ if (scsi_host_is_busy(shost)) {
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ return;
+ }
+ list_for_each_entry_safe(sdev, safe, &shost->starved_list,
+ starved_entry) {
int flagset;
/*
@@ -426,14 +428,10 @@ static void scsi_run_queue(struct reques
if (scsi_host_is_busy(shost))
break;
- sdev = list_entry(starved_list.next,
- struct scsi_device, starved_entry);
- list_del_init(&sdev->starved_entry);
- if (scsi_target_is_busy(scsi_target(sdev))) {
- list_move_tail(&sdev->starved_entry,
- &shost->starved_list);
+ if (scsi_target_is_busy(scsi_target(sdev)))
continue;
- }
+
+ list_del_init(&sdev->starved_entry);
spin_unlock(shost->host_lock);
@@ -450,8 +448,6 @@ static void scsi_run_queue(struct reques
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);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] SCSI: add check for host busy in scsi_run_queue()
2010-09-19 12:53 [PATCH] SCSI: add check for host busy in scsi_run_queue() Hillf Danton
@ 2010-09-20 17:56 ` James Bottomley
2010-09-21 15:06 ` Hillf Danton
0 siblings, 1 reply; 3+ messages in thread
From: James Bottomley @ 2010-09-20 17:56 UTC (permalink / raw)
To: Hillf Danton; +Cc: linux-scsi, Mike Christie, FUJITA Tomonori
On Sun, 2010-09-19 at 20:53 +0800, Hillf Danton wrote:
> It seems that check for host busy is deserved before scanning the starved list.
> And list operation is simplified.
>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>
> --- o/linux-2.6.36-rc4/drivers/scsi/scsi_lib.c 2010-09-13
> 07:07:38.000000000 +0800
> +++ m/linux-2.6.36-rc4/drivers/scsi/scsi_lib.c 2010-09-19
> 20:32:34.000000000 +0800
> @@ -399,18 +399,20 @@ static inline int scsi_host_is_busy(stru
> */
> static void scsi_run_queue(struct request_queue *q)
> {
> - struct scsi_device *sdev = q->queuedata;
> + struct scsi_device *sdev = q->queuedata, *safe;
> struct Scsi_Host *shost = sdev->host;
> - LIST_HEAD(starved_list);
> unsigned long flags;
>
> if (scsi_target(sdev)->single_lun)
> scsi_single_lun_run(sdev);
>
> spin_lock_irqsave(shost->host_lock, flags);
> - list_splice_init(&shost->starved_list, &starved_list);
> -
> - while (!list_empty(&starved_list)) {
> + if (scsi_host_is_busy(shost)) {
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + return;
> + }
To be honest, there's no real gain from this micro-optimisation.
> + list_for_each_entry_safe(sdev, safe, &shost->starved_list,
> + starved_entry) {
And this is definitely wrong. The list is altered in flight so the
whole purpose of the static copy is to process only what existed before
we began; otherwise you can get a livelock.
James
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] SCSI: add check for host busy in scsi_run_queue()
2010-09-20 17:56 ` James Bottomley
@ 2010-09-21 15:06 ` Hillf Danton
0 siblings, 0 replies; 3+ messages in thread
From: Hillf Danton @ 2010-09-21 15:06 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, Mike Christie, FUJITA Tomonori
On Tue, Sep 21, 2010 at 1:56 AM, James Bottomley
<James.Bottomley@suse.de> wrote:
> On Sun, 2010-09-19 at 20:53 +0800, Hillf Danton wrote:
>> It seems that check for host busy is deserved before scanning the starved list.
>> And list operation is simplified.
>>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- o/linux-2.6.36-rc4/drivers/scsi/scsi_lib.c 2010-09-13
>> 07:07:38.000000000 +0800
>> +++ m/linux-2.6.36-rc4/drivers/scsi/scsi_lib.c 2010-09-19
>> 20:32:34.000000000 +0800
>> @@ -399,18 +399,20 @@ static inline int scsi_host_is_busy(stru
>> */
>> static void scsi_run_queue(struct request_queue *q)
>> {
>> - struct scsi_device *sdev = q->queuedata;
>> + struct scsi_device *sdev = q->queuedata, *safe;
>> struct Scsi_Host *shost = sdev->host;
>> - LIST_HEAD(starved_list);
>> unsigned long flags;
>>
>> if (scsi_target(sdev)->single_lun)
>> scsi_single_lun_run(sdev);
>>
>> spin_lock_irqsave(shost->host_lock, flags);
>> - list_splice_init(&shost->starved_list, &starved_list);
>> -
>> - while (!list_empty(&starved_list)) {
>> + if (scsi_host_is_busy(shost)) {
>> + spin_unlock_irqrestore(shost->host_lock, flags);
>> + return;
>> + }
>
> To be honest, there's no real gain from this micro-optimisation.
>
>> + list_for_each_entry_safe(sdev, safe, &shost->starved_list,
>> + starved_entry) {
>
> And this is definitely wrong. The list is altered in flight so the
> whole purpose of the static copy is to process only what existed before
> we began; otherwise you can get a livelock.
>
node. Then the potential livelock could be fixed by
list_for_each_entry_safe_reverse().
> James
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-09-21 15:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-19 12:53 [PATCH] SCSI: add check for host busy in scsi_run_queue() Hillf Danton
2010-09-20 17:56 ` James Bottomley
2010-09-21 15:06 ` Hillf Danton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox