Re: [PATCH 2/4] scsi: use 64-bit LUNs

[Hannes Reinecke <hare@suse.de>]

On 02/25/2013 04:33 PM, Steffen Maier wrote:
> Hi Hannes,
>
> I like the idea and most of the patch set, so I only have a few questions left
 > and some review comments below.
>
> Just curious: Do you also plan to adapt systemd/udev, especially path_id for
 > fc transport with its open coded copy of int_to_scsilun()?
>
Yes.

> Since I don't see zfcp touched in this patch set, assuming this set will get
 > merged, I plan to adapt a few spots in zfcp that might not work 
with 64 bit
 > luns out of the box although most of it already works with 64 bit 
luns inside.
>
Ah. Yes, this might be a good idea. I've already got a patch from 
QLogic regarding qla4xxx, so maybe we should be adding them as 
separate patches on top of the original patchset.

> Speaking of opaque handling of scsi luns: Lately I noticed that some sg3_utils
 > tools decode the lun format and only report parts of the entire 
64 bit fcp lun,
 > e.g. sg_scan or "sg_luns -d". I don't have enough historical scsi 
experience to
 > know why that is, maybe because of some SPI background. By itself 
this is not a
 > problem, however, rescan-scsi-bus.sh makes use of those scsi lun 
parts and then
 > fails when matching those with the full scsi lun exposed by the 
midlayer to user
 > space. E.g. with flat space addresses of IBM DS8000 this does not 
seem to work:
>
> # sg_luns -v -s2 -d /dev/sg2 | head
>      report luns cdb: a0 00 02 00 00 00 00 00 20 00 00 00
>      report luns: requested 8192 bytes but got 4112 bytes
> Lun list length = 4104 which imples 513 lun entries
> Report luns [select_report=2]:
>      c101000000000000
>        REPORT LUNS well known logical unit
>      4020400000000000
>        Flat space addressing: lun=32
>      4020400100000000
>        Flat space addressing: lun=32
>      4020400200000000
>        Flat space addressing: lun=32
>                                   ^^<=== 0x20 of the lun's 1st short
>
> Did I overlook something or are rescan-scsi-bus.sh and maybe other tools really
 > broken with some "modern" scsi targets?
>
Should've been fixed by now.
There was a bug in rescan-scsi-bus.sh which indeed tried to decode 
LUNs, but that has been fixed.

