[PATCH] Remove sync waiting code from libata

[PATCH] Remove sync waiting code from libata

[Matthew Wilcox]

By using the scsi async probing code, we can remove the 'sync' argument
from ata_scsi_scan_host():

diff -u b/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
--- b/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6362,10 +6362,15 @@
 	}
 }
 
+/*
+ * Give ourselves ten seconds to find everything.
+ * This should be more than enough time
+ */
 int ata_scsi_scan_finished(struct Scsi_Host *shost, unsigned long time)
 {
-	ata_scsi_scan_host(ata_shost_to_port(shost), 1);
-	return 1;
+	if (ata_scsi_scan_host(ata_shost_to_port(shost)))
+		return 1;
+	return time > (10 * HZ);
 }
 
 /**
diff -u b/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
--- b/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2954,17 +2954,15 @@
 	return rc;
 }
 
-void ata_scsi_scan_host(struct ata_port *ap, int sync)
+/* Returns 1 if we are done scanning, 0 if we've scheduled async scanning */
+int ata_scsi_scan_host(struct ata_port *ap)
 {
-	int tries = 5;
-	struct ata_device *last_failed_dev = NULL;
 	struct ata_device *dev;
 	unsigned int i;
 
 	if (ap->flags & ATA_FLAG_DISABLED)
-		return;
+		return 1;
 
- repeat:
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct scsi_device *sdev;
 
@@ -2990,34 +2988,11 @@
 			break;
 	}
 	if (i == ATA_MAX_DEVICES)
-		return;
-
-	/* we're missing some SCSI devices */
-	if (sync) {
-		/* If caller requested synchrnous scan && we've made
-		 * any progress, sleep briefly and repeat.
-		 */
-		if (dev != last_failed_dev) {
-			msleep(100);
-			last_failed_dev = dev;
-			goto repeat;
-		}
-
-		/* We might be failing to detect boot device, give it
-		 * a few more chances.
-		 */
-		if (--tries) {
-			msleep(100);
-			goto repeat;
-		}
-
-		ata_port_printk(ap, KERN_ERR, "WARNING: synchronous SCSI scan "
-				"failed without making any progress,\n"
-				"                  switching to async\n");
-	}
+		return 1;
 
 	queue_delayed_work(ata_aux_wq, &ap->hotplug_task,
 			   round_jiffies_relative(HZ));
+	return 0;
 }
 
 /**
@@ -3144,7 +3119,7 @@
 	}
 
 	/* scan for new ones */
-	ata_scsi_scan_host(ap, 0);
+	ata_scsi_scan_host(ap);
 
 	DPRINTK("EXIT\n");
 }
only in patch2:
unchanged:
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -112,7 +112,7 @@ static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
 /* libata-scsi.c */
 extern int ata_scsi_add_hosts(struct ata_host *host,
 			      struct scsi_host_template *sht);
-extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
+extern int ata_scsi_scan_host(struct ata_port *ap);
 extern int ata_scsi_offline_dev(struct ata_device *dev);
 extern void ata_scsi_hotplug(struct work_struct *work);
 extern unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Remove sync waiting code from libata

[Tejun Heo]

Matthew Wilcox wrote:
> By using the scsi async probing code, we can remove the 'sync' argument
> from ata_scsi_scan_host():

Hmmm... How so?  @sync is there to keep device numbering stable even
when SCSI scan fails due to allocation failure.  I don't see how async
probing changes that.

-- 
tejun

Re: [PATCH] Remove sync waiting code from libata

[Matthew Wilcox]

On Sat, Sep 08, 2007 at 05:14:21PM +0900, Tejun Heo wrote:
> Matthew Wilcox wrote:
> > By using the scsi async probing code, we can remove the 'sync' argument
> > from ata_scsi_scan_host():
> 
> Hmmm... How so?  @sync is there to keep device numbering stable even
> when SCSI scan fails due to allocation failure.  I don't see how async
> probing changes that.

async probing also keeps device numbering stable.  As long as the device
responds within ten seconds (and the current code has half a second as
the timeout), it'll get the same number it would have had, even though
other hosts have successfully completed their probes during that time.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Remove sync waiting code from libata

[Tejun Heo]

Matthew Wilcox wrote:
> On Sat, Sep 08, 2007 at 05:14:21PM +0900, Tejun Heo wrote:
>> Matthew Wilcox wrote:
>>> By using the scsi async probing code, we can remove the 'sync' argument
>>> from ata_scsi_scan_host():
>> Hmmm... How so?  @sync is there to keep device numbering stable even
>> when SCSI scan fails due to allocation failure.  I don't see how async
>> probing changes that.
> 
> async probing also keeps device numbering stable.  As long as the device
> responds within ten seconds (and the current code has half a second as
> the timeout), it'll get the same number it would have had, even though
> other hosts have successfully completed their probes during that time.

The @sync parameter is to work around GFP_ATOMIC allocation in SCSI scan
code.  A libata SCSI host can have upto 15 devices with PMP and with
multiple devices allocation failure is not so rare, probably because
libata SCSI scanning is done back-to-back in rapid succession.

The proper fix is probably to make SCSI scanning code not use GFP_ATOMIC
(there doesn't seem to be any reason to) but @sync is the bandaid till
that happens.

Thanks.

-- 
tejun

Re: [PATCH] Remove sync waiting code from libata

[Alan Cox]

> async probing also keeps device numbering stable.  As long as the device
> responds within ten seconds (and the current code has half a second as
> the timeout), it'll get the same number it would have had, even though
> other hosts have successfully completed their probes during that time.

The usual probe time for a device which is spun down is nearer 30 seconds.

Re: [PATCH] Remove sync waiting code from libata

[Tejun Heo]

Alan Cox wrote:
>> async probing also keeps device numbering stable.  As long as the device
>> responds within ten seconds (and the current code has half a second as
>> the timeout), it'll get the same number it would have had, even though
>> other hosts have successfully completed their probes during that time.
> 
> The usual probe time for a device which is spun down is nearer 30 seconds.

libata probing is done differently tho.  It goes like...

1. libata EH probes all hardware.  Things can take quite long here but
SCSI isn't really involved.

2. After libata EH is finished, SCSI host scan is invoked.  It doesn't
do much.  When SCSI probing command reaches libata-scsi, it just fakes
the replies from the information gathered in #1 - No actual ATA reset or
command is issued.  If there are multiple devices on a host (ATA port),
this rapid probing makes SCSI host scan code fail GFP_ATOMIC allocation
from time to time.  This is why the @sync hack was used.

Thanks.

-- 
tejun