diff mbox

scsi: Make scsi_vpd_lun_id() able to use T10 vendor ID based designators

Message ID 20160504065830.GB19436@oak.ozlabs.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Paul Mackerras May 4, 2016, 6:58 a.m. UTC 
This adds code to scsi_vpd_lun_id() to enable it to use T10 vendor ID
based designators.  This is needed to allow alua to work on disks that
don't have any designators of type 2, 3 or 8.  Commit 0047220c6c36
("scsi_dh_alua: use unique device id", 2016-02-19) added a requirement
that alua can only be used on disks for which scsi_vpd_lun_id() can
produce an identifying string.

Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
 drivers/scsi/scsi_lib.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

--
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

Comments

Hannes Reinecke May 4, 2016, 10:04 a.m. UTC | #1 
On 05/04/2016 08:58 AM, Paul Mackerras wrote:
> This adds code to scsi_vpd_lun_id() to enable it to use T10 vendor ID
> based designators.  This is needed to allow alua to work on disks that
> don't have any designators of type 2, 3 or 8.  Commit 0047220c6c36
> ("scsi_dh_alua: use unique device id", 2016-02-19) added a requirement
> that alua can only be used on disks for which scsi_vpd_lun_id() can
> produce an identifying string.
> 
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
>  drivers/scsi/scsi_lib.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..f4f69cc 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3214,6 +3214,20 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
>  			goto next_desig;
>  
>  		switch (d[1] & 0xf) {
> +		case 0x1:
> +			/* T10 vendor ID */
> +			if (cur_id_size > d[3])
> +				break;
> +			/* Prefer EUI-64 or NAA IEEE Registered Extended */
> +			if ((cur_id_type == 0x2 || cur_id_type == 0x3) &&
> +			    cur_id_size == d[3])
> +				break;
> +			cur_id_size = d[3];
> +			cur_id_str = d + 4;
> +			cur_id_type = d[1] & 0xf;
> +			id_size = snprintf(id, id_len, "%*phN", cur_id_size,
> +					   cur_id_str);
> +			break;
>  		case 0x2:
>  			/* EUI-64 */
>  			if (cur_id_size > d[3])
> 
Nearly.
The thing is, a T-10 vendor specific ID is _supposed_ to be an ASCII
string. So I'd rather have it decoded as such.
And we're missing decoding for 'vendor-specific' ID, too.

I'll be sending a replacement patch.

Cheers,

Hannes
Paul Mackerras May 5, 2016, 3:50 a.m. UTC | #2 
On Wed, May 04, 2016 at 12:04:16PM +0200, Hannes Reinecke wrote:
> On 05/04/2016 08:58 AM, Paul Mackerras wrote:
> > This adds code to scsi_vpd_lun_id() to enable it to use T10 vendor ID
> > based designators.  This is needed to allow alua to work on disks that
> > don't have any designators of type 2, 3 or 8.  Commit 0047220c6c36
> > ("scsi_dh_alua: use unique device id", 2016-02-19) added a requirement
> > that alua can only be used on disks for which scsi_vpd_lun_id() can
> > produce an identifying string.
> > 
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> > ---
> >  drivers/scsi/scsi_lib.c |   14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 8106515..f4f69cc 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -3214,6 +3214,20 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
> >  			goto next_desig;
> >  
> >  		switch (d[1] & 0xf) {
> > +		case 0x1:
> > +			/* T10 vendor ID */
> > +			if (cur_id_size > d[3])
> > +				break;
> > +			/* Prefer EUI-64 or NAA IEEE Registered Extended */
> > +			if ((cur_id_type == 0x2 || cur_id_type == 0x3) &&
> > +			    cur_id_size == d[3])
> > +				break;
> > +			cur_id_size = d[3];
> > +			cur_id_str = d + 4;
> > +			cur_id_type = d[1] & 0xf;
> > +			id_size = snprintf(id, id_len, "%*phN", cur_id_size,
> > +					   cur_id_str);
> > +			break;
> >  		case 0x2:
> >  			/* EUI-64 */
> >  			if (cur_id_size > d[3])
> > 
> Nearly.
> The thing is, a T-10 vendor specific ID is _supposed_ to be an ASCII
> string. So I'd rather have it decoded as such.

Do we need to defend against non-printing characters in the string?

> And we're missing decoding for 'vendor-specific' ID, too.

There's no guarantee that this would be ASCII, right?  So would you
print it in hex?

Also, is there a preference between these types?  For example, is an
8-byte EUI-64 preferable to a vendor-specific ID of any length?

Cheers,
Paul.
--
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
Hannes Reinecke May 5, 2016, 8:01 a.m. UTC | #3 
On 05/05/2016 05:50 AM, Paul Mackerras wrote:
> On Wed, May 04, 2016 at 12:04:16PM +0200, Hannes Reinecke wrote:
>> On 05/04/2016 08:58 AM, Paul Mackerras wrote:
>>> This adds code to scsi_vpd_lun_id() to enable it to use T10 vendor ID
>>> based designators.  This is needed to allow alua to work on disks that
>>> don't have any designators of type 2, 3 or 8.  Commit 0047220c6c36
>>> ("scsi_dh_alua: use unique device id", 2016-02-19) added a requirement
>>> that alua can only be used on disks for which scsi_vpd_lun_id() can
>>> produce an identifying string.
>>>
>>> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
>>> ---
>>>  drivers/scsi/scsi_lib.c |   14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index 8106515..f4f69cc 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -3214,6 +3214,20 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
>>>  			goto next_desig;
>>>  
>>>  		switch (d[1] & 0xf) {
>>> +		case 0x1:
>>> +			/* T10 vendor ID */
>>> +			if (cur_id_size > d[3])
>>> +				break;
>>> +			/* Prefer EUI-64 or NAA IEEE Registered Extended */
>>> +			if ((cur_id_type == 0x2 || cur_id_type == 0x3) &&
>>> +			    cur_id_size == d[3])
>>> +				break;
>>> +			cur_id_size = d[3];
>>> +			cur_id_str = d + 4;
>>> +			cur_id_type = d[1] & 0xf;
>>> +			id_size = snprintf(id, id_len, "%*phN", cur_id_size,
>>> +					   cur_id_str);
>>> +			break;
>>>  		case 0x2:
>>>  			/* EUI-64 */
>>>  			if (cur_id_size > d[3])
>>>
>> Nearly.
>> The thing is, a T-10 vendor specific ID is _supposed_ to be an ASCII
>> string. So I'd rather have it decoded as such.
> 
> Do we need to defend against non-printing characters in the string?
> 
I really would like to stick to ASCII output here, as most vendors put a
meaningful string in here.
Those who don't we should be going with the '.' normal method and print
a dot '.' instead.
(Can't we fix up snprintf do do it for us? Would be soo cool ...)

>> And we're missing decoding for 'vendor-specific' ID, too.
> 
> There's no guarantee that this would be ASCII, right?  So would you
> print it in hex?
> 
Yes, that's the plan here.

> Also, is there a preference between these types?  For example, is an
> 8-byte EUI-64 preferable to a vendor-specific ID of any length?
> 
Yes. I'd treat T10 vendor ID descriptors with the lowest preference
(irrespective of the length), eclipsed (sic) only by the truly vendor
specific ones.
And actually I would make the vendor-specific decoding the default
entry, too, as we might come across some other descriptors which we
cannot decode (yet). And by having a default method for decoding we
ensure to always be able to come up with an ALUA identification.
Otherwise we might end up in the same situation as we're in now.

Cheers,

Hannes
Paul Mackerras May 5, 2016, 12:41 p.m. UTC | #4 
On Thu, May 05, 2016 at 10:01:13AM +0200, Hannes Reinecke wrote:
> On 05/05/2016 05:50 AM, Paul Mackerras wrote:
> > On Wed, May 04, 2016 at 12:04:16PM +0200, Hannes Reinecke wrote:
...
> >> Nearly.
> >> The thing is, a T-10 vendor specific ID is _supposed_ to be an ASCII
> >> string. So I'd rather have it decoded as such.
> > 
> > Do we need to defend against non-printing characters in the string?
> > 
> I really would like to stick to ASCII output here, as most vendors put a
> meaningful string in here.
> Those who don't we should be going with the '.' normal method and print
> a dot '.' instead.
> (Can't we fix up snprintf do do it for us? Would be soo cool ...)
> 
> >> And we're missing decoding for 'vendor-specific' ID, too.
> > 
> > There's no guarantee that this would be ASCII, right?  So would you
> > print it in hex?
> > 
> Yes, that's the plan here.
> 
> > Also, is there a preference between these types?  For example, is an
> > 8-byte EUI-64 preferable to a vendor-specific ID of any length?
> > 
> Yes. I'd treat T10 vendor ID descriptors with the lowest preference
> (irrespective of the length), eclipsed (sic) only by the truly vendor
> specific ones.

Well, the disks on my system all have the same vendor-specific ID,
unfortunately -- a '0' followed by 19 spaces.  Here are the
designators for all the disks on my system:

scsi 0:2:0:0: 02 01 00 20 'IBM     IPR-0   5EC4AB0000000020'
scsi 0:2:0:0: 02 00 00 14 '0                   '
scsi 0:2:1:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000080'
scsi 0:2:1:0: 02 00 00 14 '0                   '
scsi 0:2:2:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000060'
scsi 0:2:2:0: 02 00 00 14 '0                   '
scsi 0:2:3:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000040'
scsi 0:2:3:0: 02 00 00 14 '0                   '
scsi 0:2:4:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000020'
scsi 0:2:4:0: 02 00 00 14 '0                   '
scsi 0:2:5:0: 02 01 00 20 'IBM     IPR-0   5EC28C00000000C0'
scsi 0:2:5:0: 02 00 00 14 '0                   '
scsi 0:2:6:0: 02 01 00 20 'IBM     IPR-0   5EC28C00000000A0'
scsi 0:2:6:0: 02 00 00 14 '0                   '

> And actually I would make the vendor-specific decoding the default
> entry, too, as we might come across some other descriptors which we
> cannot decode (yet). And by having a default method for decoding we
> ensure to always be able to come up with an ALUA identification.
> Otherwise we might end up in the same situation as we're in now.

What would happen if we pick a designator which is not unique across
different disks?  Would we think that they were all the same disk?

Paul.
--
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
Hannes Reinecke May 6, 2016, 9:46 a.m. UTC | #5 
On 05/05/2016 02:41 PM, Paul Mackerras wrote:
> On Thu, May 05, 2016 at 10:01:13AM +0200, Hannes Reinecke wrote:
>> On 05/05/2016 05:50 AM, Paul Mackerras wrote:
>>> On Wed, May 04, 2016 at 12:04:16PM +0200, Hannes Reinecke wrote:
> ...
>>>> Nearly.
>>>> The thing is, a T-10 vendor specific ID is _supposed_ to be an ASCII
>>>> string. So I'd rather have it decoded as such.
>>>
>>> Do we need to defend against non-printing characters in the string?
>>>
>> I really would like to stick to ASCII output here, as most vendors put a
>> meaningful string in here.
>> Those who don't we should be going with the '.' normal method and print
>> a dot '.' instead.
>> (Can't we fix up snprintf do do it for us? Would be soo cool ...)
>>
>>>> And we're missing decoding for 'vendor-specific' ID, too.
>>>
>>> There's no guarantee that this would be ASCII, right?  So would you
>>> print it in hex?
>>>
>> Yes, that's the plan here.
>>
>>> Also, is there a preference between these types?  For example, is an
>>> 8-byte EUI-64 preferable to a vendor-specific ID of any length?
>>>
>> Yes. I'd treat T10 vendor ID descriptors with the lowest preference
>> (irrespective of the length), eclipsed (sic) only by the truly vendor
>> specific ones.
> 
> Well, the disks on my system all have the same vendor-specific ID,
> unfortunately -- a '0' followed by 19 spaces.  Here are the
> designators for all the disks on my system:
> 
> scsi 0:2:0:0: 02 01 00 20 'IBM     IPR-0   5EC4AB0000000020'
> scsi 0:2:0:0: 02 00 00 14 '0                   '
> scsi 0:2:1:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000080'
> scsi 0:2:1:0: 02 00 00 14 '0                   '
> scsi 0:2:2:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000060'
> scsi 0:2:2:0: 02 00 00 14 '0                   '
> scsi 0:2:3:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000040'
> scsi 0:2:3:0: 02 00 00 14 '0                   '
> scsi 0:2:4:0: 02 01 00 20 'IBM     IPR-0   5EC28C0000000020'
> scsi 0:2:4:0: 02 00 00 14 '0                   '
> scsi 0:2:5:0: 02 01 00 20 'IBM     IPR-0   5EC28C00000000C0'
> scsi 0:2:5:0: 02 00 00 14 '0                   '
> scsi 0:2:6:0: 02 01 00 20 'IBM     IPR-0   5EC28C00000000A0'
> scsi 0:2:6:0: 02 00 00 14 '0                   '
> 
My all time favourite here is HP SmartArray, which has this id:

01 03 00 10 600508b1001cc041769dcf27a752f8c2
01 00 00 04 00000000

Hence the logic to select the 'best' ID; any of the NAA or EUI
variants are (more-or-less) guaranteed to give you a valid result.
T-10 and Vendor-specific variants, OTOH, do not have a guarantee to
be even parseable; I've seen a very wide interpretation area of them
meaning of 'ASCII'...

>> And actually I would make the vendor-specific decoding the default
>> entry, too, as we might come across some other descriptors which we
>> cannot decode (yet). And by having a default method for decoding we
>> ensure to always be able to come up with an ALUA identification.
>> Otherwise we might end up in the same situation as we're in now.
> 
> What would happen if we pick a designator which is not unique across
> different disks?  Would we think that they were all the same disk?
> 
This is why I've implemented the selection mechanism.
Any of the NAA or EUI variants are guaranteed to the unique; I've
yet to see a vendor to get this wrong.
T-10 and vendor-specific variants, however, have a habit to be _not_
unique, without us being able to tell if they are.
(Which was the reason why I've omitted decoding for these).

Meanwhile I've posted two patches, one for not failing ALUA when no
VPD identifaction could be retrieved, and another one for properly
decoding T-10 vendor identifications.

Can you please test them and see if you issue is resolved?

Thanks.

Cheers,

Hannes
diff mbox

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..f4f69cc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3214,6 +3214,20 @@  int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 			goto next_desig;
 
 		switch (d[1] & 0xf) {
+		case 0x1:
+			/* T10 vendor ID */
+			if (cur_id_size > d[3])
+				break;
+			/* Prefer EUI-64 or NAA IEEE Registered Extended */
+			if ((cur_id_type == 0x2 || cur_id_type == 0x3) &&
+			    cur_id_size == d[3])
+				break;
+			cur_id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			id_size = snprintf(id, id_len, "%*phN", cur_id_size,
+					   cur_id_str);
+			break;
 		case 0x2:
 			/* EUI-64 */
 			if (cur_id_size > d[3])