[PATCH] SCSI: add check for host busy in scsi_run_queue()

[PATCH] SCSI: add check for host busy in scsi_run_queue()

[Hillf Danton]

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);

Re: [PATCH] SCSI: add check for host busy in scsi_run_queue()

[James Bottomley]

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

Re: [PATCH] SCSI: add check for host busy in scsi_run_queue()

[Hillf Danton]

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