[PATCH] scsi: Add "missing" sysfs attribute to scsi_device structure

[PATCH] scsi: Add "missing" sysfs attribute to scsi_device structure

[Ewan D. Milne]

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

This change adds a "missing" sysfs attribute to scsi_device
which is set when a previously scanned device no longer appears
in the REPORT LUNS inventory.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_scan.c   | 30 +++++++++++++++++++++++++++++-
 drivers/scsi/scsi_sysfs.c  |  2 ++
 include/scsi/scsi_device.h |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 2e5fe58..7908c06 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1317,9 +1317,11 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	struct scsi_lun *lunp, *lun_data;
 	u8 *data;
 	struct scsi_sense_hdr sshdr;
-	struct scsi_device *sdev;
+	struct scsi_device *sdev, *sdev_i;
 	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 	int ret = 0;
+	unsigned long flags;
+	bool found;
 
 	/*
 	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
@@ -1492,6 +1494,32 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		}
 	}
 
+	/*
+	 * See if all of the LUNs that we know about on the target are
+	 * still being reported in the REPORT LUNS data.  If any are not,
+	 * they have been removed from the target.  Set the "missing" flag.
+	 *
+	 * We only do this for REPORT LUNS scanning, because we do not
+	 * want to set the flag on LUNs that are inaccessible due to a
+	 * transport error.  Here, the target has responded to a command
+	 * and told us that the LUN is not present.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry(sdev_i, &starget->devices, same_target_siblings) {
+		found = 0;
+		for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
+			lun = scsilun_to_int(lunp);
+			if (memcmp(&lunp->scsi_lun[sizeof(lun)], "\0\0\0\0", 4))
+				continue;
+			else if (lun == sdev_i->lun) {
+				found = 1;
+				break;
+			}
+		}
+		sdev_i->missing = !found;
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
  out_err:
 	kfree(lun_data);
  out:
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 04c2a27..01635cc 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -536,6 +536,7 @@ sdev_rd_attr (scsi_level, "%d\n");
 sdev_rd_attr (vendor, "%.8s\n");
 sdev_rd_attr (model, "%.16s\n");
 sdev_rd_attr (rev, "%.4s\n");
+sdev_rd_attr(missing, "%d\n");
 
 /*
  * TODO: can we make these symlinks to the block layer ones?
@@ -721,6 +722,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_vendor.attr,
 	&dev_attr_model.attr,
 	&dev_attr_rev.attr,
+	&dev_attr_missing.attr,
 	&dev_attr_rescan.attr,
 	&dev_attr_delete.attr,
 	&dev_attr_state.attr,
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6efb2e1..79775f1 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -152,6 +152,7 @@ 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 */
+	unsigned missing:1;	/* No longer appears in REPORT LUNS data */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.11.7

Re: [PATCH] scsi: Add "missing" sysfs attribute to scsi_device structure

[James Bottomley]

On Mon, 2013-11-18 at 12:10 -0500, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> This change adds a "missing" sysfs attribute to scsi_device
> which is set when a previously scanned device no longer appears
> in the REPORT LUNS inventory.

A sysfs parameter with time and external input dependent semantics
really doesn't look like a good idea.  What is it you're trying to
achieve that you can't achieve from userspace by watching for the
events?

James

Re: [PATCH] scsi: Add "missing" sysfs attribute to scsi_device structure

[Ewan Milne]

On Mon, 2013-11-18 at 09:45 -0800, James Bottomley wrote:
> On Mon, 2013-11-18 at 12:10 -0500, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> > 
> > This change adds a "missing" sysfs attribute to scsi_device
> > which is set when a previously scanned device no longer appears
> > in the REPORT LUNS inventory.
> 
> A sysfs parameter with time and external input dependent semantics
> really doesn't look like a good idea.  What is it you're trying to
> achieve that you can't achieve from userspace by watching for the
> events?
> 
> James
> 
> 

Ideally, one would like to remove SCSI LUNs that are no longer being
presented by the device.  The information about which LUNs are no
longer there could, I suppose, be obtained from userspace by sending
a REPORT LUNS command, but that is somewhat redundant because the
kernel will issue one anyway when it is instructed to scan the target
for new LUNs.  This patch just makes the information visible.

Yes, the information is only valid at a point in time, but that is
true of REPORT LUNS any time it is used.  If the LUN inventory changes
again, there will be a new event generated that can be processed.

