[PATCH v3 0/6] [SCSI] Enhanced sense and Unit Attention handling

[PATCH v3 0/6] [SCSI] Enhanced sense and Unit Attention handling

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
to provide enhanced support for Unit Attention conditions, as well as
detection of a unit attention queue overflow condition and the ability
for drivers to report sense data outside of normal command completion.

There was some discussion about this a couple of years ago on the linux-scsi
mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
Although one approach is to send all SCSI sense data to a userspace daemon
for processing, this patch set does not take that approach due to the
difficulty in reliably delivering all of the data.  An interesting UA
condition might not be delivered due to a flood of media errors, for example.

The mechanism used is to flag when certain UA ASC/ASCQ codes are received
that report asynchronous changes to the storage device configuration.
An appropriate uevent is then generated for the scsi_device or scsi_target
object.  An aggregation mechanism is used to avoid generating uevents at
too high a rate, and to coalesce multiple UAs reported by LUNs on the
same target for a REPORTED LUNS DATA HAS CHANGED sense code.

The changes are enabled by a new kernel config option CONFIG_SCSI_ENHANCED_UA.
If this config option is not used, no new uevents are generated.  There are
some changes to kernel logging messages if CONFIG_SCSI_ENHANCED_UA is enabled,
because the existing messages explicitly stated that the kernel did not do
anything with the information.

Note that checkpatch is reporting errors on patch 5/6 relating to macros
in scsi_sysfs.c -- I believe these errors are incorrect and have sent a
message to the checkpatch maintainer.  The macros were derived from
existing ones already in the file.

Changes made since earlier v2 version:

   - Remove patch 1/8 "Generate uevent on sd capacity change"
   - Remove patch 8/8 "Streamline detection of FM/EOM/ILI status"
   - Changed scsi_debug to not generate UA on INQUIRY or REPORT_LUNS
   - Changed scsi_debug to only report UA queue overflow condition
     if dsense=1, as descriptor format sense data is needed

Changes made since earlier RFC version:

   - Remove patch 1/9 "Detect overflow of sense data buffer"
     Some scsi_debug changes in this patch were moved to patch 7/8
   - Corrected Kconfig help text
   - Change name of "sdev_evt_thread" to "sdev_evt_work"
   - Change name of "starget_evt_thread" to "starget_evt_work"
   - Pull code out of scsi_check_sense() that handles UAs into
     an exported function so that drivers can report conditions
     received asynchronously

Thanks to everyone for the comments on this patch series.

