Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1727283 > unrolled thread

[PATCH v4 00/11] Enhance libsas hotplug feature

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan <yanaijie@huawei.com> - 2017-09-06 11:20 +0200
    [PATCH v4 06/11] libsas: shut down the PHY if events reached the threshold Jason Yan <yanaijie@huawei.com> - 2017-09-06 11:20 +0200
    [PATCH v4 02/11] libsas: remove the numbering for each event enum Jason Yan <yanaijie@huawei.com> - 2017-09-06 11:20 +0200
      Re: [PATCH v4 02/11] libsas: remove the numbering for each event enum Johannes Thumshirn <jthumshirn@suse.de> - 2017-09-06 14:50 +0200
      Re: [PATCH v4 02/11] libsas: remove the numbering for each event         enum Christoph Hellwig <hch@lst.de> - 2017-09-06 15:30 +0200
    Re: [PATCH v4 00/11] Enhance libsas hotplug feature Christoph Hellwig <hch@lst.de> - 2017-09-06 15:30 +0200
      Re: [PATCH v4 00/11] Enhance libsas hotplug feature Jason Yan <yanaijie@huawei.com> - 2017-09-07 03:20 +0200
    Re: [PATCH v4 00/11] Enhance libsas hotplug feature John Garry <john.garry@huawei.com> - 2017-09-06 15:30 +0200

#1727283 — [PATCH v4 00/11] Enhance libsas hotplug feature

Hello all, Yijing Wang handed over this topic to me. We are working
on it the last two months. We have tested the patchset for a long
time. Here is the new version.

Now the libsas hotplug has some issues, Dan Williams report
a similar bug here before
https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg39187.html

The issues we have found
1. if LLDD burst reports lots of phy-up/phy-down sas events, some events
   may lost because a same sas events is pending now, finally libsas topo
   may different the hardware.
2. receive a phy down sas event, libsas call sas_deform_port to remove
   devices, it would first delete the sas port, then put a destruction
   discovery event in a new work, and queue it at the tail of workqueue,
   once the sas port be deleted, its children device will be deleted too,
   when the destruction work start, it will found the target device has
   been removed, and report a sysfs warnning.
3. since a hotplug process will be divided into several works, if a phy up
   sas event insert into phydown works, like
   destruction work  ---> PORTE_BYTES_DMAED (sas_form_port) ---->PHYE_LOSS_OF_SIGNAL
   the hot remove flow would broken by PORTE_BYTES_DMAED event, it's not
   we expected, and issues would occur.

v3->v4: -get rid of unused ha event and do some cleanup
        -use dynamic alloced work and support shutting down the phy if active event reached the threshold
		-use flush_workqueue instead of wait-completion to process discover events synchronously
		-direct call probe and destruct function
		-other small code improvements 
v2->v3: some code improvements suggested by Johannes and John,
	    split v2 patch 2 into several small patches.
v1->v2: some code improvements suggested by John Garry

Jason Yan (10):
  libsas: kill useless ha_event and do some cleanup
  libsas: remove the numbering for each event enum
  libsas: remove unused port_gone_completion and DISCE_PORT_GONE
  libsas: rename notify_port_event() for consistency
  libsas: Use dynamic alloced work to avoid sas event lost
  libsas: shut down the PHY if events reached the threshold
  libsas: make the event threshold configurable
  libsas: Use new workqueue to run sas event and disco event
  libsas: libsas: use flush_workqueue to process disco events
    synchronously
  libsas: direct call probe and destruct

chenxiang (1):
  libsas: add event to defer list tail instead of head when draining

 drivers/scsi/aic94xx/aic94xx_hwi.c    |   3 -
 drivers/scsi/hisi_sas/hisi_sas_main.c |   7 ++-
 drivers/scsi/libsas/sas_ata.c         |   1 -
 drivers/scsi/libsas/sas_discover.c    |  36 +++++++-----
 drivers/scsi/libsas/sas_dump.c        |  10 ----
 drivers/scsi/libsas/sas_dump.h        |   1 -
 drivers/scsi/libsas/sas_event.c       |  97 +++++++++++++++++++-------------
 drivers/scsi/libsas/sas_expander.c    |   2 +-
 drivers/scsi/libsas/sas_init.c        | 101 +++++++++++++++++++++++++++++-----
 drivers/scsi/libsas/sas_internal.h    |   7 +++
 drivers/scsi/libsas/sas_phy.c         |  73 ++++++++++++------------
 drivers/scsi/libsas/sas_port.c        |  25 +++++----
 include/scsi/libsas.h                 |  81 ++++++++++++---------------
 include/scsi/scsi_transport_sas.h     |   1 +
 14 files changed, 270 insertions(+), 175 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1727285 — [PATCH v4 06/11] libsas: shut down the PHY if events reached the threshold

