Patchwork [1/4] libsas: flush initial device discovery before completing ->scan_finished()

login
register
mail settings
Submitter Dan Williams
Date May 24, 2011, 8:17 p.m.
Message ID <20110524201748.7351.48220.stgit@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/97215/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Dan Williams - May 24, 2011, 8:17 p.m.
During initial scan libsas drivers start their phys and notify libsas
with PORTE_BYTES_DMAED events as port links are established.  This
notification in turn causes libsas to post DISCE_DISCOVER_DOMAIN events
to the queue.  Calling scsi_flush_work() at the end of scan_finished
guarantees that all preceding PORTE_BYTES_DMAED events have been
registered in the queue, but it does not guarantee that the resulting
DISCE_DISCOVER_DOMAIN events have been processed because
flush_workqueue() explicitly avoids live-locking with incoming work.

Introduce sas_flush_discovery() to guarantee that all initial discovery
events have completed.  It is called after the driver determines all
initial PORTE_BYTES_DMAED events have had a chance to enter the queue.
This does not cover BCNs that are generated during expander bring up,
only the initial sas_discover_domain() event.

Cc: David Milburn <dmilburn@redhat.com>
Cc: Srinivas <satyasrinivasp@hcl.in>
Cc: jack_wang@usish.com
Cc: lindar_liu@usish.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/aic94xx/aic94xx_init.c |    2 +-
 drivers/scsi/mvsas/mv_sas.c         |    2 +-
 drivers/scsi/pm8001/pm8001_sas.c    |    2 +-
 include/scsi/libsas.h               |   13 +++++++++++++
 4 files changed, 16 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - May 28, 2011, 12:41 a.m.
On Tue, May 24, 2011 at 1:17 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> During initial scan libsas drivers start their phys and notify libsas
> with PORTE_BYTES_DMAED events as port links are established.  This
> notification in turn causes libsas to post DISCE_DISCOVER_DOMAIN events
> to the queue.  Calling scsi_flush_work() at the end of scan_finished
> guarantees that all preceding PORTE_BYTES_DMAED events have been
> registered in the queue, but it does not guarantee that the resulting
> DISCE_DISCOVER_DOMAIN events have been processed because
> flush_workqueue() explicitly avoids live-locking with incoming work.
>
> Introduce sas_flush_discovery() to guarantee that all initial discovery
> events have completed.  It is called after the driver determines all
> initial PORTE_BYTES_DMAED events have had a chance to enter the queue.
> This does not cover BCNs that are generated during expander bring up,
> only the initial sas_discover_domain() event.

I notice that scsi-misc picked up all the patches from the series,
save this one.

Oversight, or nak?

Note that latest upstream still says the following for flush_workqueue():

 * We sleep until all works which were queued on entry have been handled,
 * but we are not livelocked by new incoming ones.

...so isci will continue to open code this double flush.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - May 28, 2011, 3:14 a.m.
On Fri, 2011-05-27 at 17:41 -0700, Dan Williams wrote:
> On Tue, May 24, 2011 at 1:17 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > During initial scan libsas drivers start their phys and notify libsas
> > with PORTE_BYTES_DMAED events as port links are established.  This
> > notification in turn causes libsas to post DISCE_DISCOVER_DOMAIN events
> > to the queue.  Calling scsi_flush_work() at the end of scan_finished
> > guarantees that all preceding PORTE_BYTES_DMAED events have been
> > registered in the queue, but it does not guarantee that the resulting
> > DISCE_DISCOVER_DOMAIN events have been processed because
> > flush_workqueue() explicitly avoids live-locking with incoming work.
> >
> > Introduce sas_flush_discovery() to guarantee that all initial discovery
> > events have completed.  It is called after the driver determines all
> > initial PORTE_BYTES_DMAED events have had a chance to enter the queue.
> > This does not cover BCNs that are generated during expander bring up,
> > only the initial sas_discover_domain() event.
> 
> I notice that scsi-misc picked up all the patches from the series,
> save this one.
> 
> Oversight, or nak?

Neither ... as I've said several times before, I want to use tejun's
drain infrastructure for this.

James



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - May 28, 2011, 6:33 a.m.
On Fri, May 27, 2011 at 8:14 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2011-05-27 at 17:41 -0700, Dan Williams wrote:
>> On Tue, May 24, 2011 at 1:17 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> > During initial scan libsas drivers start their phys and notify libsas
>> > with PORTE_BYTES_DMAED events as port links are established.  This
>> > notification in turn causes libsas to post DISCE_DISCOVER_DOMAIN events
>> > to the queue.  Calling scsi_flush_work() at the end of scan_finished
>> > guarantees that all preceding PORTE_BYTES_DMAED events have been
>> > registered in the queue, but it does not guarantee that the resulting
>> > DISCE_DISCOVER_DOMAIN events have been processed because
>> > flush_workqueue() explicitly avoids live-locking with incoming work.
>> >
>> > Introduce sas_flush_discovery() to guarantee that all initial discovery
>> > events have completed.  It is called after the driver determines all
>> > initial PORTE_BYTES_DMAED events have had a chance to enter the queue.
>> > This does not cover BCNs that are generated during expander bring up,
>> > only the initial sas_discover_domain() event.
>>
>> I notice that scsi-misc picked up all the patches from the series,
>> save this one.
>>
>> Oversight, or nak?
>
> Neither ... as I've said several times before, I want to use tejun's
> drain infrastructure for this.
>

Apologies, for the disconnect.

Found the drain_workqueue thread...

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 3b7e83d..f763daa 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -972,7 +972,7 @@  static int asd_scan_finished(struct Scsi_Host *shost, unsigned long time)
 	if (time < HZ)
 		return 0;
 	/* Wait for discovery to finish */
-	scsi_flush_work(shost);
+	sas_flush_discovery(shost);
 	return 1;
 }
 
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index adedaa9..e34c378 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -440,7 +440,7 @@  int mvs_scan_finished(struct Scsi_Host *shost, unsigned long time)
 	if (time < HZ)
 		return 0;
 	/* Wait for discovery to finish */
-	scsi_flush_work(shost);
+	sas_flush_discovery(shost);
 	return 1;
 }
 
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 6ae059e..997cf59 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -253,7 +253,7 @@  int pm8001_scan_finished(struct Scsi_Host *shost, unsigned long time)
 	if (time < HZ)
 		return 0;
 	/* Wait for discovery to finish */
-	scsi_flush_work(shost);
+	sas_flush_discovery(shost);
 	return 1;
 }
 
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 8f6bb9c..d603b26e 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -405,6 +405,19 @@  static inline void sas_phy_disconnected(struct asd_sas_phy *phy)
 	phy->linkrate = SAS_LINK_RATE_UNKNOWN;
 }
 
+/* Before returning from ->scan_finished() an LLDD calls this routine to
+ * ensure that all port notifications have been promoted to domain
+ * discovery events, and that initial domain discovery has completed
+ */
+static inline void sas_flush_discovery(struct Scsi_Host *shost)
+{
+	/* flush port events */
+	scsi_flush_work(shost);
+
+	/* flush domain discovery events queued by the port events */
+	scsi_flush_work(shost);
+}
+
 /* ---------- Tasks ---------- */
 /*
       service_response |  SAS_TASK_COMPLETE  |  SAS_TASK_UNDELIVERED |