Ewan D. Milne (6):
  [SCSI] Add a kernel config option for enhanced Unit Attention support
  [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to
    sdev_event
  [SCSI] Add support for scsi_target events
  [SCSI] Generate uevents for certain Unit Attention codes
  [SCSI] Add sysfs support for enhanced Unit Attention handling
  [SCSI] Add sense and Unit Attention generation to scsi_debug

 drivers/scsi/Kconfig       |  12 +++
 drivers/scsi/scsi_debug.c  | 138 +++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_error.c  | 187 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_lib.c    | 176 ++++++++++++++++++++++++++++++++++++++----
 drivers/scsi/scsi_priv.h   |  10 ++-
 drivers/scsi/scsi_scan.c   |  54 +++++++------
 drivers/scsi/scsi_sysfs.c  | 146 ++++++++++++++++++++++++++++++++---
 include/scsi/scsi_cmnd.h   |   4 +
 include/scsi/scsi_device.h |  62 ++++++++++++++-
 include/scsi/scsi_eh.h     |   5 ++
 10 files changed, 716 insertions(+), 78 deletions(-)

-- 
1.7.11.7

[PATCH v3 1/6] [SCSI] Add a kernel config option for enhanced Unit Attention support

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added CONFIG_SCSI_ENHANCED_UA kernel config option to enable changes
in the SCSI mid-layer which detect and report certain Unit Attention
conditions reported by devices.  These changes are primarily useful
when storage arrays that can be reconfigured are being used, so the
config option would normally not be used unless it was needed.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index e955978..5d1e614 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -280,6 +280,18 @@ config SCSI_WAIT_SCAN
 # disabling it, whereas people who accidentally switch it off may wonder why
 # their mkinitrd gets into trouble.
 
+config SCSI_ENHANCED_UA
+       bool "Enhanced SCSI Unit Attention handling"
+       depends on SCSI
+       help
+	 Certain SCSI devices report changes via a UNIT ATTENTION code.
+	 (For example, the addition or removal of LUNs from a target, or
+	 changing the number of logical blocks on a LUN.)  This option
+	 enables reporting of these changes via udev events, so that the
+	 device can be rescanned to find out what has changed.  This is
+	 primarily useful when storage arrays that can be reconfigured
+	 are attached to the system, otherwise you can say N here.
+
 menu "SCSI Transports"
 	depends on SCSI
 
-- 
1.7.11.7

[PATCH v3 2/6] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

The names of the struct and some of the functions for scsi_device
events are too generic and do not match the comments in the source.
Changed all of the names to begin with sdev_ in order to avoid
naming issues and confusion with scsi_target events to be added.
Also changed name of sdev_evt_thread() to sdev_evt_work().

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 24 ++++++++++++------------
 drivers/scsi/scsi_priv.h   |  1 +
 drivers/scsi/scsi_scan.c   |  3 +--
 drivers/scsi/scsi_sysfs.c  |  4 ++--
 include/scsi/scsi_device.h |  6 +++---
 5 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6dfb978..53f074d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(scsi_device_set_state);
  *	@sdev: associated SCSI device
  *	@evt: event to emit
  *
- *	Send a single uevent (scsi_event) to the associated scsi_device.
+ *	Send a single uevent (sdev_event) to the associated scsi_device.
  */
-static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
+static void sdev_evt_emit(struct scsi_device *sdev, struct sdev_event *evt)
 {
 	int idx = 0;
 	char *envp[3];
@@ -2198,13 +2198,13 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 }
 
 /**
- * 	sdev_evt_thread - send a uevent for each scsi event
+ *	sdev_evt_work - send a uevent for each scsi event
  *	@work: work struct for scsi_device
  *
  *	Dispatch queued events to their associated scsi_device kobjects
  *	as uevents.
  */
-void scsi_evt_thread(struct work_struct *work)
+void sdev_evt_work(struct work_struct *work)
 {
 	struct scsi_device *sdev;
 	LIST_HEAD(event_list);
@@ -2212,7 +2212,7 @@ void scsi_evt_thread(struct work_struct *work)
 	sdev = container_of(work, struct scsi_device, event_work);
 
 	while (1) {
-		struct scsi_event *evt;
+		struct sdev_event *evt;
 		struct list_head *this, *tmp;
 		unsigned long flags;
 
@@ -2224,9 +2224,9 @@ void scsi_evt_thread(struct work_struct *work)
 			break;
 
 		list_for_each_safe(this, tmp, &event_list) {
-			evt = list_entry(this, struct scsi_event, node);
+			evt = list_entry(this, struct sdev_event, node);
 			list_del(&evt->node);
-			scsi_evt_emit(sdev, evt);
+			sdev_evt_emit(sdev, evt);
 			kfree(evt);
 		}
 	}
@@ -2239,7 +2239,7 @@ void scsi_evt_thread(struct work_struct *work)
  *
  *	Assert scsi device event asynchronously.
  */
-void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt)
+void sdev_evt_send(struct scsi_device *sdev, struct sdev_event *evt)
 {
 	unsigned long flags;
 
@@ -2265,12 +2265,12 @@ EXPORT_SYMBOL_GPL(sdev_evt_send);
  *	@evt_type: type of event to allocate
  *	@gfpflags: GFP flags for allocation
  *
- *	Allocates and returns a new scsi_event.
+ *	Allocates and returns a new sdev_event.
  */
-struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
+struct sdev_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 				  gfp_t gfpflags)
 {
-	struct scsi_event *evt = kzalloc(sizeof(struct scsi_event), gfpflags);
+	struct sdev_event *evt = kzalloc(sizeof(struct sdev_event), gfpflags);
 	if (!evt)
 		return NULL;
 
@@ -2300,7 +2300,7 @@ EXPORT_SYMBOL_GPL(sdev_evt_alloc);
 void sdev_evt_send_simple(struct scsi_device *sdev,
 			  enum scsi_device_event evt_type, gfp_t gfpflags)
 {
-	struct scsi_event *evt = sdev_evt_alloc(evt_type, gfpflags);
+	struct sdev_event *evt = sdev_evt_alloc(evt_type, gfpflags);
 	if (!evt) {
 		sdev_printk(KERN_ERR, sdev, "event %d eaten due to OOM\n",
 			    evt_type);
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 07ce3f5..7f00813 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -90,6 +90,7 @@ extern void scsi_exit_queue(void);
 struct request_queue;
 struct request;
 extern struct kmem_cache *scsi_sdb_cache;
+extern void sdev_evt_work(struct work_struct *work);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 2e5fe58..47348ed 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -244,7 +244,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	struct scsi_device *sdev;
 	int display_failure_msg = 1, ret;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
-	extern void scsi_evt_thread(struct work_struct *work);
 	extern void scsi_requeue_run_queue(struct work_struct *work);
 
 	sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
@@ -267,7 +266,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
-	INIT_WORK(&sdev->event_work, scsi_evt_thread);
+	INIT_WORK(&sdev->event_work, sdev_evt_work);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 04c2a27..686211e 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -353,9 +353,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	cancel_work_sync(&sdev->event_work);
 
 	list_for_each_safe(this, tmp, &sdev->event_list) {
-		struct scsi_event *evt;
+		struct sdev_event *evt;
 
-		evt = list_entry(this, struct scsi_event, node);
+		evt = list_entry(this, struct sdev_event, node);
 		list_del(&evt->node);
 		kfree(evt);
 	}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6efb2e1..ea16419 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -56,7 +56,7 @@ enum scsi_device_event {
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
 
-struct scsi_event {
+struct sdev_event {
 	enum scsi_device_event	evt_type;
 	struct list_head	node;
 
@@ -359,9 +359,9 @@ extern int scsi_get_vpd_page(struct scsi_device *, u8 page, unsigned char *buf,
 			     int buf_len);
 extern int scsi_device_set_state(struct scsi_device *sdev,
 				 enum scsi_device_state state);
-extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
+extern struct sdev_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 					  gfp_t gfpflags);
-extern void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt);
+extern void sdev_evt_send(struct scsi_device *sdev, struct sdev_event *evt);
 extern void sdev_evt_send_simple(struct scsi_device *sdev,
 			  enum scsi_device_event evt_type, gfp_t gfpflags);
 extern int scsi_device_quiesce(struct scsi_device *sdev);
-- 
1.7.11.7

[PATCH v3 3/6] [SCSI] Add support for scsi_target events

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added capability to generate uevents on scsi_target objects.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 135 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_priv.h   |   3 +
 drivers/scsi/scsi_scan.c   |  17 ++++++
 include/scsi/scsi_device.h |  34 ++++++++++++
 4 files changed, 189 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 53f074d..c55eea1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2390,6 +2390,141 @@ scsi_target_resume(struct scsi_target *starget)
 }
 EXPORT_SYMBOL(scsi_target_resume);
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+/**
+ *	starget_evt_emit - emit a single SCSI target uevent
+ *	@starget: associated SCSI target
+ *	@evt: event to emit
+ *
+ *	Send a single uevent (starget_event) to the associated scsi_target.
+ */
+static void starget_evt_emit(struct scsi_target *starget,
+			     struct starget_event *evt)
+{
+	int idx = 0;
+	char *envp[3];
+
+	switch (evt->evt_type) {
+	case STARGET_EVT_LUN_CHANGE_REPORTED:
+		envp[idx++] = "STARGET_LUN_CHANGE_REPORTED=1";
+		break;
+	default:
+		/* do nothing */
+		break;
+	}
+
+	envp[idx++] = NULL;
+
+	kobject_uevent_env(&starget->dev.kobj, KOBJ_CHANGE, envp);
+}
+
+/**
+ *	starget_evt_work - send a uevent for each scsi event
+ *	@work: work struct for scsi_target
+ *
+ *	Dispatch queued events to their associated scsi_target kobjects
+ *	as uevents.
+ */
+void starget_evt_work(struct work_struct *work)
+{
+	struct scsi_target *starget;
+	LIST_HEAD(event_list);
+
+	starget = container_of(work, struct scsi_target, event_work);
+
+	while (1) {
+		struct starget_event *evt;
+		struct list_head *this, *tmp;
+		unsigned long flags;
+
+		spin_lock_irqsave(&starget->list_lock, flags);
+		list_splice_init(&starget->event_list, &event_list);
+		spin_unlock_irqrestore(&starget->list_lock, flags);
+
+		if (list_empty(&event_list))
+			break;
+
+		list_for_each_safe(this, tmp, &event_list) {
+			evt = list_entry(this, struct starget_event, node);
+			list_del(&evt->node);
+			starget_evt_emit(starget, evt);
+			kfree(evt);
+		}
+	}
+}
+
+/**
+ *	starget_evt_send - send asserted event to uevent thread
+ *	@starget: scsi_target event occurred on
+ *	@evt: event to send
+ *
+ *	Assert scsi target event asynchronously.
+ */
+void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&starget->list_lock, flags);
+	list_add_tail(&evt->node, &starget->event_list);
+	schedule_work(&starget->event_work);
+	spin_unlock_irqrestore(&starget->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(starget_evt_send);
+
+/**
+ *	starget_evt_alloc - allocate a new scsi_target event
+ *	@evt_type: type of event to allocate
+ *	@gfpflags: GFP flags for allocation
+ *
+ *	Allocates and returns a new starget_event.
+ */
+struct starget_event *starget_evt_alloc(enum scsi_target_event evt_type,
+					gfp_t gfpflags)
+{
+	struct starget_event *evt = kzalloc(sizeof(struct starget_event),
+					    gfpflags);
+	if (!evt)
+		return NULL;
+
+	evt->evt_type = evt_type;
+	INIT_LIST_HEAD(&evt->node);
+
+	/* evt_type-specific initialization, if any */
+	switch (evt_type) {
+	case STARGET_EVT_LUN_CHANGE_REPORTED:
+	default:
+		/* do nothing */
+		break;
+	}
+
+	return evt;
+}
+EXPORT_SYMBOL_GPL(starget_evt_alloc);
+
+/**
+ *	starget_evt_send_simple - send asserted event to uevent thread
+ *	@starget: scsi_target event occurred on
+ *	@evt_type: type of event to send
+ *	@gfpflags: GFP flags for allocation
+ *
+ *	Assert scsi target event asynchronously, given an event type.
+ */
+void starget_evt_send_simple(struct scsi_target *starget,
+			     enum scsi_target_event evt_type, gfp_t gfpflags)
+{
+	struct starget_event *evt = starget_evt_alloc(evt_type, gfpflags);
+	if (!evt) {
+		starget_printk(KERN_ERR, starget, "event %d eaten due to OOM\n",
+			       evt_type);
+		return;
+	}
+
+	starget_evt_send(starget, evt);
+}
+EXPORT_SYMBOL_GPL(starget_evt_send_simple);
+
+#endif
+
 /**
  * scsi_internal_device_block - internal function to put a device temporarily into the SDEV_BLOCK state
  * @sdev:	device to block
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 7f00813..5da5466 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -91,6 +91,9 @@ struct request_queue;
 struct request;
 extern struct kmem_cache *scsi_sdb_cache;
 extern void sdev_evt_work(struct work_struct *work);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+extern void starget_evt_work(struct work_struct *work);
+#endif
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 47348ed..2d21597 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -425,6 +425,11 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->can_queue = 0;
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	INIT_LIST_HEAD(&starget->event_list);
+	spin_lock_init(&starget->list_lock);
+	INIT_WORK(&starget->event_work, starget_evt_work);
+#endif
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
@@ -472,7 +477,19 @@ static void scsi_target_reap_usercontext(struct work_struct *work)
 {
 	struct scsi_target *starget =
 		container_of(work, struct scsi_target, ew.work);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	struct list_head *this, *tmp;
+
+	cancel_work_sync(&starget->event_work);
+
+	list_for_each_safe(this, tmp, &starget->event_list) {
+		struct starget_event *evt;
 
+		evt = list_entry(this, struct starget_event, node);
+		list_del(&evt->node);
+		kfree(evt);
+	}
+#endif
 	transport_remove_device(&starget->dev);
 	device_del(&starget->dev);
 	scsi_target_destroy(starget);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index ea16419..69cbd7e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -229,6 +229,24 @@ enum scsi_target_state {
 	STARGET_DEL,
 };
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+enum scsi_target_event {
+	STARGET_EVT_LUN_CHANGE_REPORTED	= 1,	/* UA reported */
+
+	STARGET_EVT_LAST	= STARGET_EVT_LUN_CHANGE_REPORTED,
+	STARGET_EVT_MAXBITS	= STARGET_EVT_LAST + 1
+};
+
+struct starget_event {
+	enum scsi_target_event	evt_type;
+	struct list_head	node;
+
+	/* put union of data structures, for non-simple event types,
+	 * here
+	 */
+};
+#endif
+
 /*
  * scsi_target: representation of a scsi target, for now, this is only
  * used for single_lun devices. If no one has active IO to the target,
@@ -251,6 +269,13 @@ struct scsi_target {
 						 * means no lun present. */
 	unsigned int		no_report_luns:1;	/* Don't use
 						 * REPORT LUNS for scanning. */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	DECLARE_BITMAP(supported_events, STARGET_EVT_MAXBITS);
+	struct list_head	event_list;	/* asserted events */
+	struct work_struct	event_work;
+	spinlock_t list_lock;
+#endif
+
 	/* commands actually active on LLD. protected by host lock. */
 	unsigned int		target_busy;
 	/*
@@ -368,6 +393,15 @@ extern int scsi_device_quiesce(struct scsi_device *sdev);
 extern void scsi_device_resume(struct scsi_device *sdev);
 extern void scsi_target_quiesce(struct scsi_target *);
 extern void scsi_target_resume(struct scsi_target *);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+extern struct starget_event *starget_evt_alloc(enum scsi_target_event evt_type,
+					       gfp_t gfpflags);
+extern void starget_evt_send(struct scsi_target *starget,
+			     struct starget_event *evt);
+extern void starget_evt_send_simple(struct scsi_target *starget,
+				    enum scsi_target_event evt_type,
+				    gfp_t gfpflags);
+#endif
 extern void scsi_scan_target(struct device *parent, unsigned int channel,
 			     unsigned int id, unsigned int lun, int rescan);
 extern void scsi_target_reap(struct scsi_target *);
-- 
1.7.11.7

[PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Generate a uevent on the scsi_target object when the following
Unit Attention ASC/ASCQ code is received:

    3F/0E  REPORTED LUNS DATA HAS CHANGED

Generate a uevent on the scsi_device object when the following
Unit Attention ASC/ASCQ codes are received:

    2A/01  MODE PARAMETERS CHANGED
    2A/09  CAPACITY DATA HAS CHANGED
    38/07  THIN PROVISIONING SOFT THRESHOLD REACHED

All uevent generation is aggregated and rate-limited so that any
individual event is delivered no more than once every 2 seconds.

Log kernel messages when the following Unit Attention ASC/ASCQ
codes are received:

    2A/xx  PARAMETERS CHANGED
    3F/03  INQUIRY DATA HAS CHANGED
    3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED

Also changed the kernel log messages on existing Unit Attention
codes, to remove text that indicates that the conditions were not
handled in any way (since they are now handled in some way) and
reflect the wording used in SPC-4.

Also log a kernel message when the a scsi device reports a Unit
Attention queue overflow, indicating that status has been lost.

These changes are only enabled when the kernel config option
CONFIG_SCSI_ENHANCED_UA is set.  Otherwise, the behavior is the
same as before, including the existing kernel log messages.
However, the detection of these conditions was moved to be earlier
in scsi_check_sense(), because the code was not always reached.

Added a new exported function scsi_report_sense() to allow drivers
to report sense data that is not associated with a SCSI command.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_error.c  | 187 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_lib.c    |  17 ++++-
 drivers/scsi/scsi_priv.h   |   2 +
 drivers/scsi/scsi_scan.c   |   5 ++
 drivers/scsi/scsi_sysfs.c  |   3 +
 include/scsi/scsi_cmnd.h   |   4 +
 include/scsi/scsi_device.h |  22 ++++++
 include/scsi/scsi_eh.h     |   5 ++
 8 files changed, 223 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d0f71e5..d0b5a26 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -223,6 +223,86 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
 #endif
 
 /**
+ * scsi_report_sense - Examine scsi sense information and log messages for
+ *		       certain conditions, also issue uevents if so configured.
+ * @sshdr:	sshdr to be examined
+ */
+void scsi_report_sense(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
+{
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	if (sshdr->ua_queue_overflow)
+		sdev_printk(KERN_WARNING, sdev,
+			    "Unit Attention queue overflow");
+#endif
+
+	if (sshdr->sense_key == UNIT_ATTENTION) {
+		if (sshdr->asc == 0x3f && sshdr->ascq == 0x03)
+			sdev_printk(KERN_WARNING, sdev,
+				    "Inquiry data has changed");
+		else if (sshdr->asc == 0x3f && sshdr->ascq == 0x0e) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			struct scsi_target *starget = scsi_target(sdev);
+			if (atomic_xchg(&starget->lun_change_reported, 1) == 0)
+				schedule_delayed_work(&starget->ua_dwork, 2*HZ);
+			sdev_printk(KERN_WARNING, sdev,
+				    "Reported LUNs data has changed");
+#else
+			sdev_printk(KERN_WARNING, sdev,
+				    "Warning! Received an indication that the "
+				    "LUN assignments on this target have "
+				    "changed. The Linux SCSI layer does not "
+				    "automatically remap LUN assignments.\n");
+#endif
+		} else if (sshdr->asc == 0x3f)
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			sdev_printk(KERN_WARNING, sdev,
+				    "Target operating conditions have changed");
+#else
+			sdev_printk(KERN_WARNING, sdev,
+				    "Warning! Received an indication that the "
+				    "operating parameters on this target have "
+				    "changed. The Linux SCSI layer does not "
+				    "automatically adjust these parameters.\n");
+#endif
+
+		if (sshdr->asc == 0x38 && sshdr->ascq == 0x07) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			if (atomic_xchg(&sdev->soft_threshold_reached, 1) == 0)
+				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
+			sdev_printk(KERN_WARNING, sdev,
+				    "Thin provisioning soft threshold reached");
+#else
+			sdev_printk(KERN_WARNING, sdev,
+				    "Warning! Received an indication that the "
+				    "LUN reached a thin provisioning soft "
+				    "threshold.\n");
+#endif
+		}
+
+		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			if (atomic_xchg(&sdev->mode_parameter_change_reported,
+					1) == 0)
+				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
+#endif
+			sdev_printk(KERN_WARNING, sdev,
+				    "Mode parameters changed");
+		} else if (sshdr->asc == 0x2a && sshdr->ascq == 0x09) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			if (atomic_xchg(&sdev->capacity_change_reported,
+					1) == 0)
+				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
+#endif
+			sdev_printk(KERN_WARNING, sdev,
+				    "Capacity data has changed");
+		} else if (sshdr->asc == 0x2a)
+			sdev_printk(KERN_WARNING, sdev,
+				    "Parameters changed");
+	}
+}
+EXPORT_SYMBOL(scsi_report_sense);
+
+/**
  * scsi_check_sense - Examine scsi cmd sense
  * @scmd:	Cmd to have sense checked.
  *
@@ -241,6 +321,8 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	if (! scsi_command_normalize_sense(scmd, &sshdr))
 		return FAILED;	/* no valid sense data */
 
