Re: [PATCHv7 20/23] scsi: Add 'access_state' attribute

[Hannes Reinecke]

On 02/18/2016 11:06 PM, Bart Van Assche wrote:
> On 02/15/2016 12:24 AM, Hannes Reinecke wrote:
>> --- a/include/scsi/scsi_proto.h
>> +++ b/include/scsi/scsi_proto.h
>> @@ -277,5 +277,17 @@ struct scsi_lun {
>>   __u8 scsi_lun[8];
>>   };
>>
>> +/* SPC asymmetric access states */
>> +#define SCSI_ACCESS_STATE_OPTIMAL 0x00
>> +#define SCSI_ACCESS_STATE_ACTIVE  0x01
>> +#define SCSI_ACCESS_STATE_STANDBY 0x02
>> +#define SCSI_ACCESS_STATE_UNAVAILABLE 0x03
>> +#define SCSI_ACCESS_STATE_LBA 0x04
>> +#define SCSI_ACCESS_STATE_OFFLINE 0x0e
>> +#define SCSI_ACCESS_STATE_TRANSITIONING 0x0f
>> +
>> +#define SCSI_ACCESS_STATE_MASK0x0f
>> +#define SCSI_ACCESS_STATE_PREFERRED   0x80
>> +#define SCSI_ACCESS_STATE_UNKNOWN 0x70
> 
> Please mention in the comment above these constants that the ALUA
> state values apply to both the RTPG and STPG commands but that the
> PREFERRED bit only applies to the RTPG command.
> 
> All constants in scsi_proto.h come from a SCSI standard. However,
> this patch adds a constant that does not come from a SCSI standard
> (SCSI_ACCESS_STATE_UNKNOWN). Please remove this constant entirely
> because it causes confusion. sdev_show_access_state() only
> interprets the PREFERRED bit and the lower 4 bits of the
> access_state member variable. This means that the
> SCSI_ACCESS_STATE_UNKNOWN entry in sdev_access_states[] is never
> used and hence should be left out. This also means that the
> "sdev->access_state = SCSI_ACCESS_STATE_UNKNOWN" assignment in
> scsi_scan.c is equivalent to "sdev->access_state =
> SCSI_ACCESS_STATE_OPTIMAL".
> 
Correct. Will be fixing it up.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 20/23] scsi: Add 'access_state' attribute

[Hannes Reinecke]

On 02/18/2016 10:55 PM, Bart Van Assche wrote:
> On 02/15/2016 12:24 AM, Hannes Reinecke wrote:
>> Add an 'access_state' attribute to struct scsi_device to
>> display the asymmetric LUN access state.
> 
> Do we really need to add this attribute to SCSI devices that do not
> support ALUA, e.g. ATA devices ? From a system on which this patch
> series has been installed:
> 
> [root ~]# lsscsi | grep 0:0:0:0
> [0:0:0:0] disk ATA ST1000NM0033-9ZM GA67 /dev/sda
> [root ~]# cat /sys/class/scsi_device/0:0:0:0/device/access_state
> active/optimized
> 
Well.

I've hit the same issue with my patches exposing vpd pages to sysfs.

Thing is, the sysfs entries are created during scsi_alloc_sdev().
But we only know if the have these entries during scsi_add_lun(), ie
way beyond that point. So ATM we don't have a chance of selectively
enabling them.

Any change here would require a modification to the core sysfs
behaviour, which might not that easy.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 20/23] scsi: Add 'access_state' attribute

[Bart Van Assche]

On 02/15/2016 12:24 AM, Hannes Reinecke wrote:

--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -277,5 +277,17 @@ struct scsi_lun {
__u8 scsi_lun[8];
  };

+/* SPC asymmetric access states */
+#define SCSI_ACCESS_STATE_OPTIMAL 0x00
+#define SCSI_ACCESS_STATE_ACTIVE  0x01
+#define SCSI_ACCESS_STATE_STANDBY 0x02
+#define SCSI_ACCESS_STATE_UNAVAILABLE 0x03
+#define SCSI_ACCESS_STATE_LBA 0x04
+#define SCSI_ACCESS_STATE_OFFLINE 0x0e
+#define SCSI_ACCESS_STATE_TRANSITIONING 0x0f
+
+#define SCSI_ACCESS_STATE_MASK0x0f
+#define SCSI_ACCESS_STATE_PREFERRED   0x80
+#define SCSI_ACCESS_STATE_UNKNOWN 0x70


Please mention in the comment above these constants that the ALUA state 
values apply to both the RTPG and STPG commands but that the PREFERRED 
bit only applies to the RTPG command.


All constants in scsi_proto.h come from a SCSI standard. However, this 
patch adds a constant that does not come from a SCSI standard 
(SCSI_ACCESS_STATE_UNKNOWN). Please remove this constant entirely 
because it causes confusion. sdev_show_access_state() only interprets 
the PREFERRED bit and the lower 4 bits of the access_state member 
variable. This means that the SCSI_ACCESS_STATE_UNKNOWN entry in 
sdev_access_states[] is never used and hence should be left out. This 
also means that the "sdev->access_state = SCSI_ACCESS_STATE_UNKNOWN" 
assignment in scsi_scan.c is equivalent to "sdev->access_state = 
SCSI_ACCESS_STATE_OPTIMAL".


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 20/23] scsi: Add 'access_state' attribute

[Bart Van Assche]

On 02/15/2016 12:24 AM, Hannes Reinecke wrote:

