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

Groups > linux.kernel > #1734698 > unrolled thread

[RESEND PATCH v4 0/6] Enhance libsas hotplug feature

Back to article view | Back to linux.kernel

Contents

  [RESEND PATCH v4 0/6] Enhance libsas hotplug feature Jason Yan <yanaijie@huawei.com> - 2017-09-19 09:10 +0200
    [RESEND PATCH v4 3/6] libsas: make the event threshold configurable Jason Yan <yanaijie@huawei.com> - 2017-09-19 09:10 +0200
    [RESEND PATCH v4 4/6] libsas: Use new workqueue to run sas event and disco event Jason Yan <yanaijie@huawei.com> - 2017-09-19 09:10 +0200
    [RESEND PATCH v4 2/6] libsas: shut down the PHY if events reached the threshold Jason Yan <yanaijie@huawei.com> - 2017-09-19 09:10 +0200

#1734698 — [RESEND PATCH v4 0/6] Enhance libsas hotplug feature

Thanks Martin K. Petersen for applied some of the tidy-up patches. So I do not
have to maintain these patches out of the tree. I will only send the reset
of them in the next days if needed.

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 devided 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: -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
v2->v3: some code improvements suggested by Johannes and John,
        split v2 patch 2 into several small pathes.
v1->v2: some code improvements suggested by John Garry

Jason Yan (6):
  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

 drivers/scsi/hisi_sas/hisi_sas_main.c |  6 +++
 drivers/scsi/libsas/sas_ata.c         |  1 -
 drivers/scsi/libsas/sas_discover.c    | 36 ++++++++------
 drivers/scsi/libsas/sas_event.c       | 79 ++++++++++++++++++++++--------
 drivers/scsi/libsas/sas_expander.c    |  2 +-
 drivers/scsi/libsas/sas_init.c        | 91 +++++++++++++++++++++++++++++++++--
 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                 | 29 ++++++++---
 include/scsi/scsi_transport_sas.h     |  1 +
 11 files changed, 258 insertions(+), 92 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1734701 — [RESEND PATCH v4 3/6] libsas: make the event threshold configurable

Add a sysfs attr that LLDD can configure it for every host. We made
a example in hisi_sas. Other LLDDs using libsas can implement it if
they want.

Suggested-by: Hannes Reinecke <hare@suse.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>
CC: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c |  6 ++++++
 drivers/scsi/libsas/sas_init.c        | 27 +++++++++++++++++++++++++++
 include/scsi/libsas.h                 |  1 +
 3 files changed, 34 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 5e47abb..9eceed1 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1470,6 +1470,11 @@ EXPORT_SYMBOL_GPL(hisi_sas_rescan_topology);
 struct scsi_transport_template *hisi_sas_stt;
 EXPORT_SYMBOL_GPL(hisi_sas_stt);
 
+struct device_attribute *host_attrs[] = {
+    &dev_attr_phy_event_threshold,
+    NULL,
+};
+
 static struct scsi_host_template _hisi_sas_sht = {
 	.module			= THIS_MODULE,
 	.name			= DRV_NAME,
@@ -1489,6 +1494,7 @@ static struct scsi_host_template _hisi_sas_sht = {
 	.eh_bus_reset_handler	= sas_eh_bus_reset_handler,
 	.target_destroy		= sas_target_destroy,
 	.ioctl			= sas_ioctl,
+	.shost_attrs		= host_attrs,
 };
 struct scsi_host_template *hisi_sas_sht = &_hisi_sas_sht;
 EXPORT_SYMBOL_GPL(hisi_sas_sht);
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index b1e03d5..e2d98a8 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -537,6 +537,33 @@ static struct sas_function_template sft = {
 	.smp_handler = sas_smp_handler,
 };
 
+static inline ssize_t phy_event_threshold_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", sha->event_thres);
+}
+
+static inline ssize_t phy_event_threshold_store(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
+
+	sha->event_thres = simple_strtol(buf, NULL, 10);
+
+	return count;
+}
+
+DEVICE_ATTR(phy_event_threshold,
+             S_IRUGO|S_IWUSR,
+             phy_event_threshold_show,
+             phy_event_threshold_store);
+EXPORT_SYMBOL_GPL(dev_attr_phy_event_threshold);
+
 struct scsi_transport_template *
 sas_domain_attach_transport(struct sas_domain_function_template *dft)
 {
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2fa0b13..08c1c32 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -679,6 +679,7 @@ extern int sas_bios_param(struct scsi_device *,
 			  sector_t capacity, int *hsc);
 extern struct scsi_transport_template *
 sas_domain_attach_transport(struct sas_domain_function_template *);
+extern struct device_attribute dev_attr_phy_event_threshold;
 
 int  sas_discover_root_expander(struct domain_device *);
 
-- 
2.5.0

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

#1734702 — [RESEND PATCH v4 4/6] libsas: Use new workqueue to run sas event and disco event

Now all libsas works are queued to scsi host workqueue,
include sas event work post by LLDD and sas discovery
work, and a sas hotplug flow may be divided into several
works, e.g libsas receive a PORTE_BYTES_DMAED event,
currently we process it as following steps:
sas_form_port  --- run in work in shost workq
	sas_discover_domain  --- run in another work in shost workq
		...
		sas_probe_devices  --- run in new work in shost workq
We found during hot-add a device, libsas may need run several
works in same workqueue to add device in system, the process is
not atomic, it may interrupt by other sas event works, like
PHYE_LOSS_OF_SIGNAL.

This patch is preparation of execute libsas sas event in sync. We need
to use different workqueue to run sas event and disco event. Otherwise
the work will be blocked for waiting another chained work in the same
workqueue.

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>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_discover.c |  2 +-
 drivers/scsi/libsas/sas_event.c    |  4 ++--
 drivers/scsi/libsas/sas_init.c     | 18 ++++++++++++++++++
 include/scsi/libsas.h              |  3 +++
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 60de662..14f714d 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -534,7 +534,7 @@ static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw)
 	 * workqueue, or known to be submitted from a context that is
 	 * not racing against draining
 	 */