+	scsi_report_sense(sdev, &sshdr);
+
 	if (scsi_sense_is_deferred(&sshdr))
 		return NEEDS_RETRY;
 
@@ -318,26 +400,6 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		if (scmd->device->allow_restart &&
 		    (sshdr.asc == 0x04) && (sshdr.ascq == 0x02))
 			return FAILED;
-
-		if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e)
-			scmd_printk(KERN_WARNING, scmd,
-				    "Warning! Received an indication that the "
-				    "LUN assignments on this target have "
-				    "changed. The Linux SCSI layer does not "
-				    "automatically remap LUN assignments.\n");
-		else if (sshdr.asc == 0x3f)
-			scmd_printk(KERN_WARNING, scmd,
-				    "Warning! Received an indication that the "
-				    "operating parameters on this target have "
-				    "changed. The Linux SCSI layer does not "
-				    "automatically adjust these parameters.\n");
-
-		if (sshdr.asc == 0x38 && sshdr.ascq == 0x07)
-			scmd_printk(KERN_WARNING, scmd,
-				    "Warning! Received an indication that the "
-				    "LUN reached a thin provisioning soft "
-				    "threshold.\n");
-
 		/*
 		 * Pass the UA upwards for a determination in the completion
 		 * functions.
@@ -380,6 +442,52 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	}
 }
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+/**
+ *	sdev_ua_events - send a uevent for received unit attention events
+ *	@work: work struct for scsi_device
+ *
+ *	Dispatch aggregated events to their associated scsi_device kobjects
+ *	as uevents.
+ */
+void sdev_ua_events(struct work_struct *work)
+{
+	struct scsi_device *sdev;
+
+	sdev = container_of(work, struct scsi_device, ua_dwork.work);
+
+	if (atomic_xchg(&sdev->capacity_change_reported, 0) != 0)
+		sdev_evt_send_simple(sdev, SDEV_EVT_CAPACITY_CHANGE_REPORTED,
+				     GFP_KERNEL);
+	if (atomic_xchg(&sdev->soft_threshold_reached, 0) != 0)
+		sdev_evt_send_simple(sdev, SDEV_EVT_SOFT_THRESHOLD_REACHED,
+				     GFP_KERNEL);
+	if (atomic_xchg(&sdev->mode_parameter_change_reported, 0) != 0)
+		sdev_evt_send_simple(sdev,
+				     SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,
+				     GFP_KERNEL);
+}
+
+/**
+ *	starget_ua_events - send a uevent for received unit attention events
+ *	@work: work struct for scsi_target
+ *
+ *	Dispatch aggregated events to their associated scsi_target kobjects
+ *	as uevents.
+ */
+void starget_ua_events(struct work_struct *work)
+{
+	struct scsi_target *starget;
+
+	starget = container_of(work, struct scsi_target, ua_dwork.work);
+
+	if (atomic_xchg(&starget->lun_change_reported, 0) != 0)
+		starget_evt_send_simple(starget,
+					STARGET_EVT_LUN_CHANGE_REPORTED,
+					GFP_KERNEL);
+}
+#endif
+
 static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 {
 	struct scsi_host_template *sht = sdev->host->hostt;
@@ -2039,6 +2147,10 @@ EXPORT_SYMBOL(scsi_reset_provider);
 int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
                          struct scsi_sense_hdr *sshdr)
 {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	int desc_pos, desc_len, desc_type, addl_len;
+#endif
+
 	if (!sense_buffer || !sb_len)
 		return 0;
 
@@ -2059,8 +2171,41 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 			sshdr->asc = sense_buffer[2];
 		if (sb_len > 3)
 			sshdr->ascq = sense_buffer[3];
-		if (sb_len > 7)
+		if (sb_len > 7) {
 			sshdr->additional_length = sense_buffer[7];
+
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			desc_pos = 8;
+			desc_len = sshdr->additional_length;
+
+			if (desc_len > (sb_len - 8))
+				desc_len = sb_len - 8;
+/*
+ * desc_len should be either 0 or >= 2 here but it might not be, in which case
+ * we can't continue because we can't extract addl_len (and there is no data)
+ */
+			while (desc_len >= 2) {
+				desc_type = sense_buffer[desc_pos];
+				addl_len = sense_buffer[desc_pos + 1];
+
+				if ((sshdr->sense_key == UNIT_ATTENTION) &&
+				    (desc_type == 0x02) && (desc_len >= 5) &&
+				    (addl_len >= 3) &&
+/*
+ * addl_len is supposed to be 0x6 per the spec but we don't actually need that
+ * here, we just need it to be long enough so we can examine [desc_pos + 4]
+ */
+				    ((sense_buffer[desc_pos + 4] & 0x01) != 0))
+					sshdr->ua_queue_overflow = 1;
+
+				if (addl_len > (desc_len - 2))
+					addl_len = desc_len - 2;
+
+				desc_pos += (2 + addl_len);
+				desc_len -= (2 + addl_len);
+			}
+#endif
+		}
 	} else {
 		/*
 		 * fixed format
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c55eea1..70503d2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2186,7 +2186,17 @@ static void sdev_evt_emit(struct scsi_device *sdev, struct sdev_event *evt)
 	case SDEV_EVT_MEDIA_CHANGE:
 		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
 		break;
-
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_CAPACITY_CHANGE_REPORTED=1";
+		break;
+	case SDEV_EVT_SOFT_THRESHOLD_REACHED:
+		envp[idx++] = "SDEV_SOFT_THRESHOLD_REACHED=1";
+		break;
+	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_MODE_PARAMETER_CHANGE_REPORTED=1";
+		break;
+#endif
 	default:
 		/* do nothing */
 		break;
@@ -2280,6 +2290,11 @@ struct sdev_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 	/* evt_type-specific initialization, if any */
 	switch (evt_type) {
 	case SDEV_EVT_MEDIA_CHANGE:
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+	case SDEV_EVT_SOFT_THRESHOLD_REACHED:
+	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
+#endif
 	default:
 		/* do nothing */
 		break;
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 5da5466..b237c8e 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -92,7 +92,9 @@ struct request;
 extern struct kmem_cache *scsi_sdb_cache;
 extern void sdev_evt_work(struct work_struct *work);
 #ifdef CONFIG_SCSI_ENHANCED_UA
+extern void sdev_ua_events(struct work_struct *work);
 extern void starget_evt_work(struct work_struct *work);
+extern void starget_ua_events(struct work_struct *work);
 #endif
 
 /* scsi_proc.c */
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 2d21597..1ad4287 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -268,6 +268,9 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	spin_lock_init(&sdev->list_lock);
 	INIT_WORK(&sdev->event_work, sdev_evt_work);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	INIT_DELAYED_WORK(&sdev->ua_dwork, sdev_ua_events);
+#endif
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
 	sdev->sdev_target = starget;
@@ -429,6 +432,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	INIT_LIST_HEAD(&starget->event_list);
 	spin_lock_init(&starget->list_lock);
 	INIT_WORK(&starget->event_work, starget_evt_work);
+	INIT_DELAYED_WORK(&starget->ua_dwork, starget_ua_events);
 #endif
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
@@ -481,6 +485,7 @@ static void scsi_target_reap_usercontext(struct work_struct *work)
 	struct list_head *this, *tmp;
 
 	cancel_work_sync(&starget->event_work);
+	cancel_delayed_work_sync(&starget->ua_dwork);
 
 	list_for_each_safe(this, tmp, &starget->event_list) {
 		struct starget_event *evt;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 686211e..ac0e96f 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -351,6 +351,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	cancel_delayed_work_sync(&sdev->ua_dwork);
+#endif
 
 	list_for_each_safe(this, tmp, &sdev->event_list) {
 		struct sdev_event *evt;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 1e11985..4cb7a5e 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -103,7 +103,11 @@ struct scsi_cmnd {
 	struct request *request;	/* The command we are
 				   	   working on */
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+#define SCSI_SENSE_BUFFERSIZE	252
+#else
 #define SCSI_SENSE_BUFFERSIZE 	96
+#endif
 	unsigned char *sense_buffer;
 				/* obtained by REQUEST SENSE when
 				 * CHECK CONDITION is received on original
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 69cbd7e..bc660ed 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -51,8 +51,16 @@ enum scsi_device_state {
 
 enum scsi_device_event {
 	SDEV_EVT_MEDIA_CHANGE	= 1,	/* media has changed */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	SDEV_EVT_CAPACITY_CHANGE_REPORTED	= 2,	/* UA reported */
+	SDEV_EVT_SOFT_THRESHOLD_REACHED	= 3,	/* UA soft threshold reached */
+	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED	= 4,	/* UA reported */
 
+	SDEV_EVT_LAST		= SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,
+#else
 	SDEV_EVT_LAST		= SDEV_EVT_MEDIA_CHANGE,
+#endif
+
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
 
@@ -152,6 +160,16 @@ struct scsi_device {
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	atomic_t capacity_change_reported;	/* Device has reported that
+						   capacity has changed */
+	atomic_t soft_threshold_reached;	/* Device has reported that
+						   the thin provisioning soft
+						   threshold has been reached */
+	atomic_t mode_parameter_change_reported;/* Device has reported that
+						   mode parameter has changed */
+	struct delayed_work ua_dwork;
+#endif
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
@@ -270,6 +288,10 @@ struct scsi_target {
 	unsigned int		no_report_luns:1;	/* Don't use
 						 * REPORT LUNS for scanning. */
 #ifdef CONFIG_SCSI_ENHANCED_UA
+	atomic_t	lun_change_reported;	/* Target has reported that
+						   LUNs have changed */
+	struct delayed_work ua_dwork;
+
 	DECLARE_BITMAP(supported_events, STARGET_EVT_MAXBITS);
 	struct list_head	event_list;	/* asserted events */
 	struct work_struct	event_work;
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 06a8790..b6c4d3d 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -25,6 +25,9 @@ struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
 	u8 byte5;
 	u8 byte6;
 	u8 additional_length;	/* always 0 for fixed sense format */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	unsigned int ua_queue_overflow:1;	/* UA info lost by device */
+#endif
 };
 
 static inline int scsi_sense_valid(struct scsi_sense_hdr *sshdr)
@@ -42,6 +45,8 @@ extern void scsi_eh_flush_done_q(struct list_head *done_q);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);
+extern void scsi_report_sense(struct scsi_device *sdev,
+			      struct scsi_sense_hdr *sshdr);
 extern int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 		struct scsi_sense_hdr *sshdr);
 extern int scsi_command_normalize_sense(struct scsi_cmnd *cmd,
-- 
1.7.11.7

[PATCH v3 5/6] [SCSI] Add sysfs support for enhanced Unit Attention handling

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added support for additional scsi_device events in sysfs, as
well as support for scsi_target events.  Also added "rescan"
node in scsi_target sysfs to permit targets to be rescanned
from udev rules in a more straightforward way.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_priv.h  |   4 +-
 drivers/scsi/scsi_scan.c  |  29 +---------
 drivers/scsi/scsi_sysfs.c | 139 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 135 insertions(+), 37 deletions(-)

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index b237c8e..fa3366a97 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -136,7 +136,9 @@ extern int scsi_sysfs_add_host(struct Scsi_Host *);
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
 extern void scsi_sysfs_device_initialize(struct scsi_device *);
-extern int scsi_sysfs_target_initialize(struct scsi_device *);
+extern void scsi_sysfs_target_initialize(struct scsi_target *,
+					 struct Scsi_Host *,
+					 struct device *parent);
 extern struct scsi_transport_template blank_transport_template;
 extern void __scsi_remove_device(struct scsi_device *);
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 1ad4287..1bbbc43 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -346,25 +346,6 @@ static void scsi_target_destroy(struct scsi_target *starget)
 	put_device(dev);
 }
 
-static void scsi_target_dev_release(struct device *dev)
-{
-	struct device *parent = dev->parent;
-	struct scsi_target *starget = to_scsi_target(dev);
-
-	kfree(starget);
-	put_device(parent);
-}
-
-static struct device_type scsi_target_type = {
-	.name =		"scsi_target",
-	.release =	scsi_target_dev_release,
-};
-
-int scsi_is_target_device(const struct device *dev)
-{
-	return dev->type == &scsi_target_type;
-}
-EXPORT_SYMBOL(scsi_is_target_device);
 
 static struct scsi_target *__scsi_find_target(struct device *parent,
 					      int channel, uint id)
@@ -416,15 +397,11 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 		printk(KERN_ERR "%s: allocation failure\n", __func__);
 		return NULL;
 	}
-	dev = &starget->dev;
-	device_initialize(dev);
-	starget->reap_ref = 1;
-	dev->parent = get_device(parent);
-	dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
-	dev->bus = &scsi_bus_type;
-	dev->type = &scsi_target_type;
 	starget->id = id;
 	starget->channel = channel;
+	scsi_sysfs_target_initialize(starget, shost, parent);
+	dev = &starget->dev;
+	starget->reap_ref = 1;
 	starget->can_queue = 0;
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ac0e96f..2a4f2c8 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -502,7 +502,7 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 {									\
 	int ret;							\
 	struct scsi_device *sdev;					\
-	ret = scsi_sdev_check_buf_bit(buf);				\
+	ret = scsi_sysfs_check_buf_bit(buf);				\
 	if (ret >= 0)	{						\
 		sdev = to_scsi_device(dev);				\
 		sdev->field = ret;					\
@@ -513,10 +513,10 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
 
 /*
- * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
+ * scsi_sysfs_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
  * else return -EINVAL.
  */
-static int scsi_sdev_check_buf_bit(const char *buf)
+static int scsi_sysfs_check_buf_bit(const char *buf)
 {
 	if ((buf[1] == '\0') || ((buf[1] == '\n') && (buf[2] == '\0'))) {
 		if (buf[0] == '1')
@@ -650,7 +650,8 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL);
 
 static ssize_t
-show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
+show_iostat_counterbits(struct device *dev, struct device_attribute *attr,
+			char *buf)
 {
 	return snprintf(buf, 20, "%d\n", (int)sizeof(atomic_t) * 8);
 }
@@ -681,7 +682,7 @@ sdev_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR(modalias, S_IRUGO, sdev_show_modalias, NULL);
 
-#define DECLARE_EVT_SHOW(name, Cap_name)				\
+#define DECLARE_SDEV_EVT_SHOW(name, Cap_name)				\
 static ssize_t								\
 sdev_show_evt_##name(struct device *dev, struct device_attribute *attr,	\
 		     char *buf)						\
@@ -691,7 +692,7 @@ sdev_show_evt_##name(struct device *dev, struct device_attribute *attr,	\
 	return snprintf(buf, 20, "%d\n", val);				\
 }
 
-#define DECLARE_EVT_STORE(name, Cap_name)				\
+#define DECLARE_SDEV_EVT_STORE(name, Cap_name)				\
 static ssize_t								\
 sdev_store_evt_##name(struct device *dev, struct device_attribute *attr,\
 		      const char *buf, size_t count)			\
@@ -707,14 +708,19 @@ sdev_store_evt_##name(struct device *dev, struct device_attribute *attr,\
 	return count;							\
 }
 
-#define DECLARE_EVT(name, Cap_name)					\
-	DECLARE_EVT_SHOW(name, Cap_name)				\
-	DECLARE_EVT_STORE(name, Cap_name)				\
+#define DECLARE_SDEV_EVT(name, Cap_name)				\
+	DECLARE_SDEV_EVT_SHOW(name, Cap_name)				\
+	DECLARE_SDEV_EVT_STORE(name, Cap_name)				\
 	static DEVICE_ATTR(evt_##name, S_IRUGO, sdev_show_evt_##name,	\
 			   sdev_store_evt_##name);
 #define REF_EVT(name) &dev_attr_evt_##name.attr
 
-DECLARE_EVT(media_change, MEDIA_CHANGE)
+DECLARE_SDEV_EVT(media_change, MEDIA_CHANGE)
+#ifdef CONFIG_SCSI_ENHANCED_UA
+DECLARE_SDEV_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
+DECLARE_SDEV_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED)
+DECLARE_SDEV_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
+#endif
 
 /* Default template for device attributes.  May NOT be modified */
 static struct attribute *scsi_sdev_attrs[] = {
@@ -734,6 +740,11 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_ioerr_cnt.attr,
 	&dev_attr_modalias.attr,
 	REF_EVT(media_change),
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	REF_EVT(capacity_change_reported),
+	REF_EVT(soft_threshold_reached),
+	REF_EVT(mode_parameter_change_reported),
+#endif
 	NULL
 };
 
@@ -1124,6 +1135,114 @@ int scsi_is_sdev_device(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_sdev_device);
 
+static ssize_t
+starget_store_rescan_field(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct scsi_target *starget = to_scsi_target(dev);
+
+	scsi_scan_target(starget->dev.parent, starget->channel, starget->id,
+			 SCAN_WILD_CARD, 1);
+	return count;
+}
+/* DEVICE_ATTR(rescan) clashes with dev_attr_rescan for sdev */
+struct device_attribute dev_attr_trescan =
+	__ATTR(rescan, S_IWUSR, NULL, starget_store_rescan_field);
+
+#define DECLARE_STARGET_EVT_SHOW(name, Cap_name)			\
+static ssize_t								\
+starget_show_evt_##name(struct device *dev,				\
+			struct device_attribute *attr, char *buf)	\
+{									\
+	struct scsi_target *starget = to_scsi_target(dev);		\
+	int val = test_bit(STARGET_EVT_##Cap_name,			\
+			   starget->supported_events);			\
+	return snprintf(buf, 20, "%d\n", val);				\
+}
+
+#define DECLARE_STARGET_EVT_STORE(name, Cap_name)			\
+static ssize_t								\
+starget_store_evt_##name(struct device *dev,				\
+			 struct device_attribute *attr,			\
+			 const char *buf, size_t count)			\
+{									\
+	struct scsi_target *starget = to_scsi_target(dev);		\
+	unsigned int val;						\
+	int err = kstrtouint(buf, 10, &val);				\
+	if (err != 0)							\
+		return err;						\
+	if (val == 0)							\
+		clear_bit(STARGET_EVT_##Cap_name,			\
+			  starget->supported_events);			\
+	else if (val == 1)						\
+		set_bit(STARGET_EVT_##Cap_name,				\
+			starget->supported_events);			\
+	else								\
+		return -EINVAL;						\
+	return count;							\
+}
+
+#define DECLARE_STARGET_EVT(name, Cap_name)				\
+	DECLARE_STARGET_EVT_SHOW(name, Cap_name)			\
+	DECLARE_STARGET_EVT_STORE(name, Cap_name)			\
+	static DEVICE_ATTR(evt_##name, S_IRUGO,				\
+			   starget_show_evt_##name,			\
+			   starget_store_evt_##name);
+
+#ifdef CONFIG_SCSI_ENHANCED_UA
+DECLARE_STARGET_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
+#endif
+
+static struct attribute *scsi_target_attrs[] = {
+	&dev_attr_trescan.attr,
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	REF_EVT(lun_change_reported),
+#endif
+	NULL
+};
+
+static struct attribute_group scsi_target_attr_group = {
+	.attrs =	scsi_target_attrs,
+};
+
+static const struct attribute_group *scsi_target_attr_groups[] = {
+	&scsi_target_attr_group,
+	NULL
+};
+
+static void scsi_target_dev_release(struct device *dev)
+{
+	struct device *parent = dev->parent;
+	struct scsi_target *starget = to_scsi_target(dev);
+
+	kfree(starget);
+	put_device(parent);
+}
+
+static struct device_type scsi_target_type = {
+	.name =		"scsi_target",
+	.release =	scsi_target_dev_release,
+	.groups =	scsi_target_attr_groups,
+};
+
+int scsi_is_target_device(const struct device *dev)
+{
+	return dev->type == &scsi_target_type;
+}
+EXPORT_SYMBOL(scsi_is_target_device);
+
+void scsi_sysfs_target_initialize(struct scsi_target *starget,
+				  struct Scsi_Host *shost,
+				  struct device *parent)
+{
+	device_initialize(&starget->dev);
+	starget->dev.parent = get_device(parent);
+	starget->dev.bus = &scsi_bus_type;
+	starget->dev.type = &scsi_target_type;
+	dev_set_name(&starget->dev, "target%d:%d:%d", shost->host_no,
+		     starget->channel, starget->id);
+}
+
 /* A blank transport template that is used in drivers that don't
  * yet implement Transport Attributes */
 struct scsi_transport_template blank_transport_template = { { { {NULL, }, }, }, };
-- 
1.7.11.7

[PATCH v3 6/6] [SCSI] Add sense and Unit Attention generation to scsi_debug

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added capability to scsi_debug to generate sense and Unit Attention
conditions to exercise the enhanced sense and Unit Attention handling.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_debug.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..1d8c2bb 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -230,6 +230,7 @@ struct sdebug_dev_info {
 	char reset;
 	char stopped;
 	char used;
+	char sense_pending;
 };
 
 struct sdebug_host_info {
@@ -3050,10 +3051,27 @@ static ssize_t sdebug_max_luns_store(struct device_driver * ddp,
 				     const char * buf, size_t count)
 {
         int n;
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+#endif
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		scsi_debug_max_luns = n;
 		sdebug_max_tgts_luns();
+
+#ifdef CONFIG_SCSI_ENHANCED_UA
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x3f, 0x0e);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+#endif
 		return count;
 	}
 	return -EINVAL;
@@ -3100,12 +3118,28 @@ static ssize_t sdebug_virtual_gb_store(struct device_driver * ddp,
 				       const char * buf, size_t count)
 {
         int n;
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+#endif
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		scsi_debug_virtual_gb = n;
 
 		sdebug_capacity = get_sdebug_capacity();
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x2a, 0x09);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+#endif
 		return count;
 	}
 	return -EINVAL;