Add an 'access_state' attribute to struct scsi_device to
display the asymmetric LUN access state.


Do we really need to add this attribute to SCSI devices that do not 
support ALUA, e.g. ATA devices ? From a system on which this patch 
series has been installed:


[root ~]# lsscsi | grep 0:0:0:0
[0:0:0:0] disk ATA ST1000NM0033-9ZM GA67 /dev/sda
[root ~]# cat /sys/class/scsi_device/0:0:0:0/device/access_state
active/optimized

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv7 20/23] scsi: Add 'access_state' attribute

[Hannes Reinecke]

Add an 'access_state' attribute to struct scsi_device to
display the asymmetric LUN access state.

Reviewed-by: Ewan Milne 
Reviewed-by: Christoph Hellwig 
Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/scsi_scan.c   |  1 +
 drivers/scsi/scsi_sysfs.c  | 49 ++
 include/scsi/scsi_device.h |  1 +
 include/scsi/scsi_proto.h  | 12 
 4 files changed, 63 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 97074c9..5bf3945 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct 
scsi_target *starget,
sdev->lun = lun;
sdev->channel = starget->channel;
sdev->sdev_state = SDEV_CREATED;
+   sdev->access_state = SCSI_ACCESS_STATE_UNKNOWN;
INIT_LIST_HEAD(>siblings);
INIT_LIST_HEAD(>same_target_siblings);
INIT_LIST_HEAD(>cmd_list);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 4f18a85..e1541d5 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -81,6 +81,34 @@ const char *scsi_host_state_name(enum scsi_host_state state)
return name;
 }
 
+static const struct {
+   unsigned char   value;
+   char*name;
+} sdev_access_states[] = {
+   { SCSI_ACCESS_STATE_OPTIMAL, "active/optimized" },
+   { SCSI_ACCESS_STATE_ACTIVE, "active/non-optimized" },
+   { SCSI_ACCESS_STATE_STANDBY, "standby" },
+   { SCSI_ACCESS_STATE_UNAVAILABLE, "unavailable" },
+   { SCSI_ACCESS_STATE_LBA, "lba-dependent" },
+   { SCSI_ACCESS_STATE_OFFLINE, "offline" },
+   { SCSI_ACCESS_STATE_TRANSITIONING, "transitioning" },
+   { SCSI_ACCESS_STATE_UNKNOWN, "unknown" },
+};
+
+const char *scsi_access_state_name(unsigned char state)
+{
+   int i;
+   char *name = NULL;
+
+   for (i = 0; i < ARRAY_SIZE(sdev_access_states); i++) {
+   if (sdev_access_states[i].value == state) {
+   name = sdev_access_states[i].name;
+   break;
+   }
+   }
+   return name;
+}
+
 static int check_set(unsigned long long *val, char *src)
 {
char *last;
@@ -973,6 +1001,26 @@ sdev_store_dh_state(struct device *dev, struct 
device_attribute *attr,
 
 static DEVICE_ATTR(dh_state, S_IRUGO | S_IWUSR, sdev_show_dh_state,
   sdev_store_dh_state);
+
+static ssize_t
+sdev_show_access_state(struct device *dev,
+  struct device_attribute *attr,
+  char *buf)
+{
+   struct scsi_device *sdev = to_scsi_device(dev);
+   unsigned char access_state;
+   bool pref = false;
+
+   if (sdev->access_state & SCSI_ACCESS_STATE_PREFERRED)
+   pref = true;
+
+   access_state = (sdev->access_state & SCSI_ACCESS_STATE_MASK);
+
+   return snprintf(buf, 32, "%s%s\n",
+   scsi_access_state_name(access_state),
+   pref ? " preferred" : "");
+}
+static DEVICE_ATTR(access_state, S_IRUGO, sdev_show_access_state, NULL);
 #endif
 
 static ssize_t
@@ -1047,6 +1095,7 @@ static struct attribute *scsi_sdev_attrs[] = {
_attr_wwid.attr,
 #ifdef CONFIG_SCSI_DH
_attr_dh_state.attr,
+   _attr_access_state.attr,
 #endif
_attr_queue_ramp_up_period.attr,
REF_EVT(media_change),
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 4af2b24..c067019 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -201,6 +201,7 @@ struct scsi_device {
struct scsi_device_handler *handler;
void*handler_data;
 
+   unsigned char   access_state;
enum scsi_device_state sdev_state;
unsigned long   sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long;
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index a9fbf1b..683bf29 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -277,5 +277,17 @@ struct scsi_lun {
__u8 scsi_lun[8];
 };
 
+/* SPC asymmetric access states */
+#define SCSI_ACCESS_STATE_OPTIMAL 0x00
+#define SCSI_ACCESS_STATE_ACTIVE  0x01
+#define SCSI_ACCESS_STATE_STANDBY 0x02
+#define SCSI_ACCESS_STATE_UNAVAILABLE 0x03
+#define SCSI_ACCESS_STATE_LBA 0x04
+#define SCSI_ACCESS_STATE_OFFLINE 0x0e
+#define SCSI_ACCESS_STATE_TRANSITIONING 0x0f
+
+#define SCSI_ACCESS_STATE_MASK0x0f
+#define SCSI_ACCESS_STATE_PREFERRED   0x80
+#define SCSI_ACCESS_STATE_UNKNOWN 0x70
 
 #endif /* _SCSI_PROTO_H_ */
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html