| Message ID | 1499670369-44143-5-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 07/10/2017 09:06 AM, Yijing Wang wrote: > Introduce wait-complete for libsas sas event processing, > execute sas port create/destruct in sync. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > CC: John Garry <john.garry@huawei.com> > CC: Johannes Thumshirn <jthumshirn@suse.de> > CC: Ewan Milne <emilne@redhat.com> > CC: Christoph Hellwig <hch@lst.de> > CC: Tomas Henzl <thenzl@redhat.com> > CC: Dan Williams <dan.j.williams@intel.com> > --- > drivers/scsi/libsas/sas_discover.c | 41 ++++++++++++++++++++++++++++---------- > drivers/scsi/libsas/sas_internal.h | 34 +++++++++++++++++++++++++++++++ > drivers/scsi/libsas/sas_port.c | 4 ++++ > include/scsi/libsas.h | 5 ++++- > 4 files changed, 72 insertions(+), 12 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c > index 60de662..5d4a3a8 100644 > --- a/drivers/scsi/libsas/sas_discover.c > +++ b/drivers/scsi/libsas/sas_discover.c > @@ -525,16 +525,43 @@ static void sas_revalidate_domain(struct work_struct *work) > mutex_unlock(&ha->disco_mutex); > } > > +static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { > + [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, > + [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, > + [DISCE_PROBE] = sas_probe_devices, > + [DISCE_SUSPEND] = sas_suspend_devices, > + [DISCE_RESUME] = sas_resume_devices, > + [DISCE_DESTRUCT] = sas_destruct_devices, > +}; > + > +/* a simple wrapper for sas discover event funtions */ > +static void sas_discover_common_fn(struct work_struct *work) > +{ > + struct sas_discovery_event *ev = to_sas_discovery_event(work); > + struct asd_sas_port *port = ev->port; > + > + sas_event_fns[ev->type](work); > + sas_port_put(port); > +} > + > + > /* ---------- Events ---------- */ > > static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw) > { > + int ret; > + struct sas_discovery_event *ev = to_sas_discovery_event(&sw->work); > + struct asd_sas_port *port = ev->port; > + > /* chained work is not subject to SA_HA_DRAINING or > * SAS_HA_REGISTERED, because it is either submitted in the > * workqueue, or known to be submitted from a context that is > * not racing against draining > */ > - scsi_queue_work(ha->core.shost, &sw->work); > + sas_port_get(port); > + ret = scsi_queue_work(ha->core.shost, &sw->work); > + if (ret != 1) > + sas_port_put(port); > } > > static void sas_chain_event(int event, unsigned long *pending, > @@ -575,18 +602,10 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port) > { > int i; > > - static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { > - [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, > - [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, > - [DISCE_PROBE] = sas_probe_devices, > - [DISCE_SUSPEND] = sas_suspend_devices, > - [DISCE_RESUME] = sas_resume_devices, > - [DISCE_DESTRUCT] = sas_destruct_devices, > - }; > - > disc->pending = 0; > for (i = 0; i < DISC_NUM_EVENTS; i++) { > - INIT_SAS_WORK(&disc->disc_work[i].work, sas_event_fns[i]); > + INIT_SAS_WORK(&disc->disc_work[i].work, sas_discover_common_fn); > disc->disc_work[i].port = port; > + disc->disc_work[i].type = i; > } > } > diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h > index f03ce64..890b5d26 100644 > --- a/drivers/scsi/libsas/sas_internal.h > +++ b/drivers/scsi/libsas/sas_internal.h > @@ -100,6 +100,40 @@ void sas_free_device(struct kref *kref); > extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS]; > extern const work_func_t sas_port_event_fns[PORT_NUM_EVENTS]; > > +static void sas_complete_event(struct kref *kref) > +{ > + struct asd_sas_port *port = container_of(kref, struct asd_sas_port, ref); > + > + complete_all(&port->completion); > +} > + > +static inline void sas_port_put(struct asd_sas_port *port) > +{ > + if (port->is_sync) > + kref_put(&port->ref, sas_complete_event); > +} > + > +static inline void sas_port_wait_init(struct asd_sas_port *port) > +{ > + init_completion(&port->completion); > + kref_init(&port->ref); > + port->is_sync = true; > +} > + > +static inline void sas_port_wait_completion( > + struct asd_sas_port *port) > +{ > + sas_port_put(port); > + wait_for_completion(&port->completion); > + port->is_sync = false; > +} > + > +static inline void sas_port_get(struct asd_sas_port *port) > +{ > + if (port && port->is_sync) > + kref_get(&port->ref); > +} > + > #ifdef CONFIG_SCSI_SAS_HOST_SMP > extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req, > struct request *rsp); > diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c > index 9326628..d589adb 100644 > --- a/drivers/scsi/libsas/sas_port.c > +++ b/drivers/scsi/libsas/sas_port.c > @@ -191,7 +191,9 @@ static void sas_form_port(struct asd_sas_phy *phy) > if (si->dft->lldd_port_formed) > si->dft->lldd_port_formed(phy); > > + sas_port_wait_init(port); > sas_discover_event(phy->port, DISCE_DISCOVER_DOMAIN); > + sas_port_wait_completion(port); > } > > /** > @@ -218,7 +220,9 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) > dev->pathways--; > > if (port->num_phys == 1) { > + sas_port_wait_init(port); > sas_unregister_domain_devices(port, gone); > + sas_port_wait_completion(port); > sas_port_delete(port->port); > port->port = NULL; > } else { I would rather use the standard on-stack completion here; like this: DECLARE_COMPLETION_ONSTACK(complete); port->completion = &complete; sas_unregister_domain_devices(port, gone); wait_for_completion(&complete); sas_port_delete(port->port); which would simplify the above helpers to: static inline void sas_port_put(struct asd_sas_port *port) { if (port->completion) kref_put(&port->ref, sas_complete_event); } and you could do away with the 'is_sync' helper. Cheers, Hannes
在 2017/7/14 14:51, Hannes Reinecke 写道: > On 07/10/2017 09:06 AM, Yijing Wang wrote: >> Introduce wait-complete for libsas sas event processing, >> execute sas port create/destruct in sync. >> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> CC: John Garry <john.garry@huawei.com> >> CC: Johannes Thumshirn <jthumshirn@suse.de> >> CC: Ewan Milne <emilne@redhat.com> >> CC: Christoph Hellwig <hch@lst.de> >> CC: Tomas Henzl <thenzl@redhat.com> >> CC: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/scsi/libsas/sas_discover.c | 41 ++++++++++++++++++++++++++++---------- >> drivers/scsi/libsas/sas_internal.h | 34 +++++++++++++++++++++++++++++++ >> drivers/scsi/libsas/sas_port.c | 4 ++++ >> include/scsi/libsas.h | 5 ++++- >> 4 files changed, 72 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c >> index 60de662..5d4a3a8 100644 >> --- a/drivers/scsi/libsas/sas_discover.c >> +++ b/drivers/scsi/libsas/sas_discover.c >> @@ -525,16 +525,43 @@ static void sas_revalidate_domain(struct work_struct *work) >> mutex_unlock(&ha->disco_mutex); >> } >> >> +static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { >> + [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, >> + [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, >> + [DISCE_PROBE] = sas_probe_devices, >> + [DISCE_SUSPEND] = sas_suspend_devices, >> + [DISCE_RESUME] = sas_resume_devices, >> + [DISCE_DESTRUCT] = sas_destruct_devices, >> +}; >> + >> +/* a simple wrapper for sas discover event funtions */ >> +static void sas_discover_common_fn(struct work_struct *work) >> +{ >> + struct sas_discovery_event *ev = to_sas_discovery_event(work); >> + struct asd_sas_port *port = ev->port; >> + >> + sas_event_fns[ev->type](work); >> + sas_port_put(port); >> +} >> + >> + >> /* ---------- Events ---------- */ >> >> static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw) >> { >> + int ret; >> + struct sas_discovery_event *ev = to_sas_discovery_event(&sw->work); >> + struct asd_sas_port *port = ev->port; >> + >> /* chained work is not subject to SA_HA_DRAINING or >> * SAS_HA_REGISTERED, because it is either submitted in the >> * workqueue, or known to be submitted from a context that is >> * not racing against draining >> */ >> - scsi_queue_work(ha->core.shost, &sw->work); >> + sas_port_get(port); >> + ret = scsi_queue_work(ha->core.shost, &sw->work); >> + if (ret != 1) >> + sas_port_put(port); >> } >> >> static void sas_chain_event(int event, unsigned long *pending, >> @@ -575,18 +602,10 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port) >> { >> int i; >> >> - static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { >> - [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, >> - [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, >> - [DISCE_PROBE] = sas_probe_devices, >> - [DISCE_SUSPEND] = sas_suspend_devices, >> - [DISCE_RESUME] = sas_resume_devices, >> - [DISCE_DESTRUCT] = sas_destruct_devices, >> - }; >> - >> disc->pending = 0; >> for (i = 0; i < DISC_NUM_EVENTS; i++) { >> - INIT_SAS_WORK(&disc->disc_work[i].work, sas_event_fns[i]); >> + INIT_SAS_WORK(&disc->disc_work[i].work, sas_discover_common_fn); >> disc->disc_work[i].port = port; >> + disc->disc_work[i].type = i; >> } >> } >> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h >> index f03ce64..890b5d26 100644 >> --- a/drivers/scsi/libsas/sas_internal.h >> +++ b/drivers/scsi/libsas/sas_internal.h >> @@ -100,6 +100,40 @@ void sas_free_device(struct kref *kref); >> extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS]; >> extern const work_func_t sas_port_event_fns[PORT_NUM_EVENTS]; >> >> +static void sas_complete_event(struct kref *kref) >> +{ >> + struct asd_sas_port *port = container_of(kref, struct asd_sas_port, ref); >> + >> + complete_all(&port->completion); >> +} >> + >> +static inline void sas_port_put(struct asd_sas_port *port) >> +{ >> + if (port->is_sync) >> + kref_put(&port->ref, sas_complete_event); >> +} >> + >> +static inline void sas_port_wait_init(struct asd_sas_port *port) >> +{ >> + init_completion(&port->completion); >> + kref_init(&port->ref); >> + port->is_sync = true; >> +} >> + >> +static inline void sas_port_wait_completion( >> + struct asd_sas_port *port) >> +{ >> + sas_port_put(port); >> + wait_for_completion(&port->completion); >> + port->is_sync = false; >> +} >> + >> +static inline void sas_port_get(struct asd_sas_port *port) >> +{ >> + if (port && port->is_sync) >> + kref_get(&port->ref); >> +} >> + >> #ifdef CONFIG_SCSI_SAS_HOST_SMP >> extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req, >> struct request *rsp); >> diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c >> index 9326628..d589adb 100644 >> --- a/drivers/scsi/libsas/sas_port.c >> +++ b/drivers/scsi/libsas/sas_port.c >> @@ -191,7 +191,9 @@ static void sas_form_port(struct asd_sas_phy *phy) >> if (si->dft->lldd_port_formed) >> si->dft->lldd_port_formed(phy); >> >> + sas_port_wait_init(port); >> sas_discover_event(phy->port, DISCE_DISCOVER_DOMAIN); >> + sas_port_wait_completion(port); >> } >> >> /** >> @@ -218,7 +220,9 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) >> dev->pathways--; >> >> if (port->num_phys == 1) { >> + sas_port_wait_init(port); >> sas_unregister_domain_devices(port, gone); >> + sas_port_wait_completion(port); >> sas_port_delete(port->port); >> port->port = NULL; >> } else { > > I would rather use the standard on-stack completion here; > like this: > > DECLARE_COMPLETION_ONSTACK(complete); > port->completion = &complete; > sas_unregister_domain_devices(port, gone); > wait_for_completion(&complete); > sas_port_delete(port->port); > > which would simplify the above helpers to: > > static inline void sas_port_put(struct asd_sas_port *port) > { > if (port->completion) > kref_put(&port->ref, sas_complete_event); > } > > and you could do away with the 'is_sync' helper. It looks better, thanks! > > Cheers, > > Hannes >
On 14/07/2017 07:51, Hannes Reinecke wrote: >> #ifdef CONFIG_SCSI_SAS_HOST_SMP >> > extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req, >> > struct request *rsp); >> > diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c >> > index 9326628..d589adb 100644 >> > --- a/drivers/scsi/libsas/sas_port.c >> > +++ b/drivers/scsi/libsas/sas_port.c >> > @@ -191,7 +191,9 @@ static void sas_form_port(struct asd_sas_phy *phy) >> > if (si->dft->lldd_port_formed) >> > si->dft->lldd_port_formed(phy); >> > >> > + sas_port_wait_init(port); >> > sas_discover_event(phy->port, DISCE_DISCOVER_DOMAIN); >> > + sas_port_wait_completion(port); >> > } >> > >> > /** >> > @@ -218,7 +220,9 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) >> > dev->pathways--; >> > Hannes thanks for checking. >> > if (port->num_phys == 1) { >> > + sas_port_wait_init(port); >> > sas_unregister_domain_devices(port, gone); >> > + sas_port_wait_completion(port); >> > sas_port_delete(port->port); >> > port->port = NULL; >> > } else { > I would rather use the standard on-stack completion here; > like this: > > DECLARE_COMPLETION_ONSTACK(complete); > port->completion = &complete; > sas_unregister_domain_devices(port, gone); > wait_for_completion(&complete); > sas_port_delete(port->port); > > which would simplify the above helpers to: > > static inline void sas_port_put(struct asd_sas_port *port) > { > if (port->completion) > kref_put(&port->ref, sas_complete_event); > } > > and you could do away with the 'is_sync' helper. > I did wonder if we could avoid using completion altogether and just flush the respective queue which the work item is being processed in. But, due to the intricacy of SCSI/ATA EH, and since we still use shost workqueue for the libsas hotplug processing, maybe it best to keep it straightforward and keep using completions. Anyway, The idea to declare the completion on the stack seems sound. And, for patch 6/7, I don't think the is_sync element is even required without any change to declaration of completion in struct sas_discovery_event. John > Cheers, > > Hannes > --
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c index 60de662..5d4a3a8 100644 --- a/drivers/scsi/libsas/sas_discover.c +++ b/drivers/scsi/libsas/sas_discover.c @@ -525,16 +525,43 @@ static void sas_revalidate_domain(struct work_struct *work) mutex_unlock(&ha->disco_mutex); } +static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { + [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, + [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, + [DISCE_PROBE] = sas_probe_devices, + [DISCE_SUSPEND] = sas_suspend_devices, + [DISCE_RESUME] = sas_resume_devices, + [DISCE_DESTRUCT] = sas_destruct_devices, +}; + +/* a simple wrapper for sas discover event funtions */ +static void sas_discover_common_fn(struct work_struct *work) +{ + struct sas_discovery_event *ev = to_sas_discovery_event(work); + struct asd_sas_port *port = ev->port; + + sas_event_fns[ev->type](work); + sas_port_put(port); +} + + /* ---------- Events ---------- */ static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw) { + int ret; + struct sas_discovery_event *ev = to_sas_discovery_event(&sw->work); + struct asd_sas_port *port = ev->port; + /* chained work is not subject to SA_HA_DRAINING or * SAS_HA_REGISTERED, because it is either submitted in the * workqueue, or known to be submitted from a context that is * not racing against draining */ - scsi_queue_work(ha->core.shost, &sw->work); + sas_port_get(port); + ret = scsi_queue_work(ha->core.shost, &sw->work); + if (ret != 1) + sas_port_put(port); } static void sas_chain_event(int event, unsigned long *pending, @@ -575,18 +602,10 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port) { int i; - static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { - [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, - [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, - [DISCE_PROBE] = sas_probe_devices, - [DISCE_SUSPEND] = sas_suspend_devices, - [DISCE_RESUME] = sas_resume_devices, - [DISCE_DESTRUCT] = sas_destruct_devices, - }; - disc->pending = 0; for (i = 0; i < DISC_NUM_EVENTS; i++) { - INIT_SAS_WORK(&disc->disc_work[i].work, sas_event_fns[i]); + INIT_SAS_WORK(&disc->disc_work[i].work, sas_discover_common_fn); disc->disc_work[i].port = port; + disc->disc_work[i].type = i; } } diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h index f03ce64..890b5d26 100644 --- a/drivers/scsi/libsas/sas_internal.h +++ b/drivers/scsi/libsas/sas_internal.h @@ -100,6 +100,40 @@ void sas_free_device(struct kref *kref); extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS]; extern const work_func_t sas_port_event_fns[PORT_NUM_EVENTS]; +static void sas_complete_event(struct kref *kref) +{ + struct asd_sas_port *port = container_of(kref, struct asd_sas_port, ref); + + complete_all(&port->completion); +} + +static inline void sas_port_put(struct asd_sas_port *port) +{ + if (port->is_sync) + kref_put(&port->ref, sas_complete_event); +} + +static inline void sas_port_wait_init(struct asd_sas_port *port) +{ + init_completion(&port->completion); + kref_init(&port->ref); + port->is_sync = true; +} + +static inline void sas_port_wait_completion( + struct asd_sas_port *port) +{ + sas_port_put(port); + wait_for_completion(&port->completion); + port->is_sync = false; +} + +static inline void sas_port_get(struct asd_sas_port *port) +{ + if (port && port->is_sync) + kref_get(&port->ref); +} + #ifdef CONFIG_SCSI_SAS_HOST_SMP extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req, struct request *rsp); diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c index 9326628..d589adb 100644 --- a/drivers/scsi/libsas/sas_port.c +++ b/drivers/scsi/libsas/sas_port.c @@ -191,7 +191,9 @@ static void sas_form_port(struct asd_sas_phy *phy) if (si->dft->lldd_port_formed) si->dft->lldd_port_formed(phy); + sas_port_wait_init(port); sas_discover_event(phy->port, DISCE_DISCOVER_DOMAIN); + sas_port_wait_completion(port); } /** @@ -218,7 +220,9 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) dev->pathways--; if (port->num_phys == 1) { + sas_port_wait_init(port); sas_unregister_domain_devices(port, gone); + sas_port_wait_completion(port); sas_port_delete(port->port); port->port = NULL; } else { diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index a01ca42..c2ef05e 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -242,6 +242,7 @@ static inline void INIT_SAS_WORK(struct sas_work *sw, void (*fn)(struct work_str struct sas_discovery_event { struct sas_work work; struct asd_sas_port *port; + enum discover_event type; }; static inline struct sas_discovery_event *to_sas_discovery_event(struct work_struct *work) @@ -273,7 +274,9 @@ struct asd_sas_port { struct sas_work work; int suspended; - + struct kref ref; + struct completion completion; + bool is_sync; /* public: */ int id;
Introduce wait-complete for libsas sas event processing, execute sas port create/destruct in sync. Signed-off-by: Yijing Wang <wangyijing@huawei.com> CC: John Garry <john.garry@huawei.com> CC: Johannes Thumshirn <jthumshirn@suse.de> CC: Ewan Milne <emilne@redhat.com> CC: Christoph Hellwig <hch@lst.de> CC: Tomas Henzl <thenzl@redhat.com> CC: Dan Williams <dan.j.williams@intel.com> --- drivers/scsi/libsas/sas_discover.c | 41 ++++++++++++++++++++++++++++---------- drivers/scsi/libsas/sas_internal.h | 34 +++++++++++++++++++++++++++++++ drivers/scsi/libsas/sas_port.c | 4 ++++ include/scsi/libsas.h | 5 ++++- 4 files changed, 72 insertions(+), 12 deletions(-)