REPORT LUNS

REPORT LUNS

[Josef Möllers]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

Hi,

Wile trying to figure out how to ignore non-existing LUNs on RAID
devices, I have fallen across an error in scsilun_to_int() where it
doesn't behave as the comment suggests and a problematic piece of code
in scsi_report_lun_scan().

Patch is included.

-- 
Josef Möllers (Pinguinpfleger bei FSC)
	If failure had no penalty success would not be a prize
						-- T.  Pratchett

[-- Attachment #2: patch-2.5.70a --]
[-- Type: text/plain, Size: 996 bytes --]

--- linux-2.5.70/drivers/scsi/scsi_scan.c	2003-05-27 03:00:46.000000000 +0200
+++ linux-2.5.70a/drivers/scsi/scsi_scan.c	2003-06-13 15:54:22.000000000 +0200
@@ -884,8 +884,8 @@
 
 	lun = 0;
 	for (i = 0; i < sizeof(lun); i += 2)
-		lun = lun | (((scsilun->scsi_lun[i] << 8) |
-			      scsilun->scsi_lun[i + 1]) << (i * 8));
+		lun = lun | (((scsilun->scsi_lun[i] & 0x3f) << (i * 8 + 1)) |
+			      ((scsilun->scsi_lun[i + 1]) << (i * 8)));
 	return lun;
 }
 
@@ -1027,13 +1027,18 @@
 	 * the header, so start at 1 and go up to and including num_luns.
 	 */
 	for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
+		int j;
+
 		lun = scsilun_to_int(lunp);
 
 		/*
 		 * Check if the unused part of lunp is non-zero, and so
 		 * does not fit in lun.
 		 */