-	scsi_queue_work(ha->core.shost, &sw->work);
+	queue_work(ha->disco_q, &sw->work);
 }
 
 static void sas_chain_event(int event, unsigned long *pending,
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 6db941d..b124198 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -40,7 +40,7 @@ int sas_queue_work(struct sas_ha_struct *ha, struct sas_work *sw)
 		if (list_empty(&sw->drain_node))
 			list_add_tail(&sw->drain_node, &ha->defer_q);
 	} else
-		rc = scsi_queue_work(ha->core.shost, &sw->work);
+		rc = queue_work(ha->event_q, &sw->work);
 
 	return rc;
 }
@@ -61,7 +61,7 @@ static int sas_queue_event(int event, struct sas_work *work,
 
 void __sas_drain_work(struct sas_ha_struct *ha)
 {
-	struct workqueue_struct *wq = ha->core.shost->work_q;
+	struct workqueue_struct *wq = ha->event_q;
 	struct sas_work *sw, *_sw;
 	int ret;
 
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index e2d98a8..b49c46f 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -109,6 +109,7 @@ void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
 
 int sas_register_ha(struct sas_ha_struct *sas_ha)
 {
+	char name[64];
 	int error = 0;
 
 	mutex_init(&sas_ha->disco_mutex);
@@ -142,10 +143,24 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
 		goto Undo_ports;
 	}
 
+	error = -ENOMEM;
+	snprintf(name, sizeof(name), "%s_event_q", dev_name(sas_ha->dev));
+	sas_ha->event_q = create_singlethread_workqueue(name);
+	if (!sas_ha->event_q)
+		goto Undo_ports;
+
+	snprintf(name, sizeof(name), "%s_disco_q", dev_name(sas_ha->dev));
+	sas_ha->disco_q = create_singlethread_workqueue(name);
+	if (!sas_ha->disco_q)
+		goto Undo_event_q;
+
 	INIT_LIST_HEAD(&sas_ha->eh_done_q);
 	INIT_LIST_HEAD(&sas_ha->eh_ata_q);
 
 	return 0;
+
+Undo_event_q:
+	destroy_workqueue(sas_ha->event_q);
 Undo_ports:
 	sas_unregister_ports(sas_ha);
 Undo_phys:
@@ -176,6 +191,9 @@ int sas_unregister_ha(struct sas_ha_struct *sas_ha)
 	__sas_drain_work(sas_ha);
 	mutex_unlock(&sas_ha->drain_mutex);
 
+	destroy_workqueue(sas_ha->disco_q);
+	destroy_workqueue(sas_ha->event_q);
+
 	return 0;
 }
 
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 08c1c32..d1ab157 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -388,6 +388,9 @@ struct sas_ha_struct {
 	struct device *dev;	  /* should be set */
 	struct module *lldd_module; /* should be set */
 
+	struct workqueue_struct *event_q;
+	struct workqueue_struct *disco_q;
+
 	u8 *sas_addr;		  /* must be set */
 	u8 hashed_sas_addr[HASHED_SAS_ADDR_SIZE];
 
-- 
2.5.0

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

#1734703 — [RESEND PATCH v4 2/6] 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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web