@@ -3206,6 +3240,89 @@ static ssize_t sdebug_map_show(struct device_driver *ddp, char *buf)
 DRIVER_ATTR(map, S_IRUGO, sdebug_map_show, NULL);
 
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+static ssize_t sdebug_ua_overflow_store(struct device_driver *ddp,
+					const char *buf, size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0) &&
+            scsi_debug_dsense) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0, 0);
+				devip->sense_buff[7] = 8;
+				devip->sense_buff[8] = 0x02;
+				devip->sense_buff[9] = 0x06;
+				devip->sense_buff[12] = 0x01;
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(ua_overflow, S_IWUSR, NULL, sdebug_ua_overflow_store);
+
+static ssize_t sdebug_soft_threshold_reached_store(struct device_driver *ddp,
+						   const char *buf,
+						   size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x38, 0x07);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(soft_threshold_reached, S_IWUSR, NULL,
+	    sdebug_soft_threshold_reached_store);
+
+static ssize_t sdebug_mode_parameters_changed_store(struct device_driver *ddp,
+						    const char *buf,
+						    size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x2a, 0x01);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(mode_parameters_changed, S_IWUSR, NULL,
+	    sdebug_mode_parameters_changed_store);
+#endif
+
 /* Note: The following function creates attribute files in the
    /sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
    files (over those found in the /sys/module/scsi_debug/parameters
@@ -3239,11 +3356,27 @@ static int do_create_driverfs_files(void)
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_guard);
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_ato);
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_map);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_ua_overflow);
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_soft_threshold_reached);
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_mode_parameters_changed);
+#endif
 	return ret;
 }
 
 static void do_remove_driverfs_files(void)
 {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_mode_parameters_changed);
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_soft_threshold_reached);
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_ua_overflow);
+#endif
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_map);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_ato);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_guard);
@@ -3926,6 +4059,11 @@ write:
 		errsts = check_condition_result;
 		break;
 	}
+	if (!errsts && devip->sense_pending && (*cmd != INQUIRY) &&
+	    (*cmd != REPORT_LUNS)) {
+		devip->sense_pending = 0;
+		errsts = check_condition_result;
+	}
 	return schedule_resp(SCpnt, devip, done, errsts,
 			     (delay_override ? 0 : scsi_debug_delay));
 }
-- 
1.7.11.7

Re: [PATCH v3 3/6] [SCSI] Add support for scsi_target events

[Bart Van Assche]

On 06/19/13 19:42, Ewan D. Milne wrote:
> +static void starget_evt_emit(struct scsi_target *starget,
> +			     struct starget_event *evt)
> +{
> +	int idx = 0;
> +	char *envp[3];
> +
> +	switch (evt->evt_type) {
> +	case STARGET_EVT_LUN_CHANGE_REPORTED:
> +		envp[idx++] = "STARGET_LUN_CHANGE_REPORTED=1";
> +		break;
> +	default:
> +		/* do nothing */
> +		break;
> +	}
> +
> +	envp[idx++] = NULL;
> +
> +	kobject_uevent_env(&starget->dev.kobj, KOBJ_CHANGE, envp);
> +}