If the PHY burst too many events, we will alloc a lot of events for the
worker. This may leads to memory exhaustion.

Dan Williams suggested to shut down the PHY if the events reached the
threshold, because in this case the PHY may have gone into some
erroneous state. Users can re-enable the PHY by sysfs if they want.

We cannot use the fixed memory pool because if we run out of events, the
shut down event and loss of signal event will lost too. The events still
need to be allocated and processed in this case.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jason Yan <yanaijie@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>
---
 drivers/scsi/libsas/sas_init.c | 21 ++++++++++++++++++++-
 drivers/scsi/libsas/sas_phy.c  | 31 ++++++++++++++++++++++++++++++-
 include/scsi/libsas.h          |  6 ++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 85c278a..b1e03d5 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -122,6 +122,8 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
 	INIT_LIST_HEAD(&sas_ha->defer_q);
 	INIT_LIST_HEAD(&sas_ha->eh_dev_q);
 
+	sas_ha->event_thres = SAS_PHY_SHUTDOWN_THRES;
+
 	error = sas_register_phys(sas_ha);
 	if (error) {
 		printk(KERN_NOTICE "couldn't register sas phys:%d\n", error);
@@ -556,14 +558,31 @@ EXPORT_SYMBOL_GPL(sas_domain_attach_transport);
 
 struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
 {
+	struct asd_sas_event *event;
 	gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
 
-	return kmem_cache_zalloc(sas_event_cache, flags);
+	event = kmem_cache_zalloc(sas_event_cache, flags);
+	if (!event)
+		return NULL;
+
+	atomic_inc(&phy->event_nr);
+	if (atomic_read(&phy->event_nr) > phy->ha->event_thres &&
+	    !phy->in_shutdown) {
+		phy->in_shutdown = 1;
+		sas_printk("The phy%02d bursting events, shut it down.\n",
+			   phy->id);
+		sas_notify_phy_event(phy, PHYE_SHUTDOWN);
+	}
+
+	return event;
 }
 
 void sas_free_event(struct asd_sas_event *event)
 {
+	struct asd_sas_phy *phy = event->phy;
+
 	kmem_cache_free(sas_event_cache, event);
+	atomic_dec(&phy->event_nr);
 }
 
 /* ---------- SAS Class register/unregister ---------- */
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index 59f8292..3df1eec 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -35,6 +35,7 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
+	phy->in_shutdown = 0;
 	phy->error = 0;
 	sas_deform_port(phy, 1);
 }
@@ -44,6 +45,7 @@ static void sas_phye_oob_done(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
+	phy->in_shutdown = 0;
 	phy->error = 0;
 }
 
@@ -105,6 +107,32 @@ static void sas_phye_resume_timeout(struct work_struct *work)
 }
 
 
+static void sas_phye_shutdown(struct work_struct *work)
+{
+	struct asd_sas_event *ev = to_asd_sas_event(work);
+	struct asd_sas_phy *phy = ev->phy;
+	struct sas_ha_struct *sas_ha = phy->ha;
+	struct sas_internal *i =
+		to_sas_internal(sas_ha->core.shost->transportt);
+
+	if (phy->enabled && i->dft->lldd_control_phy) {
+		int ret;
+
+		phy->error = 0;
+		phy->enabled = 0;
+		ret = i->dft->lldd_control_phy(phy, PHY_FUNC_DISABLE, NULL);
+		if (ret)
+			sas_printk("lldd disable phy%02d returned %d\n",
+				phy->id, ret);
+
+	} else if (!i->dft->lldd_control_phy)
+		sas_printk("lldd does not support phy%02d control\n", phy->id);
+	else
+		sas_printk("phy%02d is not enabled, cannot shutdown\n",
+			phy->id);
+
+}
+
 /* ---------- Phy class registration ---------- */
 
 int sas_register_phys(struct sas_ha_struct *sas_ha)
@@ -116,6 +144,7 @@ int sas_register_phys(struct sas_ha_struct *sas_ha)
 		struct asd_sas_phy *phy = sas_ha->sas_phy[i];
 
 		phy->error = 0;
+		atomic_set(&phy->event_nr, 0);
 		INIT_LIST_HEAD(&phy->port_phy_el);
 
 		phy->port = NULL;