> On 02/19/2013 09:18 AM, Hannes Reinecke wrote:
>> The SCSI standard uses a 64-bit value for LUNs, and large arrays
>> employing large LUN numbers become more and more common.
>> So move the linux SCSI stack to use 64-bit LUN numbers.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>
>>    drivers/scsi/scsi_proc.c                |    2 +-
>
>>    drivers/scsi/scsi_sysfs.c               |   14 ++++----
>>    drivers/scsi/scsi_transport_fc.c        |    4 +-
>>    drivers/scsi/scsi_transport_iscsi.c     |    4 +-
>>    drivers/scsi/scsi_transport_sas.c       |    2 +-
>>    drivers/scsi/sg.c                       |    4 +-
>
>>    include/scsi/scsi.h                     |    2 +-
>>    include/scsi/scsi_device.h              |   22 ++++++------
>>    include/scsi/scsi_transport.h           |    2 +-
>>    50 files changed, 239 insertions(+), 247 deletions(-)
>
>> --- a/drivers/scsi/scsi_proc.c
>> +++ b/drivers/scsi/scsi_proc.c
>> @@ -196,7 +196,7 @@ static int proc_print_scsidevice(struct device *dev, void *data)
>>
>>    	sdev = to_scsi_device(dev);
>>    	seq_printf(s,
>> -		"Host: scsi%d Channel: %02d Id: %02d Lun: %02d\n  Vendor: ",
>> +		"Host: scsi%d Channel: %02d Id: %02d Lun: %02llu\n  Vendor: ",
>>    		sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
>>    	for (i = 0; i < 8; i++) {
>>    		if (sdev->vendor[i] >= 0x20)
>
> Is it intentional that you did not adapt scsi_add_single_device(),
 > scsi_remove_single_device(), proc_scsi_write() to cope with 64 
bit luns?
> (in the admittedly old and probably somewhat deprecated procfs interface;
 > but then again, proc_print_scsidevice can output 64 bit luns now)
>
I deliberately did _not_ touch procfs (apart from the necessary 
bits). Precisely because it's being marked as deprecated.

>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 931a7d9..6e98f05 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -80,7 +80,7 @@ const char *scsi_host_state_name(enum scsi_host_state state)
>>    	return name;
>>    }
>>
>> -static int check_set(unsigned int *val, char *src)
>> +static int check_set(unsigned long long *val, char *src)
>>    {
>>    	char *last;
>>
>> @@ -90,7 +90,7 @@ static int check_set(unsigned int *val, char *src)
>>    		/*
>>    		 * Doesn't check for int overflow
>>    		 */
>> -		*val = simple_strtoul(src, &last, 0);
>> +		*val = simple_strtoull(src, &last, 0);
>>    		if (*last != '\0')
>>    			return 1;
>>    	}
>> @@ -99,11 +99,11 @@ static int check_set(unsigned int *val, char *src)
>>
>>    static int scsi_scan(struct Scsi_Host *shost, const char *str)
>>    {
>> -	char s1[15], s2[15], s3[15], junk;
>> -	unsigned int channel, id, lun;
>> +	char s1[15], s2[15], s3[17], junk;
>
> Since we use automatic base detection with the 3rd argument of simple_strtoull()
 > being 0 in check_set() above, I think the user is free to specify 
the lun to scan
 > for in decimal/octal/hex base for s3.
 >
Yes, we could (in principle).
However, it's not quite clear to me what the user is supposed to 
enter here in these cases.
Original output from REPORT LUNS?
Output from scsilun_to_int?
So we would need to come up with a proper naming scheme to identify 
these kind of things.
However, this should be done with another patch as it's a different 
story. scsi_debug has a similar issue, and will need to be updated 
for this, too.

> With 64 bits, couldn't this need a maximum of 20 decimal digits (plus '\0' termination)
 > which is more than 16? I think this is a legitimate use case as 
long as the scsi lun is
 > represented in decimal in sysfs so users might just have it from 
the h:c:t:l device name there.
> While I don't think anyone would specify the lun in octal, it could even need 22 digits.
> [Maximum number of digits is ceil(ln(2**64-1)/ln(base)) if I'm not mistaken.]
>
Yeah, you're right. I need to increase this to the correct size.

>> +	unsigned long long channel, id, lun;
>>    	int res;
>>
>> -	res = sscanf(str, "%10s %10s %10s %c", s1, s2, s3, &junk);
>> +	res = sscanf(str, "%10s %10s %16s %c", s1, s2, s3, &junk);
>
> ditto
>
>>    	if (res != 3)
>>    		return -EINVAL;
>>    	if (check_set(&channel, s1))
>
>> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
>> index e894ca7..091210f 100644
>> --- a/drivers/scsi/scsi_transport_fc.c
>> +++ b/drivers/scsi/scsi_transport_fc.c
>> @@ -2093,7 +2093,7 @@ fc_timed_out(struct scsi_cmnd *scmd)
>>     * on the rport.
>>     */
>>    static void
>> -fc_user_scan_tgt(struct Scsi_Host *shost, uint channel, uint id, uint lun)
>> +fc_user_scan_tgt(struct Scsi_Host *shost, uint channel, uint id, uint64_t lun)
>>    {
>>    	struct fc_rport *rport;
>>    	unsigned long flags;
>> @@ -2125,7 +2125,7 @@ fc_user_scan_tgt(struct Scsi_Host *shost, uint channel, uint id, uint lun)
>>     * object as the parent.
>>     */
>>    static int
>> -fc_user_scan(struct Scsi_Host *shost, uint channel, uint id, uint lun)
>> +fc_user_scan(struct Scsi_Host *shost, uint channel, uint id, uint64_t lun)
>
> Is it OK, that the maximum lun 2**64-1 overlaps with SCAN_WILD_CARD==~0 from scsi.h?
>
It's the same as today, isn't it?

>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index be2c9a6..271d23d 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>
> I guess we cannot adapt sg_ioctl SG_GET_SCSI_ID that easily without breaking
 > the user space interface. But how does a user of this interface 
know that there
 > are 64 bit luns in the kernel but the ioctl cannot handle the new 
kernel functionality
 > (and may be affected by aliasing)?
>
Hmm. I thought that SG_GET_SCSI_ID is largely deprecated by now; 
every user should be converted to use sysfs.
But yeah, you're right. I'll be looking into that.

Thanks for your input.

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