Sorry but it's not clear to me why the envp[] array has size three while 
at most two entries are used ? And shouldn't that array be declared as 
const char*[] instead of char *[] since string constants have type const 
char[] ?

> +		list_for_each_safe(this, tmp, &event_list) {
> +			evt = list_entry(this, struct starget_event, node);

Any reason why list_for_each_entry_safe() has not been used here ?

> +void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&starget->list_lock, flags);
> +	list_add_tail(&evt->node, &starget->event_list);
> +	schedule_work(&starget->event_work);
> +	spin_unlock_irqrestore(&starget->list_lock, flags);
> +}

Is it necessary here to invoke schedule_work() under protection of 
list_lock, or would it be safe to invoke schedule_work() after the 
spin_unlock_irqrestore() ?

> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	struct list_head *this, *tmp;
> +
> +	cancel_work_sync(&starget->event_work);
> +
> +	list_for_each_safe(this, tmp, &starget->event_list) {
> +		struct starget_event *evt;
>
> +		evt = list_entry(this, struct starget_event, node);
> +		list_del(&evt->node);
> +		kfree(evt);
> +	}
> +#endif

Same question here: any reason why list_for_each_entry_safe() has not 
been used ?

Thanks,

Bart.

Re: [PATCH v3 1/6] [SCSI] Add a kernel config option for enhanced Unit Attention support

[James Bottomley]

On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> Added CONFIG_SCSI_ENHANCED_UA kernel config option to enable changes
> in the SCSI mid-layer which detect and report certain Unit Attention
> conditions reported by devices.  These changes are primarily useful
> when storage arrays that can be reconfigured are being used, so the
> config option would normally not be used unless it was needed.
> 
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>  drivers/scsi/Kconfig | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index e955978..5d1e614 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -280,6 +280,18 @@ config SCSI_WAIT_SCAN
>  # disabling it, whereas people who accidentally switch it off may wonder why
>  # their mkinitrd gets into trouble.
>  
> +config SCSI_ENHANCED_UA
> +       bool "Enhanced SCSI Unit Attention handling"
> +       depends on SCSI
> +       help
> +	 Certain SCSI devices report changes via a UNIT ATTENTION code.
> +	 (For example, the addition or removal of LUNs from a target, or
> +	 changing the number of logical blocks on a LUN.)  This option
> +	 enables reporting of these changes via udev events, so that the
> +	 device can be rescanned to find out what has changed.  This is
> +	 primarily useful when storage arrays that can be reconfigured
> +	 are attached to the system, otherwise you can say N here.
> +

I don't think we need this.  If we're going to do device events, we
should do it unconditionally.  The recourse for not wanting them is not
listening for them.  That way we don't have two separate config branches
to maintain, one of which will surely bitrot.

James

Re: [PATCH v3 2/6] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[James Bottomley]

On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> The names of the struct and some of the functions for scsi_device
> events are too generic and do not match the comments in the source.
> Changed all of the names to begin with sdev_ in order to avoid
> naming issues and confusion with scsi_target events to be added.
> Also changed name of sdev_evt_thread() to sdev_evt_work().

I don't really understand the rationale here.  Our usual namespace
prefix is scsi_ although we don't obey it universally, of course.  sdev_
isn't one of our namespace prefixes.  we might use scsi_device_ instead,
but I really don't see the need.  Plus all the name changes makes code
really difficult to review.

The two different event structures you introduce are actually identical
except for the values of the enums, so I don't see a need to have them
as separate.  Since the event hangs off a list in the scsi_device, it
can do the same for a scsi_target.  Events actually fire on generic
devices, so that's probably where we should start: change scsi_evt_emit
to take a generic device.  Then you can actually do a static translation
array between the enumerated event and the environment string.  I think
this unification will drastically reduce the number of lines in this
patch, since most of the infrastructure is now reused instead of being
duplicated.

The only other design point I'd add is that we probably need an internal
way to listen for events (I can see dm wanting this), so probably the
scsi_evt_emit should also send the event down a notifier chain, since
kernel internal stuff can't listen for a kobject uevent.  I cc'd the
dm-devel list to see what their opinion is of this.

For them, you should probably also summarise what events we're actually
proposing to send

scsi_target: 

REPORTED LUNS DATA HAS CHANGED

scsi_device:

MODE PARAMETERS CHANGED
CAPACITY DATA HAS CHANGED
THIN PROVISIONING SOFT THRESHOLD REACHED

James