-		if (memcmp(&lunp->scsi_lun[sizeof(lun)], "\0\0\0\0", 4)) {
+		for (j = sizeof(lun); j < sizeof(struct scsi_lun); j++)
+		    if (lunp->scsi_lun[j] != 0)
+			break;
+		if (j < sizeof(struct scsi_lun)) {
 			int i;
 
 			/*

Re: REPORT LUNS

[Patrick Mansfield]

On Fri, Jun 13, 2003 at 04:04:48PM +0200, Josef Möllers wrote:
> Hi,
> 
> Wile trying to figure out how to ignore non-existing LUNs on RAID
> devices, I have fallen across an error in scsilun_to_int() where it
> doesn't behave as the comment suggests and a problematic piece of code
> in scsi_report_lun_scan().
> 
> Patch is included.

If we clear the upper bits (address method, per SAM-3), someone has to
eventually set them again, and this is not handled today in linux scsi
core.

What adapter and driver are you using? AFAIK the Emulex driver figures out
the address method (high two bits) and ors it with each LUN sent down.

I don't recall what happens with the qlogic driver, I thought it had some
special code to handle these, but can't find it.

For SPI it seems OK to ignore the upper bits since they are truncated.
Maybe some devices work OK this way - they set an address method of 10b or
01b, and then don't care that the LUN comes in without the high bits set.

So the change might work for some adapters, but break on others.

We are probably broken for anyone using the 11b extended logical unit
addressing method (with or without your patch).

Can you change your RAID device to use the peripheral device addressing
method - address method of 00b - instead?

In any case we should do what actually works, given that we are
translating an 8 byte LUN to a (host ordered) int, and later converting
that back to an 8 or 1 byte LUN (for qlogic, to a 2 byte LUN that is then
converted to an 8 byte LUN).

> @@ -1027,13 +1027,18 @@
>  	 * the header, so start at 1 and go up to and including num_luns.
>  	 */
>  	for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
> +		int j;
> +
>  		lun = scsilun_to_int(lunp);
>  
>  		/*
>  		 * Check if the unused part of lunp is non-zero, and so
>  		 * does not fit in lun.
>  		 */
> -		if (memcmp(&lunp->scsi_lun[sizeof(lun)], "\0\0\0\0", 4)) {
> +		for (j = sizeof(lun); j < sizeof(struct scsi_lun); j++)
> +		    if (lunp->scsi_lun[j] != 0)
> +			break;
> +		if (j < sizeof(struct scsi_lun)) {
>  			int i;
>  
>  			/*

What does the above patch fix? It looks like they both check if the last
four bytes are 0.

PS: Going offline really soon now for about a week.

-- Patrick Mansfield
-
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: REPORT LUNS

[Josef Möllers]

Patrick Mansfield wrote:
> 
> On Fri, Jun 13, 2003 at 04:04:48PM +0200, Josef Möllers wrote:
> > Hi,
> >
> > Wile trying to figure out how to ignore non-existing LUNs on RAID
> > devices, I have fallen across an error in scsilun_to_int() where it
> > doesn't behave as the comment suggests and a problematic piece of code
> > in scsi_report_lun_scan().
> >
> > Patch is included.
> 
> If we clear the upper bits (address method, per SAM-3), someone has to
> eventually set them again, and this is not handled today in linux scsi
> core.

The comment said that the upper two bits were ignored which they were
not in the original code, so, apart from fixing the shifts, I also added
the mask to keep the code in sync with the comment. I haven't yet
followed the use of the value thus created.

> > @@ -1027,13 +1027,18 @@
> >        * the header, so start at 1 and go up to and including num_luns.
> >        */
> >       for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
> > +             int j;
> > +
> >               lun = scsilun_to_int(lunp);
> >
> >               /*
> >                * Check if the unused part of lunp is non-zero, and so
> >                * does not fit in lun.
> >                */
> > -             if (memcmp(&lunp->scsi_lun[sizeof(lun)], "\0\0\0\0", 4)) {
> > +             for (j = sizeof(lun); j < sizeof(struct scsi_lun); j++)
> > +                 if (lunp->scsi_lun[j] != 0)
> > +                     break;
> > +             if (j < sizeof(struct scsi_lun)) {
> >                       int i;
> >
> >                       /*
> 
> What does the above patch fix? It looks like they both check if the last
> four bytes are 0.

It does so, iff sizeof(lun) is 4 and sizeof(struct scsi_lun) is 8.
Perhaps it was more a question of personal taste to change the code so
that it also works for 64 bit ints.

-- 
Josef Möllers (Pinguinpfleger bei FSC)
	If failure had no penalty success would not be a prize
						-- T.  Pratchett
-
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: REPORT LUNS

[Patrick Mansfield]

On Mon, Jun 16, 2003 at 08:55:18AM +0200, Josef Möllers wrote:
> Patrick Mansfield wrote:
> > 
> > On Fri, Jun 13, 2003 at 04:04:48PM +0200, Josef Möllers wrote:
> > > Hi,
> > >
> > > Wile trying to figure out how to ignore non-existing LUNs on RAID
> > > devices, I have fallen across an error in scsilun_to_int() where it
> > > doesn't behave as the comment suggests and a problematic piece of code
> > > in scsi_report_lun_scan().
> > >
> > > Patch is included.
> > 
> > If we clear the upper bits (address method, per SAM-3), someone has to
> > eventually set them again, and this is not handled today in linux scsi
> > core.
> 
> The comment said that the upper two bits were ignored which they were
> not in the original code, so, apart from fixing the shifts, I also added
> the mask to keep the code in sync with the comment. I haven't yet
> followed the use of the value thus created.

Ignored meaning "does nothing with it", when we should probably do
something - proper conversion, an error or warning output. The current
code might work (with high bits set), if we did not exceed the
host->max_lun. We might need an 8 byte lun everywhere to make it work
right for all cases.

Can you give more details of the problem you are seeing, and software
(adapter driver) and hardware in use?

I did not notice your shift changes - I don't see how they work or what
it fixes.

If scsi_lun was: 0x1 0x2 0x3 0x4 0 0 0 0

Current code:

First loop in scsilun_to_int, with i = 0:

	lun = 0 | ( (0x1 << 8 | 0x2) << 0) = 0x0102

Second loop i = 2:

	lun = 0x0102 | ( (0x3 << 8 | 0x4) << 16) = 0x03040102

Your patch would give:

First loop i = 0:

	lun = 0 | ((0x1 & 0x3f) << 1) | (0x2 << 0) = 0x02

Second loop i = 2:

	lun = 0x02 | ( ((0x3 & 0x3f) << 17) | (0x4 << 16) = 0x00060002

-- Patrick Mansfield
-
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: REPORT LUNS

[Josef Möllers]

Patrick Mansfield wrote:
> 
> On Mon, Jun 16, 2003 at 08:55:18AM +0200, Josef Möllers wrote:
> > Patrick Mansfield wrote:
> > >
> > > On Fri, Jun 13, 2003 at 04:04:48PM +0200, Josef Möllers wrote:
> > > > Hi,
> > > >
> > > > Wile trying to figure out how to ignore non-existing LUNs on RAID
> > > > devices, I have fallen across an error in scsilun_to_int() where it
> > > > doesn't behave as the comment suggests and a problematic piece of code
> > > > in scsi_report_lun_scan().
> > > >
> > > > Patch is included.
> > >
> > > If we clear the upper bits (address method, per SAM-3), someone has to
> > > eventually set them again, and this is not handled today in linux scsi
> > > core.
> >
> > The comment said that the upper two bits were ignored which they were
> > not in the original code, so, apart from fixing the shifts, I also added
> > the mask to keep the code in sync with the comment. I haven't yet
> > followed the use of the value thus created.
> 
> Ignored meaning "does nothing with it", when we should probably do
> something - proper conversion, an error or warning output. The current
> code might work (with high bits set), if we did not exceed the
> host->max_lun. We might need an 8 byte lun everywhere to make it work
> right for all cases.

OK.

> Can you give more details of the problem you are seeing, and software
> (adapter driver) and hardware in use?

The main problem we have is with current 2.4 kernels that some RAID
systems report SCSI-2 but allow for more than 8 LUNs, so rather than put
each box into the device_list[] (BLIST_SPARSELUN), I was investigating
how this problem could be solved in an orderly fashion: either by using
REPORT LUNS or by examining the Peripheral Qualifier in the INQUIRY data
(which would require BLIST_SPARSELUN, nonetheless).
I looked at the 2.5 code and found that REPORT LUNS was implemented but
stumbled across the comment. I couln't really make sense out of SCSI-3
and a manual I have for EMC RAID subsystems on the data returned by the
REPORT LUNS command. I then looked at the code, got confused about the
shifts and broke the code with my patch.

> I did not notice your shift changes - I don't see how they work or what
> it fixes.

Looking at it fresh in the morning: no, it doesn't fix anything, it
breaks code which is OK. I learnt my lesson to be more careful.

> If scsi_lun was: 0x1 0x2 0x3 0x4 0 0 0 0
> 
> Current code:
> 
> First loop in scsilun_to_int, with i = 0:
> 
>         lun = 0 | ( (0x1 << 8 | 0x2) << 0) = 0x0102
> 
> Second loop i = 2:
> 
>         lun = 0x0102 | ( (0x3 << 8 | 0x4) << 16) = 0x03040102
> 
> Your patch would give:
> 
> First loop i = 0:
> 
>         lun = 0 | ((0x1 & 0x3f) << 1) | (0x2 << 0) = 0x02
> 
> Second loop i = 2:
> 
>         lun = 0x02 | ( ((0x3 & 0x3f) << 17) | (0x4 << 16) = 0x00060002

You're absolutely right. If i needed an excuse (other than not having
been careful enough), I'd attribute it to the heat.

I'll test my patches more thoroughly in the future.

Sorry,

Josef
-- 
Josef Möllers (Pinguinpfleger bei FSC)
	If failure had no penalty success would not be a prize
						-- T.  Pratchett
-
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: Report luns

[Douglas Gilbert]

Patrick Mansfield wrote:
>>Douglas Gilbert wrote:
>>
>>>So if that fails scsi_scan.c might issue a REPORT LUNS command to
>>>this lun: 0xc101 which is the value of the REPORT LUNS well-known lu.
>>>Note that this is a 16 bit quantity (not 64 bit as I said in a
>>>previous post about luns). For simplicity let us assume that
>>>that it is at the top level. [If a device returns HiSup=1 in its
>>>standard INQUIRY response then it claims to support a 4 level
>>>hierarchy in its luns (with 16 bits in each level).]
>>
>>And also supports REPORT LUNS.
>>
>>So in either case, we send REPORT LUNS to LUN 0 and if
>>this fails to REPORT LUNS W-LUN (0xC101...).
> 
> 
> Do we also have to change to send the INQUIRY to the well known LUN?

Yes, all well known lus have mandatory support for INQUIRY.

> And then someday add black and white list flags and code for them ...

Hope not. If an INQUIRY to lun 0 fails (and perhaps the transport level
doesn't object) then send an INQUIRY to lun 0xc101 . If that succeeds
then send a REPORT LUNS to lun 0xc101 ...

> Why is stuff like this even added to the standard??? Why not use LUN 0
> (all zeroes) or at worst have one well known LUN like the all f's?

All f's is already defined in SAM-3 as "lu not specified", whatever
that means :-)

> The hierarchical LUN stuff and the various address modes are dumb, just
> treat the LUN as a tag/identifier (similiar to a TCP/IP network port) and
> leave the interpretation of it up to the target device (or at worst the
> transport).

As you have worked in the scsi_scan.c area you are well placed
to comment. I have only recently been looking at luns. I also
wonder about the device peripheral type returned by an INQUIRY
sent to a W-LUN (i.e. well known lun). If it was closely associated
with disks (e.g. a RAID) and returned a "direct access" device then
the sd driver wouldn't make much sense of a W-LUN device. Note this
is not a problem while select=0 is set in scsi_scan's REPORT LUN
commands.


An example, inspired by the BCC draft, involving well
known logical units might help. Say we have
a bridge from SAS (or FC, it doesn't matter) to SPI-4 (U320).
One way to implement the bridge is to map the 15 possible
devices on the U320 bus to luns behind a single target in
a SAS domain (or SAS bus, it's the same thing). Lets assume
there are 5 U320 disks connected to the SPI bus and assume
SPI target ids 0 to 4 map to SAS luns 0 to 4.

So now if one sends a REPORT LUNS to lun 0 that U320 disk is
expected to know about its sibling disks!? That is why I used
the term "hack" with respect to lun 0. What really happens
is the device server in the bridge intercepts the REPORT LUNS
command and produces a response reflecting its knowledge of
those 5 U320 disks present. Why play this game? Why not talk
directly to the device server (addressed via a well known lu).
This way an INQUIRY to a W-LUN yields information about
the bridge, while an INQUIRY to lun 0 yields information about
that disk.
If the TARGET LOG PAGES W-LUN is supported then the
bridge can supply logs from its point of view (a SAS
bridge device) while a LOG SENSE command sent to lun 4
reports that U320 disk's logs.


Luben is probably correct in saying manufacturers will
support both lun 0 and the REPORT LUNS well known lu
in advanced products such as bridges. In Linux currently
we can detect the presence of well known lus (e.g.
sg_luns --select=1 /dev/sg3). If anything is found we
have no mechanism to probe them.

Doug Gilbert

Re: Report luns

[Luben Tuikov]

Douglas Gilbert wrote:
> 
> So now if one sends a REPORT LUNS to lun 0 that U320 disk is
> expected to know about its sibling disks!? That is why I used
> the term "hack" with respect to lun 0. What really happens
> is the device server in the bridge intercepts the REPORT LUNS
> command and produces a response reflecting its knowledge of
> those 5 U320 disks present. Why play this game? Why not talk
> directly to the device server (addressed via a well known lu).
> This way an INQUIRY to a W-LUN yields information about
> the bridge, while an INQUIRY to lun 0 yields information about
> that disk.

Very good point!

So we can reverse the logic into sending REPORT LUNS to
the REPORT LUNS W-LUN *first* and if this fails (most probably
in early years), then issue REPORT LUNS to LUN 0.

	Luben