On Tue, Sep 21, 2010 at 1:56 AM, James Bottomley <James.Bottomley@xxxxxxx> 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@xxxxxxxxx> >> --- >> >> --- 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html