Re: [PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes

[James Bottomley]

On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> Generate a uevent on the scsi_target object when the following
> Unit Attention ASC/ASCQ code is received:
> 
>     3F/0E  REPORTED LUNS DATA HAS CHANGED
> 
> Generate a uevent on the scsi_device object when the following
> Unit Attention ASC/ASCQ codes are received:
> 
>     2A/01  MODE PARAMETERS CHANGED
>     2A/09  CAPACITY DATA HAS CHANGED
>     38/07  THIN PROVISIONING SOFT THRESHOLD REACHED
> 
> All uevent generation is aggregated and rate-limited so that any
> individual event is delivered no more than once every 2 seconds.

Why?  What causes you to think these events would be repeated on a
massive scale.  Mode and Capacity changes are signalled only once per
actual change, which doesn't occur very often.  SBC-3 says that the TP
thresholds are only signalled once but may be signalled again after a
reset.  In general, T10 treats UA as exceptional conditions ... there's
no reason to think they keep repeating.

Dumping all the hand rolled rate limit code would simplify the patch
quite a lot.

> Log kernel messages when the following Unit Attention ASC/ASCQ
> codes are received:
> 
>     2A/xx  PARAMETERS CHANGED
>     3F/03  INQUIRY DATA HAS CHANGED
>     3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED
> 
> Also changed the kernel log messages on existing Unit Attention
> codes, to remove text that indicates that the conditions were not
> handled in any way (since they are now handled in some way) and
> reflect the wording used in SPC-4.

This isn't a good idea because

     1. They might not be acted on if there's no actual listener for the
        uevent
     2. Anyone currently using a log message reaper to process the event
        (which is currently the only way of doing it) would now miss it
        because the log message has changed.

> Also log a kernel message when the a scsi device reports a Unit
> Attention queue overflow, indicating that status has been lost.
> 
> These changes are only enabled when the kernel config option
> CONFIG_SCSI_ENHANCED_UA is set.  Otherwise, the behavior is the
> same as before, including the existing kernel log messages.
> However, the detection of these conditions was moved to be earlier
> in scsi_check_sense(), because the code was not always reached.
> 
> Added a new exported function scsi_report_sense() to allow drivers
> to report sense data that is not associated with a SCSI command.

No: we don't add APIs until there are consumers.  The refactor into
scsi_report_sense is fine, just don't add the export or prototype until
someone comes along with a use case.

James

Re: [PATCH v3 3/6] [SCSI] Add support for scsi_target events

[Ewan Milne]

On Wed, 2013-06-19 at 19:54 +0200, Bart Van Assche wrote:
> On 06/19/13 19:42, Ewan D. Milne wrote:
> > +static void starget_evt_emit(struct scsi_target *starget,
> > +			     struct starget_event *evt)
> > +{
> > +	int idx = 0;
> > +	char *envp[3];
> > +
> > +	switch (evt->evt_type) {
> > +	case STARGET_EVT_LUN_CHANGE_REPORTED:
> > +		envp[idx++] = "STARGET_LUN_CHANGE_REPORTED=1";
> > +		break;
> > +	default:
> > +		/* do nothing */
> > +		break;
> > +	}
> > +
> > +	envp[idx++] = NULL;
> > +
> > +	kobject_uevent_env(&starget->dev.kobj, KOBJ_CHANGE, envp);
> > +}
> 
> Sorry but it's not clear to me why the envp[] array has size three while 
> at most two entries are used ? And shouldn't that array be declared as 
> const char*[] instead of char *[] since string constants have type const 
> char[] ?

Hmm.  I copied the code from the (renamed) scsi_evt_emit() which had
char *envp[3], but you are right, only 2 entries are needed.  As far
as the const attribute goes, the signature of kobject_uevent_env() in
include/linux/kobject.h doesn't have the const attribute on the envp
parameter, and other callers don't use const.

> 
> > +		list_for_each_safe(this, tmp, &event_list) {
> > +			evt = list_entry(this, struct starget_event, node);
> 
> Any reason why list_for_each_entry_safe() has not been used here ?

No particular reason, again the code was copied from the (renamed)
sdev_evt_thread(). 

> 
> > +void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&starget->list_lock, flags);
> > +	list_add_tail(&evt->node, &starget->event_list);
> > +	schedule_work(&starget->event_work);
> > +	spin_unlock_irqrestore(&starget->list_lock, flags);
> > +}
> 
> Is it necessary here to invoke schedule_work() under protection of 
> list_lock, or would it be safe to invoke schedule_work() after the 
> spin_unlock_irqrestore() ?

I think the call to schedule_work() can be moved outside, the lock, I'll
have to verify that.

> 
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +	struct list_head *this, *tmp;
> > +
> > +	cancel_work_sync(&starget->event_work);
> > +
> > +	list_for_each_safe(this, tmp, &starget->event_list) {
> > +		struct starget_event *evt;
> >
> > +		evt = list_entry(this, struct starget_event, node);
> > +		list_del(&evt->node);
> > +		kfree(evt);
> > +	}
> > +#endif
> 
> Same question here: any reason why list_for_each_entry_safe() has not 
> been used ?

Same answer:  the code came from scsi_device_dev_release_usercontext().

I see that James would prefer that the events be unified rather than
specific to scsi_device or scsi_target, so I'll remove the duplication.

Thanks for the comments.

-Ewan

> 
> Thanks,
> 
> Bart.

Re: [PATCH v3 1/6] [SCSI] Add a kernel config option for enhanced Unit Attention support

[Ewan Milne]

On Wed, 2013-06-19 at 18:35 +0000, James Bottomley wrote:
> On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> > 
> > Added CONFIG_SCSI_ENHANCED_UA kernel config option to enable changes
> > in the SCSI mid-layer which detect and report certain Unit Attention
> > conditions reported by devices.  These changes are primarily useful
> > when storage arrays that can be reconfigured are being used, so the
> > config option would normally not be used unless it was needed.
> > 
> > Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> > ---
> >  drivers/scsi/Kconfig | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > index e955978..5d1e614 100644
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -280,6 +280,18 @@ config SCSI_WAIT_SCAN
> >  # disabling it, whereas people who accidentally switch it off may wonder why
> >  # their mkinitrd gets into trouble.
> >  
> > +config SCSI_ENHANCED_UA
> > +       bool "Enhanced SCSI Unit Attention handling"
> > +       depends on SCSI
> > +       help
> > +	 Certain SCSI devices report changes via a UNIT ATTENTION code.
> > +	 (For example, the addition or removal of LUNs from a target, or
> > +	 changing the number of logical blocks on a LUN.)  This option
> > +	 enables reporting of these changes via udev events, so that the
> > +	 device can be rescanned to find out what has changed.  This is
> > +	 primarily useful when storage arrays that can be reconfigured
> > +	 are attached to the system, otherwise you can say N here.
> > +
> 
> I don't think we need this.  If we're going to do device events, we
> should do it unconditionally.  The recourse for not wanting them is not
> listening for them.  That way we don't have two separate config branches
> to maintain, one of which will surely bitrot.

OK.  I added it because I thought this wouldn't be useful for desktop
and/or embedded kernels, but I'm happy if it is not needed.

-Ewan

> 
> James
> 

Re: [PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes

[Hannes Reinecke]

Hi Ewan,

some comments inline:

On 06/19/2013 07:42 PM, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> Generate a uevent on the scsi_target object when the following
> Unit Attention ASC/ASCQ code is received:
> 
>     3F/0E  REPORTED LUNS DATA HAS CHANGED
> 
> Generate a uevent on the scsi_device object when the following
> Unit Attention ASC/ASCQ codes are received:
> 
>     2A/01  MODE PARAMETERS CHANGED
>     2A/09  CAPACITY DATA HAS CHANGED
>     38/07  THIN PROVISIONING SOFT THRESHOLD REACHED
> 
> All uevent generation is aggregated and rate-limited so that any
> individual event is delivered no more than once every 2 seconds.
> 
> Log kernel messages when the following Unit Attention ASC/ASCQ
> codes are received:
> 
>     2A/xx  PARAMETERS CHANGED
>     3F/03  INQUIRY DATA HAS CHANGED
>     3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED
> 
> Also changed the kernel log messages on existing Unit Attention
> codes, to remove text that indicates that the conditions were not
> handled in any way (since they are now handled in some way) and
> reflect the wording used in SPC-4.
> 
> Also log a kernel message when the a scsi device reports a Unit
> Attention queue overflow, indicating that status has been lost.
> 
> These changes are only enabled when the kernel config option
> CONFIG_SCSI_ENHANCED_UA is set.  Otherwise, the behavior is the
> same as before, including the existing kernel log messages.
> However, the detection of these conditions was moved to be earlier
> in scsi_check_sense(), because the code was not always reached.
> 
> Added a new exported function scsi_report_sense() to allow drivers
> to report sense data that is not associated with a SCSI command.
> 
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>  drivers/scsi/scsi_error.c  | 187 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/scsi/scsi_lib.c    |  17 ++++-
>  drivers/scsi/scsi_priv.h   |   2 +
>  drivers/scsi/scsi_scan.c   |   5 ++
>  drivers/scsi/scsi_sysfs.c  |   3 +
>  include/scsi/scsi_cmnd.h   |   4 +
>  include/scsi/scsi_device.h |  22 ++++++
>  include/scsi/scsi_eh.h     |   5 ++
>  8 files changed, 223 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index d0f71e5..d0b5a26 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -223,6 +223,86 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
>  #endif
>  
>  /**
> + * scsi_report_sense - Examine scsi sense information and log messages for
> + *		       certain conditions, also issue uevents if so configured.
> + * @sshdr:	sshdr to be examined
> + */
> +void scsi_report_sense(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
> +{
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	if (sshdr->ua_queue_overflow)
> +		sdev_printk(KERN_WARNING, sdev,
> +			    "Unit Attention queue overflow");
> +#endif
> +
> +	if (sshdr->sense_key == UNIT_ATTENTION) {
> +		if (sshdr->asc == 0x3f && sshdr->ascq == 0x03)
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Inquiry data has changed");
Why no event for this one?
I would think that this event warrants a device rescan, so
definitely would want to be informed here ...

> +		else if (sshdr->asc == 0x3f && sshdr->ascq == 0x0e) {
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +			struct scsi_target *starget = scsi_target(sdev);
> +			if (atomic_xchg(&starget->lun_change_reported, 1) == 0)
> +				schedule_delayed_work(&starget->ua_dwork, 2*HZ);
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Reported LUNs data has changed");
> +#else
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Warning! Received an indication that the "
> +				    "LUN assignments on this target have "
> +				    "changed. The Linux SCSI layer does not "
> +				    "automatically remap LUN assignments.\n");
> +#endif
As James B. said, I would not modify the existing comment.
Just because we did send an event that doesn't mean that someone
actually too some action for that event ...
So I'd prefer to have the original messages in place.

> +		} else if (sshdr->asc == 0x3f)
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Target operating conditions have changed");
> +#else
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Warning! Received an indication that the "
> +				    "operating parameters on this target have "
> +				    "changed. The Linux SCSI layer does not "
> +				    "automatically adjust these parameters.\n");
> +#endif
> +
> +		if (sshdr->asc == 0x38 && sshdr->ascq == 0x07) {
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +			if (atomic_xchg(&sdev->soft_threshold_reached, 1) == 0)
> +				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Thin provisioning soft threshold reached");
> +#else
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Warning! Received an indication that the "
> +				    "LUN reached a thin provisioning soft "
> +				    "threshold.\n");
> +#endif
Again, there is no need to change the message ...

> +		}
> +
> +		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +			if (atomic_xchg(&sdev->mode_parameter_change_reported,
> +					1) == 0)
> +				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
> +#endif
I'm not utterly keen on adding individual variables per event.
This will increase the structure for each new event.

I'd rather have a common variable which could then hold a bitmask of
the events. That way we could easily add more events to it.

[ .. ]
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index c55eea1..70503d2 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2186,7 +2186,17 @@ static void sdev_evt_emit(struct scsi_device *sdev, struct sdev_event *evt)
>  	case SDEV_EVT_MEDIA_CHANGE:
>  		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
>  		break;
> -
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
> +		envp[idx++] = "SDEV_CAPACITY_CHANGE_REPORTED=1";
> +		break;
> +	case SDEV_EVT_SOFT_THRESHOLD_REACHED:
> +		envp[idx++] = "SDEV_SOFT_THRESHOLD_REACHED=1";
> +		break;
> +	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
> +		envp[idx++] = "SDEV_MODE_PARAMETER_CHANGE_REPORTED=1";
> +		break;
> +#endif
Again, I don't really like to have on/off variables for uevents.
This will making it quite hard to code udev rules for it.
I'd rather have a common variable name like SDEV_EVENT
and then the event as the value.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/6] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Ewan Milne]