@@ -151,5 +180,5 @@ const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS] = {
 	[PHYE_OOB_ERROR] = sas_phye_oob_error,
 	[PHYE_SPINUP_HOLD] = sas_phye_spinup_hold,
 	[PHYE_RESUME_TIMEOUT] = sas_phye_resume_timeout,
-
+	[PHYE_SHUTDOWN] = sas_phye_shutdown,
 };
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index c80321b..2fa0b13 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -75,6 +75,7 @@ enum phy_event {
 	PHYE_OOB_ERROR,
 	PHYE_SPINUP_HOLD,             /* hot plug SATA, no COMWAKE sent */
 	PHYE_RESUME_TIMEOUT,
+	PHYE_SHUTDOWN,
 	PHY_NUM_EVENTS,
 };
 
@@ -310,12 +311,15 @@ static inline void INIT_SAS_EVENT(struct asd_sas_event *ev, void (*fn)(struct wo
 	ev->event = event;
 }
 
+#define SAS_PHY_SHUTDOWN_THRES   1024
 
 /* The phy pretty much is controlled by the LLDD.
  * The class only reads those fields.
  */
 struct asd_sas_phy {
 /* private: */
+	atomic_t event_nr;
+	int in_shutdown;
 	int error;
 	int suspended;
 
@@ -403,6 +407,8 @@ struct sas_ha_struct {
 
 	struct list_head eh_done_q;  /* complete via scsi_eh_flush_done_q */
 	struct list_head eh_ata_q; /* scmds to promote from sas to ata eh */
+
+	int event_thres;
 };
 
 #define SHOST_TO_SAS_HA(_shost) (*(struct sas_ha_struct **)(_shost)->hostdata)
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1727286 — [PATCH v4 02/11] libsas: remove the numbering for each event enum

Numbering for each event enum makes no sense. Remove the numbering so
that we don't have to calculate the number by hand every time.

Signed-off-by: Jason Yan <yanaijie@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>
---
 include/scsi/libsas.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index e536597..ccf3b48 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -62,31 +62,31 @@ enum sas_phy_type {
  */
 enum port_event {
 	PORTE_BYTES_DMAED     = 0U,
-	PORTE_BROADCAST_RCVD  = 1,
-	PORTE_LINK_RESET_ERR  = 2,
-	PORTE_TIMER_EVENT     = 3,
-	PORTE_HARD_RESET      = 4,
-	PORT_NUM_EVENTS       = 5,
+	PORTE_BROADCAST_RCVD,
+	PORTE_LINK_RESET_ERR,
+	PORTE_TIMER_EVENT,
+	PORTE_HARD_RESET,
+	PORT_NUM_EVENTS,
 };
 
 enum phy_event {
 	PHYE_LOSS_OF_SIGNAL   = 0U,
-	PHYE_OOB_DONE         = 1,
-	PHYE_OOB_ERROR        = 2,
-	PHYE_SPINUP_HOLD      = 3, /* hot plug SATA, no COMWAKE sent */
-	PHYE_RESUME_TIMEOUT   = 4,
-	PHY_NUM_EVENTS        = 5,
+	PHYE_OOB_DONE,
+	PHYE_OOB_ERROR,
+	PHYE_SPINUP_HOLD,             /* hot plug SATA, no COMWAKE sent */
+	PHYE_RESUME_TIMEOUT,
+	PHY_NUM_EVENTS,
 };
 
 enum discover_event {
 	DISCE_DISCOVER_DOMAIN   = 0U,
-	DISCE_REVALIDATE_DOMAIN = 1,
-	DISCE_PORT_GONE         = 2,
-	DISCE_PROBE		= 3,
-	DISCE_SUSPEND		= 4,
-	DISCE_RESUME		= 5,
-	DISCE_DESTRUCT		= 6,
-	DISC_NUM_EVENTS		= 7,
+	DISCE_REVALIDATE_DOMAIN,
+	DISCE_PORT_GONE,
+	DISCE_PROBE,
+	DISCE_SUSPEND,
+	DISCE_RESUME,
+	DISCE_DESTRUCT,
+	DISC_NUM_EVENTS,
 };
 
 /* ---------- Expander Devices ---------- */
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1727414 — Re: [PATCH v4 02/11] libsas: remove the numbering for each event enum

I guess it boils down to personal preference, but
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1727460 — Re: [PATCH v4 02/11] libsas: remove the numbering for each event enum

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1727468

On Wed, Sep 06, 2017 at 02:07:57PM +0100, John Garry wrote:
> Regardless of the fate of the rest of the patches in this series, I think 
> patches 1,2,3,4,11/11 can be taken in isolation (subject to review, of 
> course). It would save maintaining them out-of-tree.

I did a quick review of those and they all look fine to me.

I'll try to find some time to review the real changes in the next
days.

[toc] | [prev] | [next] | [standalone]

#1727864

On 2017/9/6 21:22, Christoph Hellwig wrote:
> On Wed, Sep 06, 2017 at 02:07:57PM +0100, John Garry wrote:
>> Regardless of the fate of the rest of the patches in this series, I think
>> patches 1,2,3,4,11/11 can be taken in isolation (subject to review, of
>> course). It would save maintaining them out-of-tree.
>
> I did a quick review of those and they all look fine to me.
>
> I'll try to find some time to review the real changes in the next
> days.
>
> .
>

Thank you very much and I'm looking forward to your suggestions
of the real changes.

[toc] | [prev] | [next] | [standalone]

#1727470

On 06/09/2017 10:15, Jason Yan wrote:
> Hello all, Yijing Wang handed over this topic to me. We are working
> on it the last two months. We have tested the patchset for a long
> time. Here is the new version.
>
> Now the libsas hotplug has some issues, Dan Williams report
> a similar bug here before
> https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg39187.html
>
> The issues we have found
> 1. if LLDD burst reports lots of phy-up/phy-down sas events, some events
>    may lost because a same sas events is pending now, finally libsas topo
>    may different the hardware.
> 2. receive a phy down sas event, libsas call sas_deform_port to remove
>    devices, it would first delete the sas port, then put a destruction
>    discovery event in a new work, and queue it at the tail of workqueue,
>    once the sas port be deleted, its children device will be deleted too,
>    when the destruction work start, it will found the target device has
>    been removed, and report a sysfs warnning.
> 3. since a hotplug process will be divided into several works, if a phy up
>    sas event insert into phydown works, like
>    destruction work  ---> PORTE_BYTES_DMAED (sas_form_port) ---->PHYE_LOSS_OF_SIGNAL
>    the hot remove flow would broken by PORTE_BYTES_DMAED event, it's not
>    we expected, and issues would occur.
>
> v3->v4: -get rid of unused ha event and do some cleanup
>         -use dynamic alloced work and support shutting down the phy if active event reached the threshold
> 		-use flush_workqueue instead of wait-completion to process discover events synchronously
> 		-direct call probe and destruct function
> 		-other small code improvements
> v2->v3: some code improvements suggested by Johannes and John,
> 	    split v2 patch 2 into several small patches.
> v1->v2: some code improvements suggested by John Garry
>
> Jason Yan (10):
>   libsas: kill useless ha_event and do some cleanup
>   libsas: remove the numbering for each event enum
>   libsas: remove unused port_gone_completion and DISCE_PORT_GONE
>   libsas: rename notify_port_event() for consistency
>   libsas: Use dynamic alloced work to avoid sas event lost
>   libsas: shut down the PHY if events reached the threshold
>   libsas: make the event threshold configurable
>   libsas: Use new workqueue to run sas event and disco event
>   libsas: libsas: use flush_workqueue to process disco events
>     synchronously
>   libsas: direct call probe and destruct
>
> chenxiang (1):
>   libsas: add event to defer list tail instead of head when draining
>

Regardless of the fate of the rest of the patches in this series, I 
think patches 1,2,3,4,11/11 can be taken in isolation (subject to 
review, of course). It would save maintaining them out-of-tree.

John

>  drivers/scsi/aic94xx/aic94xx_hwi.c    |   3 -
>  drivers/scsi/hisi_sas/hisi_sas_main.c |   7 ++-
>  drivers/scsi/libsas/sas_ata.c         |   1 -
>  drivers/scsi/libsas/sas_discover.c    |  36 +++++++-----
>  drivers/scsi/libsas/sas_dump.c        |  10 ----
>  drivers/scsi/libsas/sas_dump.h        |   1 -
>  drivers/scsi/libsas/sas_event.c       |  97 +++++++++++++++++++-------------
>  drivers/scsi/libsas/sas_expander.c    |   2 +-
>  drivers/scsi/libsas/sas_init.c        | 101 +++++++++++++++++++++++++++++-----
>  drivers/scsi/libsas/sas_internal.h    |   7 +++
>  drivers/scsi/libsas/sas_phy.c         |  73 ++++++++++++------------
>  drivers/scsi/libsas/sas_port.c        |  25 +++++----
>  include/scsi/libsas.h                 |  81 ++++++++++++---------------
>  include/scsi/scsi_transport_sas.h     |   1 +
>  14 files changed, 270 insertions(+), 175 deletions(-)
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web