A udev event handler tell the kernel to scan for new LUNs, and then
potentially tell the kernel to delete scsi_devices that are now missing.

-Ewan

Re: [PATCH] scsi: Add "missing" sysfs attribute to scsi_device structure

[James Bottomley]

On Mon, 2013-11-18 at 12:59 -0500, Ewan Milne wrote:
> On Mon, 2013-11-18 at 09:45 -0800, James Bottomley wrote:
> > On Mon, 2013-11-18 at 12:10 -0500, Ewan D. Milne wrote:
> > > From: "Ewan D. Milne" <emilne@redhat.com>
> > > 
> > > This change adds a "missing" sysfs attribute to scsi_device
> > > which is set when a previously scanned device no longer appears
> > > in the REPORT LUNS inventory.
> > 
> > A sysfs parameter with time and external input dependent semantics
> > really doesn't look like a good idea.  What is it you're trying to
> > achieve that you can't achieve from userspace by watching for the
> > events?
> > 
> > James
> > 
> > 
> 
> Ideally, one would like to remove SCSI LUNs that are no longer being
> presented by the device.  The information about which LUNs are no
> longer there could, I suppose, be obtained from userspace by sending
> a REPORT LUNS command, but that is somewhat redundant because the
> kernel will issue one anyway when it is instructed to scan the target
> for new LUNs.  This patch just makes the information visible.

But that's the problem, it doesn't.  missing gets reset on every REPORT
LUN, that makes the information potentially inaccurate, particularly if
the device triggers multiple change events.

> Yes, the information is only valid at a point in time, but that is
> true of REPORT LUNS any time it is used.  If the LUN inventory changes
> again, there will be a new event generated that can be processed.
> 
> A udev event handler tell the kernel to scan for new LUNs, and then
> potentially tell the kernel to delete scsi_devices that are now missing.

I don't see what's wrong with sending REPORT LUNS from user space and
pruning devices that no longer exist.  You should probably do that
before you trigger a REPORT LUN scan; that way anything that reappeared
gets put back, so there's no chance of losing a LUN.

James

Re: [PATCH] scsi: Add "missing" sysfs attribute to scsi_device structure

[Ewan Milne]

On Mon, 2013-11-18 at 10:15 -0800, James Bottomley wrote:
> On Mon, 2013-11-18 at 12:59 -0500, Ewan Milne wrote:
> > On Mon, 2013-11-18 at 09:45 -0800, James Bottomley wrote:
> > > On Mon, 2013-11-18 at 12:10 -0500, Ewan D. Milne wrote:
> > > > From: "Ewan D. Milne" <emilne@redhat.com>
> > > > 
> > > > This change adds a "missing" sysfs attribute to scsi_device
> > > > which is set when a previously scanned device no longer appears
> > > > in the REPORT LUNS inventory.
> > > 
> > > A sysfs parameter with time and external input dependent semantics
> > > really doesn't look like a good idea.  What is it you're trying to
> > > achieve that you can't achieve from userspace by watching for the
> > > events?
> > > 
> > > James
> > > 
> > > 
> > 
> > Ideally, one would like to remove SCSI LUNs that are no longer being
> > presented by the device.  The information about which LUNs are no
> > longer there could, I suppose, be obtained from userspace by sending
> > a REPORT LUNS command, but that is somewhat redundant because the
> > kernel will issue one anyway when it is instructed to scan the target
> > for new LUNs.  This patch just makes the information visible.
> 
> But that's the problem, it doesn't.  missing gets reset on every REPORT
> LUN, that makes the information potentially inaccurate, particularly if
> the device triggers multiple change events.
> 
> > Yes, the information is only valid at a point in time, but that is
> > true of REPORT LUNS any time it is used.  If the LUN inventory changes
> > again, there will be a new event generated that can be processed.
> > 
> > A udev event handler tell the kernel to scan for new LUNs, and then
> > potentially tell the kernel to delete scsi_devices that are now missing.
> 
> I don't see what's wrong with sending REPORT LUNS from user space and
> pruning devices that no longer exist.  You should probably do that
> before you trigger a REPORT LUN scan; that way anything that reappeared
> gets put back, so there's no chance of losing a LUN.

Well, yes, except that deleting it first and then (potentially)
re-adding it would leave a period of time when the LUN isn't there.
But, that's exactly what the target reported, so I guess that's OK.

I appreciate the feedback, I'll look at issuing REPORT LUNS from
userspace.

-Ewan

> 
> James
> 
>