On Wed, 2013-06-19 at 18:36 +0000, James Bottomley wrote:
> On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> > 
> > The names of the struct and some of the functions for scsi_device
> > events are too generic and do not match the comments in the source.
> > Changed all of the names to begin with sdev_ in order to avoid
> > naming issues and confusion with scsi_target events to be added.
> > Also changed name of sdev_evt_thread() to sdev_evt_work().
> 
> I don't really understand the rationale here.  Our usual namespace
> prefix is scsi_ although we don't obey it universally, of course.  sdev_
> isn't one of our namespace prefixes.  we might use scsi_device_ instead,
> but I really don't see the need.  Plus all the name changes makes code
> really difficult to review.
> 
> The two different event structures you introduce are actually identical
> except for the values of the enums, so I don't see a need to have them
> as separate.  Since the event hangs off a list in the scsi_device, it
> can do the same for a scsi_target.  Events actually fire on generic
> devices, so that's probably where we should start: change scsi_evt_emit
> to take a generic device.  Then you can actually do a static translation
> array between the enumerated event and the environment string.  I think
> this unification will drastically reduce the number of lines in this
> patch, since most of the infrastructure is now reused instead of being
> duplicated.

OK.

> 
> The only other design point I'd add is that we probably need an internal
> way to listen for events (I can see dm wanting this), so probably the
> scsi_evt_emit should also send the event down a notifier chain, since
> kernel internal stuff can't listen for a kobject uevent.  I cc'd the
> dm-devel list to see what their opinion is of this.

Sure.  I thought of doing this, but didn't because I wasn't planning
to change anything else to listen for it.  The other thing is that the
UA typically indicates that something has changed, but another command
is needed to find out exactly *what*, e.g. with the WCE bit that is used
to control flush behavior, a MODE SENSE command is needed.  A rescan of
the device in response to the UA would do that.

> 
> For them, you should probably also summarise what events we're actually
> proposing to send
> 
> scsi_target: 
> 
> REPORTED LUNS DATA HAS CHANGED
> 
> scsi_device:
> 
> MODE PARAMETERS CHANGED
> CAPACITY DATA HAS CHANGED
> THIN PROVISIONING SOFT THRESHOLD REACHED
> 
> James
> 

Re: [PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes

[Ewan Milne]

On Wed, 2013-06-19 at 18:48 +0000, James Bottomley wrote:
> On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> > 
> > Generate a uevent on the scsi_target object when the following
> > Unit Attention ASC/ASCQ code is received:
> > 
> >     3F/0E  REPORTED LUNS DATA HAS CHANGED
> > 
> > Generate a uevent on the scsi_device object when the following
> > Unit Attention ASC/ASCQ codes are received:
> > 
> >     2A/01  MODE PARAMETERS CHANGED
> >     2A/09  CAPACITY DATA HAS CHANGED
> >     38/07  THIN PROVISIONING SOFT THRESHOLD REACHED
> > 
> > All uevent generation is aggregated and rate-limited so that any
> > individual event is delivered no more than once every 2 seconds.
> 
> Why?  What causes you to think these events would be repeated on a
> massive scale.  Mode and Capacity changes are signalled only once per
> actual change, which doesn't occur very often.  SBC-3 says that the TP
> thresholds are only signalled once but may be signalled again after a
> reset.  In general, T10 treats UA as exceptional conditions ... there's
> no reason to think they keep repeating.

Well, the concern I had is that since a UA can theoretically be reported
on every command, a malfunctioning device could quickly overload udevd.
I have seen cases where udevd gets significantly behind when processing
a flood of events, and didn't want to make that worse.  Kay had concerns
about that when Hannes was working on this a while back, I believe.
I also didn't want other events to get lost if UA events filled up the
NL queue to udevd in userspace.

The other thing that aggregation helps with is when every LUN on a
target says REPORTED LUNS DATA HAS CHANGED.  Some storage arrays allow
hundreds of LUNS on a target, and I think they will all report the UA
if the LUN provisioning to the host is changed.  There is a mode that
can be used to suppress this, and only report one UA, but I don't know
if all storage arrays support it.  Now, granted, the UAs will only be
reported by each LUN when they receive a command, so this could happen
at any time in the future, but unfortunately that is the way SCSI works.

Of course, perhaps it would be better to provide a rate limit or
aggregation mechanism in the uevent code, rather than in its callers.
I'm not sure what it would take to get that to happen, I'll look at it.

> 
> Dumping all the hand rolled rate limit code would simplify the patch
> quite a lot.

Yes, it would be nice if there were a way to avoid it.

> 
> > Log kernel messages when the following Unit Attention ASC/ASCQ
> > codes are received:
> > 
> >     2A/xx  PARAMETERS CHANGED
> >     3F/03  INQUIRY DATA HAS CHANGED
> >     3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED
> > 
> > Also changed the kernel log messages on existing Unit Attention
> > codes, to remove text that indicates that the conditions were not
> > handled in any way (since they are now handled in some way) and
> > reflect the wording used in SPC-4.
> 
> This isn't a good idea because
> 
>      1. They might not be acted on if there's no actual listener for the
>         uevent
>      2. Anyone currently using a log message reaper to process the event
>         (which is currently the only way of doing it) would now miss it
>         because the log message has changed.

I'll leave the messages alone, then.  It just looked wierd to me.

> 
> > Also log a kernel message when the a scsi device reports a Unit
> > Attention queue overflow, indicating that status has been lost.
> > 
> > These changes are only enabled when the kernel config option
> > CONFIG_SCSI_ENHANCED_UA is set.  Otherwise, the behavior is the
> > same as before, including the existing kernel log messages.
> > However, the detection of these conditions was moved to be earlier
> > in scsi_check_sense(), because the code was not always reached.
> > 
> > Added a new exported function scsi_report_sense() to allow drivers
> > to report sense data that is not associated with a SCSI command.
> 
> No: we don't add APIs until there are consumers.  The refactor into
> scsi_report_sense is fine, just don't add the export or prototype until
> someone comes along with a use case.

Sure, I'll remove the export.  I added it because an earlier reviewer
asked for it, but they can add the export later if they need it.

Thanks for your comments.  I'll send a revised patch.  I'll need to
think about the rate limit concerns a bit more first.

> 
> James
> 

Re: [PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes

[James Bottomley]

On Mon, 2013-06-24 at 10:11 -0400, Ewan Milne wrote:
> On Wed, 2013-06-19 at 18:48 +0000, James Bottomley wrote:
> > On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> > > From: "Ewan D. Milne" <emilne@redhat.com>
> > > 
> > > Generate a uevent on the scsi_target object when the following
> > > Unit Attention ASC/ASCQ code is received:
> > > 
> > >     3F/0E  REPORTED LUNS DATA HAS CHANGED
> > > 
> > > Generate a uevent on the scsi_device object when the following
> > > Unit Attention ASC/ASCQ codes are received:
> > > 
> > >     2A/01  MODE PARAMETERS CHANGED
> > >     2A/09  CAPACITY DATA HAS CHANGED
> > >     38/07  THIN PROVISIONING SOFT THRESHOLD REACHED
> > > 
> > > All uevent generation is aggregated and rate-limited so that any
> > > individual event is delivered no more than once every 2 seconds.
> > 
> > Why?  What causes you to think these events would be repeated on a
> > massive scale.  Mode and Capacity changes are signalled only once per
> > actual change, which doesn't occur very often.  SBC-3 says that the TP
> > thresholds are only signalled once but may be signalled again after a
> > reset.  In general, T10 treats UA as exceptional conditions ... there's
> > no reason to think they keep repeating.
> 
> Well, the concern I had is that since a UA can theoretically be reported
> on every command, a malfunctioning device could quickly overload udevd.

We had devices in the 1990s that did this ... I haven't seen any for
years.  I take it the qualifier "theoretical" means you haven't actually
seen this behaviour from a current device in the field?

> I have seen cases where udevd gets significantly behind when processing
> a flood of events, and didn't want to make that worse.  Kay had concerns
> about that when Hannes was working on this a while back, I believe.
> I also didn't want other events to get lost if UA events filled up the
> NL queue to udevd in userspace.

The events you're reporting are infrequent in normal operation.  If the
device goes rogue and floods them, udev issues are likely to be the
least of our concerns.

The fact that we may generate a flood because we have a massive number
of LUNs which each report the infrequent event is a concern, but it
should be fixed without rate limiting, see below:

> The other thing that aggregation helps with is when every LUN on a
> target says REPORTED LUNS DATA HAS CHANGED.  Some storage arrays allow
> hundreds of LUNS on a target, and I think they will all report the UA
> if the LUN provisioning to the host is changed.  There is a mode that
> can be used to suppress this, and only report one UA, but I don't know
> if all storage arrays support it.  Now, granted, the UAs will only be
> reported by each LUN when they receive a command, so this could happen
> at any time in the future, but unfortunately that is the way SCSI works.

So fixing this problem is what's needed rather than a generic rate limit
mechanism.  We already have a rudimentary mechanism for suppressing the
flood of UAs we get on target reset ... reuse the same thing to make
sure we only get one REPORTED LUNS DATA HAS CHANGED per target.

Note there are some fixes required to the current mechanism:  Firstly it
should clear expecting_cc_ua on the first successful command that
doesn't return a UA to prevent spurious memory (just in case arrays try
to be clever) with an

if (unlikely(scmd->device->expecting_cc_ua))
        scmd->device->expecting_cc_ua = 0;

just so we don't dirty a cache line if it's unnecessary

> Of course, perhaps it would be better to provide a rate limit or
> aggregation mechanism in the uevent code, rather than in its callers.
> I'm not sure what it would take to get that to happen, I'll look at it.

My point is that once we get to rate limiting, we've lost ... we're
already dropping stuff that may be important, so lets begin without the
ratelimit code and instead fix the problems that may cause data floods
instead.

James

Re: [PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes

[Ewan Milne]

On Mon, 2013-06-24 at 14:58 +0000, James Bottomley wrote:
> On Mon, 2013-06-24 at 10:11 -0400, Ewan Milne wrote:
> > On Wed, 2013-06-19 at 18:48 +0000, James Bottomley wrote:
> > > On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> > > > From: "Ewan D. Milne" <emilne@redhat.com>
> > > > 
> > > > Generate a uevent on the scsi_target object when the following
> > > > Unit Attention ASC/ASCQ code is received:
> > > > 
> > > >     3F/0E  REPORTED LUNS DATA HAS CHANGED
> > > > 
> > > > Generate a uevent on the scsi_device object when the following
> > > > Unit Attention ASC/ASCQ codes are received:
> > > > 
> > > >     2A/01  MODE PARAMETERS CHANGED
> > > >     2A/09  CAPACITY DATA HAS CHANGED
> > > >     38/07  THIN PROVISIONING SOFT THRESHOLD REACHED
> > > > 
> > > > All uevent generation is aggregated and rate-limited so that any
> > > > individual event is delivered no more than once every 2 seconds.
> > > 
> > > Why?  What causes you to think these events would be repeated on a
> > > massive scale.  Mode and Capacity changes are signalled only once per
> > > actual change, which doesn't occur very often.  SBC-3 says that the TP
> > > thresholds are only signalled once but may be signalled again after a
> > > reset.  In general, T10 treats UA as exceptional conditions ... there's
> > > no reason to think they keep repeating.
> > 
> > Well, the concern I had is that since a UA can theoretically be reported
> > on every command, a malfunctioning device could quickly overload udevd.
> 
> We had devices in the 1990s that did this ... I haven't seen any for
> years.  I take it the qualifier "theoretical" means you haven't actually
> seen this behaviour from a current device in the field?

No, you're right, I haven't.  I was just trying to be careful.
If you think it's OK for Mode and Capacity changes to be reported
each time we get a UA, that's fine.

> 
> > I have seen cases where udevd gets significantly behind when processing
> > a flood of events, and didn't want to make that worse.  Kay had concerns
> > about that when Hannes was working on this a while back, I believe.
> > I also didn't want other events to get lost if UA events filled up the
> > NL queue to udevd in userspace.
> 
> The events you're reporting are infrequent in normal operation.  If the
> device goes rogue and floods them, udev issues are likely to be the
> least of our concerns.
> 
> The fact that we may generate a flood because we have a massive number
> of LUNs which each report the infrequent event is a concern, but it
> should be fixed without rate limiting, see below:
> 
> > The other thing that aggregation helps with is when every LUN on a
> > target says REPORTED LUNS DATA HAS CHANGED.  Some storage arrays allow
> > hundreds of LUNS on a target, and I think they will all report the UA
> > if the LUN provisioning to the host is changed.  There is a mode that
> > can be used to suppress this, and only report one UA, but I don't know
> > if all storage arrays support it.  Now, granted, the UAs will only be
> > reported by each LUN when they receive a command, so this could happen
> > at any time in the future, but unfortunately that is the way SCSI works.
> 
> So fixing this problem is what's needed rather than a generic rate limit
> mechanism.  We already have a rudimentary mechanism for suppressing the
> flood of UAs we get on target reset ... reuse the same thing to make
> sure we only get one REPORTED LUNS DATA HAS CHANGED per target.

I looked at doing this, but unfortunately it appears as if it is hard
to know which LUNs are expected to report the REPORTED LUNS DATA HAS
CHANGED if some other LUN has done so.  The difficulty is in the SPC-3
behavior that potentially clears this UA condition on all LUNs that are
accessible on an I_T nexus when a REPORT LUNS command is received.

So, some, but perhaps not all, of the LUNs would report the UA.

Since there isn't a good way to know when a REPORT LUNS command enters
the enabled task state on the device server (as opposed to when it is
sent by the host) relative to when other commands on the various LUNs
are either processed or terminated with the UA, there is the potential
of suppressing a subsequent UA if the LUN inventory changes again.
(The UA should be pending on *some* other LUN(s) in this case, and not
be masked by this logic, but there is no guarantee that those LUNs will
be accessed.)

The suppression of UAs (e.g. 29/00) received following a TARGET RESET
doesn't have this problem, because those UA are only cleared when a
command that is not an INQUIRY, REPORT LUNS, or REQUEST SENSE is
received by each LUN.

I suppose this could be solved by stopping other I/O to the target when
a REPORT LUNS is being issued, but that seems like an invasive change.

Devices conforming to SAM-4 don't have this problem, because they only
return REPORTED LUNS DATA HAS CHANGED on one LUN, but it would be more
useful to make this work with SCSI-3 devices.

> 
> Note there are some fixes required to the current mechanism:  Firstly it
> should clear expecting_cc_ua on the first successful command that
> doesn't return a UA to prevent spurious memory (just in case arrays try
> to be clever) with an
> 
> if (unlikely(scmd->device->expecting_cc_ua))
>         scmd->device->expecting_cc_ua = 0;
> 
> just so we don't dirty a cache line if it's unnecessary

Also, I suspect that it would be necessary to use a separate flag.  If
a LUN has a pending REPORTED LUNS DATA HAS CHANGED unit attention, but
no ordinary commands are issued and then a TARGET RESET is performed,
I'm not sure whether or not the 29/00 UA would be reported first.  It's
supposed to have a higher precedence, but SAM-5 says that the device
server *may* select any UA in the queue when reporting one.

SAM-5 also says that the device server *may* clear lower priority UAs,
which is another problem.  We might never get notification of a change.
It makes it difficult to track the device state on the host.

Given all this, it seemed like rate limiting the uevents was the way to
deal with whatever the device managed to give us.  I'm not enamored with
the idea, though.  Do you think stopping I/O during a REPORT LUNS is a
workable approach?  I'm not sure what else would work reliably.

-Ewan

> 
> > Of course, perhaps it would be better to provide a rate limit or
> > aggregation mechanism in the uevent code, rather than in its callers.
> > I'm not sure what it would take to get that to happen, I'll look at it.
> 
> My point is that once we get to rate limiting, we've lost ... we're
> already dropping stuff that may be important, so lets begin without the
> ratelimit code and instead fix the problems that may cause data floods
> instead.
> 
> James
> 

Re: [PATCH v3 0/6] [SCSI] Enhanced sense and Unit Attention handling

[James Bottomley]

On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
> to provide enhanced support for Unit Attention conditions, as well as
> detection of a unit attention queue overflow condition and the ability
> for drivers to report sense data outside of normal command completion.
> 
> There was some discussion about this a couple of years ago on the linux-scsi
> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
> Although one approach is to send all SCSI sense data to a userspace daemon
> for processing, this patch set does not take that approach due to the
> difficulty in reliably delivering all of the data.  An interesting UA
> condition might not be delivered due to a flood of media errors, for example.
> 
> The mechanism used is to flag when certain UA ASC/ASCQ codes are received
> that report asynchronous changes to the storage device configuration.
> An appropriate uevent is then generated for the scsi_device or scsi_target
> object.  An aggregation mechanism is used to avoid generating uevents at
> too high a rate, and to coalesce multiple UAs reported by LUNs on the
> same target for a REPORTED LUNS DATA HAS CHANGED sense code.
> 
> The changes are enabled by a new kernel config option CONFIG_SCSI_ENHANCED_UA.
> If this config option is not used, no new uevents are generated.  There are
> some changes to kernel logging messages if CONFIG_SCSI_ENHANCED_UA is enabled,
> because the existing messages explicitly stated that the kernel did not do
> anything with the information.
> 
> Note that checkpatch is reporting errors on patch 5/6 relating to macros
> in scsi_sysfs.c -- I believe these errors are incorrect and have sent a
> message to the checkpatch maintainer.  The macros were derived from
> existing ones already in the file.
> 
> Changes made since earlier v2 version:
> 
>    - Remove patch 1/8 "Generate uevent on sd capacity change"
>    - Remove patch 8/8 "Streamline detection of FM/EOM/ILI status"
>    - Changed scsi_debug to not generate UA on INQUIRY or REPORT_LUNS
>    - Changed scsi_debug to only report UA queue overflow condition
>      if dsense=1, as descriptor format sense data is needed
> 
> Changes made since earlier RFC version:
> 
>    - Remove patch 1/9 "Detect overflow of sense data buffer"
>      Some scsi_debug changes in this patch were moved to patch 7/8
>    - Corrected Kconfig help text
>    - Change name of "sdev_evt_thread" to "sdev_evt_work"
>    - Change name of "starget_evt_thread" to "starget_evt_work"
>    - Pull code out of scsi_check_sense() that handles UAs into
>      an exported function so that drivers can report conditions
>      received asynchronously
> 
> Thanks to everyone for the comments on this patch series.
> 
> Ewan D. Milne (6):
>   [SCSI] Add a kernel config option for enhanced Unit Attention support
>   [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to
>     sdev_event
>   [SCSI] Add support for scsi_target events
>   [SCSI] Generate uevents for certain Unit Attention codes
>   [SCSI] Add sysfs support for enhanced Unit Attention handling
>   [SCSI] Add sense and Unit Attention generation to scsi_debug

Ping on this, please.

I have another possible consumer of this infrastructure, when it's
ready, which is the SCSI RAID drivers.  We've been getting complaints
that there's no event we get from them when a RAID system goes from
online -> degraded which should be the sysadmin's cue to go in and
replace the disk (well, this isn't quite true, a lot come with
proprietary monitoring daemons which do this, but they're pretty unique
per controller).

I was thinking we might resurrect the orphaned raid_class.c to do this
and give a universally displayable but rudimentary view of the topology
and health of the device and add an easy hook for RAID events.

James

Re: [PATCH v3 0/6] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Mon, 2013-07-22 at 15:31 +0000, James Bottomley wrote:
> Ping on this, please.
> 
> I have another possible consumer of this infrastructure, when it's
> ready, which is the SCSI RAID drivers.  We've been getting complaints
> that there's no event we get from them when a RAID system goes from
> online -> degraded which should be the sysadmin's cue to go in and
> replace the disk (well, this isn't quite true, a lot come with
> proprietary monitoring daemons which do this, but they're pretty unique
> per controller).
> 
> I was thinking we might resurrect the orphaned raid_class.c to do this
> and give a universally displayable but rudimentary view of the topology
> and health of the device and add an easy hook for RAID events.
> 
> James
> 

I'm testing a v4 version of these patches, which address your earlier
comments.  I'm still working on the code to suppress the duplicate
REPORTED LUNS DATA HAS CHANGED unit attentions from multiple LUNs.
As I mentioned in my earlier mail, doing this with the expected_cc_ua
mechanism is imperfect, because SCSI-3 devices will stop reporting it
when one of the LUNs on the target *receives* a REPORT LUNs command, and
it is difficult to know when this happens.  I'm trying out clearing
the flag before the SCSI scan code issues a REPORT LUNs, that might be
good enough.  (It doesn't handle someone from sending this command
through the sg driver, though.)

Will post the v4 soon.

-Ewan