Re: standards revisions

Re: standards revisions

[Nicholas A. Bellinger]

On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> Currenly all non-pscsi bakcneds report their standards version as
> SPC 2 via ->get_device_rev.

No, the proper on-the-wire bits to signal SPC-3 compliance are already
being returned by virtual backend drivers within standard INQUIRY
payload data.  

Notice the comment:

root@tifa:/usr/src/target-pending.git# grep SCSI_SPC_2 drivers/target/*.c
drivers/target/target_core_file.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
drivers/target/target_core_iblock.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
drivers/target/target_core_rd.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */

>  In addition to putting it into the
> inquirty data in spc_emulate_inquiry_std we also use it in two
> places to check on the version of the persistent reservation and
> alua support.  What is the benefit of supporting the old-style SCSI 2
> reservations and ALUA support when they can't be used at all with
> the virtual backends, and can only be used in the corner case emulated
> ALUA/PR support for pscsi?
> 

It's the include/scsi/scsi.h SCSI_3 + SCSI_SPC_* version definition
names that are incorrect:

#define SCSI_UNKNOWN    0
#define SCSI_1          1
#define SCSI_1_CCS      2
#define SCSI_2          3
#define SCSI_3          4        /* SPC */
#define SCSI_SPC_2      5
#define SCSI_SPC_3      6

from spc4r30 section 6.4.2 Standard INQUIRY data, Table 142 -- Version:

   00h     The device server does not claim conformance to any standard.
01h to 02h Obsolete
   03h     The device server complies to ANSI INCITS 301-1997 (a withdrawn standard).
   04h     The device server complies to ANSI INCITS 351-2001 (SPC-2).
   05h     The device server complies to ANSI INCITS 408-2005 (SPC-3).
   06h     The device server complies to this standard.

How about the following patch to fix the long-standing incorrect name
usage of these three scsi.h defines..?

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index d947ffc..60ae194 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -834,7 +834,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
        sdev->lockable = sdev->removable;
        sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
 
-       if (sdev->scsi_level >= SCSI_3 ||
+       if (sdev->scsi_level >= SCSI_SPC_2 ||
                        (sdev->inquiry_len > 56 && inq_result[56] & 0x04))
                sdev->ppr = 1;
        if (inq_result[7] & 0x60)
@@ -1200,7 +1200,7 @@ static void scsi_sequential_lun_scan(struct scsi_target *starget,
         * Do not scan SCSI-2 or lower device past LUN 7, unless
         * BLIST_LARGELUN.
         */
-       if (scsi_level < SCSI_3 && !(bflags & BLIST_LARGELUN))
+       if (scsi_level < SCSI_SPC_2 && !(bflags & BLIST_LARGELUN))
                max_dev_lun = min(8U, max_dev_lun);
 
        /*
@@ -1327,7 +1327,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
        if (starget->scsi_level < SCSI_2 &&
            starget->scsi_level != SCSI_UNKNOWN)
                return 1;
-       if (starget->scsi_level < SCSI_3 &&
+       if (starget->scsi_level < SCSI_SPC_2 &&
            (!(bflags & BLIST_REPORTLUN2) || shost->max_lun <= 8))
                return 1;
        if (bflags & BLIST_NOLUN)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4df73e5..f7dd7ec 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1901,7 +1901,7 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
                return 0;
        if (sdp->try_rc_10_first)
                return 0;
-       if (sdp->scsi_level > SCSI_SPC_2)
+       if (sdp->scsi_level > SCSI_SPC_3)
                return 1;
        if (scsi_device_protection(sdp))
                return 1;
@@ -2443,7 +2443,7 @@ static int sd_try_extended_inquiry(struct scsi_device *sdp)
         * some USB ones crash on receiving them, and the pages
         * we currently ask for are for SPC-3 and beyond
         */
-       if (sdp->scsi_level > SCSI_SPC_2 && !sdp->skip_vpd_pages)
+       if (sdp->scsi_level > SCSI_SPC_3 && !sdp->skip_vpd_pages)
                return 1;
        return 0;
 }
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 0360383..65941e5 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -536,7 +536,7 @@ static ssize_t fd_show_configfs_dev_params(
  */
 static u32 fd_get_device_rev(struct se_device *dev)
 {
-       return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
+       return SCSI_SPC_3;
 }
 
 /*     fd_get_device_type(): (Part of se_subsystem_api_t template)
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 29408d4..7ed94b0 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -713,7 +713,7 @@ fail:
 
 static u32 iblock_get_device_rev(struct se_device *dev)
 {
-       return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
+       return SCSI_SPC_3;
 }
 
 static u32 iblock_get_device_type(struct se_device *dev)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index d00bbe3..df5a811 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -445,7 +445,7 @@ static ssize_t rd_show_configfs_dev_params(
 
 static u32 rd_get_device_rev(struct se_device *dev)
 {
-       return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
+       return SCSI_SPC_3;
 }
 
 static u32 rd_get_device_type(struct se_device *dev)
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 66216c1..cad47eb 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -536,9 +536,9 @@ static inline int scsi_is_wlun(unsigned int lun)
 #define SCSI_1          1
 #define SCSI_1_CCS      2
 #define SCSI_2          3
-#define SCSI_3          4        /* SPC */
-#define SCSI_SPC_2      5
-#define SCSI_SPC_3      6
+#define SCSI_SPC_2      4      /* ANSI INCITS 351-2001 (SPC-2) */
+#define SCSI_SPC_3      5      /* ANSI INCITS 408-2005 (SPC-3) */
+#define SCSI_SPC_4      6
 
 /*
  * INQ PERIPHERAL QUALIFIERS

Re: standards revisions

[James Bottomley]

On Sat, 2012-10-06 at 23:11 -0700, Nicholas A. Bellinger wrote:
> On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> > Currenly all non-pscsi bakcneds report their standards version as
> > SPC 2 via ->get_device_rev.
> 
> No, the proper on-the-wire bits to signal SPC-3 compliance are already
> being returned by virtual backend drivers within standard INQUIRY
> payload data.  
> 
> Notice the comment:
> 
> root@tifa:/usr/src/target-pending.git# grep SCSI_SPC_2 drivers/target/*.c
> drivers/target/target_core_file.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> drivers/target/target_core_iblock.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> drivers/target/target_core_rd.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */

That's causing confusion, I think!

> >  In addition to putting it into the
> > inquirty data in spc_emulate_inquiry_std we also use it in two
> > places to check on the version of the persistent reservation and
> > alua support.  What is the benefit of supporting the old-style SCSI 2
> > reservations and ALUA support when they can't be used at all with
> > the virtual backends, and can only be used in the corner case emulated
> > ALUA/PR support for pscsi?
> > 
> 
> It's the include/scsi/scsi.h SCSI_3 + SCSI_SPC_* version definition
> names that are incorrect:
> 
> #define SCSI_UNKNOWN    0
> #define SCSI_1          1
> #define SCSI_1_CCS      2
> #define SCSI_2          3
> #define SCSI_3          4        /* SPC */
> #define SCSI_SPC_2      5
> #define SCSI_SPC_3      6
> 
> from spc4r30 section 6.4.2 Standard INQUIRY data, Table 142 -- Version:
> 
>    00h     The device server does not claim conformance to any standard.
> 01h to 02h Obsolete
>    03h     The device server complies to ANSI INCITS 301-1997 (a withdrawn standard).
>    04h     The device server complies to ANSI INCITS 351-2001 (SPC-2).
>    05h     The device server complies to ANSI INCITS 408-2005 (SPC-3).
>    06h     The device server complies to this standard.
> 
> How about the following patch to fix the long-standing incorrect name
> usage of these three scsi.h defines..?

Because it's not incorrect.  Your confusion is that the SCSI_ constants
should match the inquiry data ... they shouldn't.  Look where we set
scsi_level in scsi_scan.c:708-713.  We have to fit an extra identifier
for SCSI_1_CCS so the scsi_level doesn't actually match the inquiry
data; it's incremented by 1 for SCSI_3 and above.  SCSI_3 does exist, by
the way, it was defined in the first version of SPC and there are some
devices which conform to it.

James

Re: standards revisions

[Nicholas A. Bellinger]

On Sun, 2012-10-07 at 09:16 +0100, James Bottomley wrote:
> On Sat, 2012-10-06 at 23:11 -0700, Nicholas A. Bellinger wrote:
> > On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> > > Currenly all non-pscsi bakcneds report their standards version as
> > > SPC 2 via ->get_device_rev.
> > 
> > No, the proper on-the-wire bits to signal SPC-3 compliance are already
> > being returned by virtual backend drivers within standard INQUIRY
> > payload data.  
> > 
> > Notice the comment:
> > 
> > root@tifa:/usr/src/target-pending.git# grep SCSI_SPC_2 drivers/target/*.c
> > drivers/target/target_core_file.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > drivers/target/target_core_iblock.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > drivers/target/target_core_rd.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> 
> That's causing confusion, I think!
> 
> > >  In addition to putting it into the
> > > inquirty data in spc_emulate_inquiry_std we also use it in two
> > > places to check on the version of the persistent reservation and
> > > alua support.  What is the benefit of supporting the old-style SCSI 2
> > > reservations and ALUA support when they can't be used at all with
> > > the virtual backends, and can only be used in the corner case emulated
> > > ALUA/PR support for pscsi?
> > > 
> > 
> > It's the include/scsi/scsi.h SCSI_3 + SCSI_SPC_* version definition
> > names that are incorrect:
> > 
> > #define SCSI_UNKNOWN    0
> > #define SCSI_1          1
> > #define SCSI_1_CCS      2
> > #define SCSI_2          3
> > #define SCSI_3          4        /* SPC */
> > #define SCSI_SPC_2      5
> > #define SCSI_SPC_3      6
> > 
> > from spc4r30 section 6.4.2 Standard INQUIRY data, Table 142 -- Version:
> > 
> >    00h     The device server does not claim conformance to any standard.
> > 01h to 02h Obsolete
> >    03h     The device server complies to ANSI INCITS 301-1997 (a withdrawn standard).
> >    04h     The device server complies to ANSI INCITS 351-2001 (SPC-2).
> >    05h     The device server complies to ANSI INCITS 408-2005 (SPC-3).
> >    06h     The device server complies to this standard.
> > 
> > How about the following patch to fix the long-standing incorrect name
> > usage of these three scsi.h defines..?
> 
> Because it's not incorrect.  Your confusion is that the SCSI_ constants
> should match the inquiry data ... they shouldn't.

No, my current confusion is stemming from why it's OK for SCSI_SPC_[2,3]
constant names+values to not match what is actually defined in SPC-4
drafts for what target INQUIRY emulation bits end up going 'over the
wire'.

> Look where we set
> scsi_level in scsi_scan.c:708-713.  We have to fit an extra identifier
> for SCSI_1_CCS so the scsi_level doesn't actually match the inquiry
> data; it's incremented by 1 for SCSI_3 and above.

The extra increment by 1 is required by some Linux/SCSI client
implementation dependent requirements, yes..?

> SCSI_3 does exist, by
> the way, it was defined in the first version of SPC and there are some
> devices which conform to it.
> 

Regardless, SCSI_SPC_[2,3] -> SCSI_SPC[3,4] constants for target-core +
scsi-core should still be re-named to avoid the extra confusion this
introduces for folks reading code.

--nab

Re: standards revisions

[James Bottomley]

On Sun, 2012-10-07 at 11:11 -0700, Nicholas A. Bellinger wrote:
> On Sun, 2012-10-07 at 09:16 +0100, James Bottomley wrote:
> > On Sat, 2012-10-06 at 23:11 -0700, Nicholas A. Bellinger wrote:
> > > On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> > > > Currenly all non-pscsi bakcneds report their standards version as
> > > > SPC 2 via ->get_device_rev.
> > > 
> > > No, the proper on-the-wire bits to signal SPC-3 compliance are already
> > > being returned by virtual backend drivers within standard INQUIRY
> > > payload data.  
> > > 
> > > Notice the comment:
> > > 
> > > root@tifa:/usr/src/target-pending.git# grep SCSI_SPC_2 drivers/target/*.c
> > > drivers/target/target_core_file.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > > drivers/target/target_core_iblock.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > > drivers/target/target_core_rd.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > 
> > That's causing confusion, I think!
> > 
> > > >  In addition to putting it into the
> > > > inquirty data in spc_emulate_inquiry_std we also use it in two
> > > > places to check on the version of the persistent reservation and
> > > > alua support.  What is the benefit of supporting the old-style SCSI 2
> > > > reservations and ALUA support when they can't be used at all with
> > > > the virtual backends, and can only be used in the corner case emulated
> > > > ALUA/PR support for pscsi?
> > > > 
> > > 
> > > It's the include/scsi/scsi.h SCSI_3 + SCSI_SPC_* version definition
> > > names that are incorrect:
> > > 
> > > #define SCSI_UNKNOWN    0
> > > #define SCSI_1          1
> > > #define SCSI_1_CCS      2
> > > #define SCSI_2          3
> > > #define SCSI_3          4        /* SPC */
> > > #define SCSI_SPC_2      5
> > > #define SCSI_SPC_3      6
> > > 
> > > from spc4r30 section 6.4.2 Standard INQUIRY data, Table 142 -- Version:
> > > 
> > >    00h     The device server does not claim conformance to any standard.
> > > 01h to 02h Obsolete
> > >    03h     The device server complies to ANSI INCITS 301-1997 (a withdrawn standard).
> > >    04h     The device server complies to ANSI INCITS 351-2001 (SPC-2).
> > >    05h     The device server complies to ANSI INCITS 408-2005 (SPC-3).
> > >    06h     The device server complies to this standard.
> > > 
> > > How about the following patch to fix the long-standing incorrect name
> > > usage of these three scsi.h defines..?
> > 
> > Because it's not incorrect.  Your confusion is that the SCSI_ constants
> > should match the inquiry data ... they shouldn't.
> 
> No, my current confusion is stemming from why it's OK for SCSI_SPC_[2,3]
> constant names+values to not match what is actually defined in SPC-4
> drafts for what target INQUIRY emulation bits end up going 'over the
> wire'.

OK, I don't understand what you didn't get about the explanation below.
But the gist is it's not a constant representing inquiry[2]&7; it's an
ordered set of enumerations representing gross capabilities abstracted
into sdev->scsi_level.

> > Look where we set
> > scsi_level in scsi_scan.c:708-713.  We have to fit an extra identifier
> > for SCSI_1_CCS so the scsi_level doesn't actually match the inquiry
> > data; it's incremented by 1 for SCSI_3 and above.
> 
> The extra increment by 1 is required by some Linux/SCSI client
> implementation dependent requirements, yes..?

Not at all, no.  It's defined and used in the mid-layer.  ULDs may
consult it or even (once in the case of usb) modify scsi_level, but they
don't use it for directly altering inquiry data.

> > SCSI_3 does exist, by
> > the way, it was defined in the first version of SPC and there are some
> > devices which conform to it.
> > 
> 
> Regardless, SCSI_SPC_[2,3] -> SCSI_SPC[3,4] constants for target-core +
> scsi-core should still be re-named to avoid the extra confusion this
> introduces for folks reading code.

I'm not convinced there is confusion; this use of enumerated levelling
goes back to 2.2 and no-one else has had a problem with it.  You're the
only one whose had an issue and it does seem to be because you didn't
bother reading the comment above their definitions in the header file
which does explain what's going on.

James

Re: standards revisions

[Nicholas A. Bellinger]

On Mon, 2012-10-08 at 08:45 +0100, James Bottomley wrote:
> On Sun, 2012-10-07 at 11:11 -0700, Nicholas A. Bellinger wrote:
> > On Sun, 2012-10-07 at 09:16 +0100, James Bottomley wrote:
> > > On Sat, 2012-10-06 at 23:11 -0700, Nicholas A. Bellinger wrote:
> > > > On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> > > > > Currenly all non-pscsi bakcneds report their standards version as
> > > > > SPC 2 via ->get_device_rev.
> > > > 
> > > > No, the proper on-the-wire bits to signal SPC-3 compliance are already
> > > > being returned by virtual backend drivers within standard INQUIRY
> > > > payload data.  
> > > > 
> > > > Notice the comment:
> > > > 
> > > > root@tifa:/usr/src/target-pending.git# grep SCSI_SPC_2 drivers/target/*.c
> > > > drivers/target/target_core_file.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > > > drivers/target/target_core_iblock.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > > > drivers/target/target_core_rd.c:	return SCSI_SPC_2; /* Returns SPC-3 in Initiator Data */
> > > 
> > > That's causing confusion, I think!
> > > 
> > > > >  In addition to putting it into the
> > > > > inquirty data in spc_emulate_inquiry_std we also use it in two
> > > > > places to check on the version of the persistent reservation and
> > > > > alua support.  What is the benefit of supporting the old-style SCSI 2
> > > > > reservations and ALUA support when they can't be used at all with
> > > > > the virtual backends, and can only be used in the corner case emulated
> > > > > ALUA/PR support for pscsi?
> > > > > 
> > > > 
> > > > It's the include/scsi/scsi.h SCSI_3 + SCSI_SPC_* version definition
> > > > names that are incorrect:
> > > > 
> > > > #define SCSI_UNKNOWN    0
> > > > #define SCSI_1          1
> > > > #define SCSI_1_CCS      2
> > > > #define SCSI_2          3
> > > > #define SCSI_3          4        /* SPC */
> > > > #define SCSI_SPC_2      5
> > > > #define SCSI_SPC_3      6
> > > > 
> > > > from spc4r30 section 6.4.2 Standard INQUIRY data, Table 142 -- Version:
> > > > 
> > > >    00h     The device server does not claim conformance to any standard.
> > > > 01h to 02h Obsolete
> > > >    03h     The device server complies to ANSI INCITS 301-1997 (a withdrawn standard).
> > > >    04h     The device server complies to ANSI INCITS 351-2001 (SPC-2).
> > > >    05h     The device server complies to ANSI INCITS 408-2005 (SPC-3).
> > > >    06h     The device server complies to this standard.
> > > > 
> > > > How about the following patch to fix the long-standing incorrect name
> > > > usage of these three scsi.h defines..?
> > > 
> > > Because it's not incorrect.  Your confusion is that the SCSI_ constants
> > > should match the inquiry data ... they shouldn't.
> > 
> > No, my current confusion is stemming from why it's OK for SCSI_SPC_[2,3]
> > constant names+values to not match what is actually defined in SPC-4
> > drafts for what target INQUIRY emulation bits end up going 'over the
> > wire'.
> 
> OK, I don't understand what you didn't get about the explanation below.
> But the gist is it's not a constant representing inquiry[2]&7; it's an
> ordered set of enumerations representing gross capabilities abstracted
> into sdev->scsi_level.
> 

Yes, there is no confusion on how scsi-core is working here..

> > > SCSI_3 does exist, by
> > > the way, it was defined in the first version of SPC and there are some
> > > devices which conform to it.
> > > 
> > 
> > Regardless, SCSI_SPC_[2,3] -> SCSI_SPC[3,4] constants for target-core +
> > scsi-core should still be re-named to avoid the extra confusion this
> > introduces for folks reading code.
> 
> I'm not convinced there is confusion; this use of enumerated levelling
> goes back to 2.2 and no-one else has had a problem with it.  You're the
> only one whose had an issue and it does seem to be because you didn't
> bother reading the comment above their definitions in the header file
> which does explain what's going on.
> 

The point is that it would be nice to have constants representing
on-the-wire values in scsi.h that both scsi-core and target-core can
use.

What about just defining the proper 'on-the-wire' that target-core needs
instead..?

diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 66216c1..13ee02b 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -541,6 +541,13 @@ static inline int scsi_is_wlun(unsigned int lun)
 #define SCSI_SPC_3      6
 
 /*
+ * ANSI INCITS Standard INQUIRY resp[2] version bits
+ */
+#define SCSI_ANSIVER_SPC2      0x04    /* ANSI INCITS 351-2001 (SPC-2) */
+#define SCSI_ANSIVER_SPC3      0x05    /* ANSI INCITS 408-2005 (SPC-3) */
+#define SCSI_ANSIVER_SPC4      0x06    /* SPC-4 standard draft */
+
+/*
  * INQ PERIPHERAL QUALIFIERS
  */
 #define SCSI_INQ_PQ_CON         0x00

Re: standards revisions

[Christoph Hellwig]

On Sat, Oct 06, 2012 at 11:11:50PM -0700, Nicholas A. Bellinger wrote:
> On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> > Currenly all non-pscsi bakcneds report their standards version as
> > SPC 2 via ->get_device_rev.
> 
> No, the proper on-the-wire bits to signal SPC-3 compliance are already
> being returned by virtual backend drivers within standard INQUIRY
> payload data.  

I missed that, but it doesn't matter for the point I was making, which
is the code to special case the SCSI_2 case, which can't happen for
any virtual backend.  In addition it also can't happen for pscsi as
we don't wire up any command emulation but REPORT LUNS for it any more,
effectively making it dead code.

Re: standards revisions

[Nicholas A. Bellinger]

On Sun, 2012-10-07 at 10:51 -0400, Christoph Hellwig wrote: 
> On Sat, Oct 06, 2012 at 11:11:50PM -0700, Nicholas A. Bellinger wrote:
> > On Sat, 2012-10-06 at 21:49 -0400, Christoph Hellwig wrote:
> > > Currenly all non-pscsi bakcneds report their standards version as
> > > SPC 2 via ->get_device_rev.
> > 
> > No, the proper on-the-wire bits to signal SPC-3 compliance are already
> > being returned by virtual backend drivers within standard INQUIRY
> > payload data.  
> 
> I missed that, but it doesn't matter for the point I was making, which
> is the code to special case the SCSI_2 case, which can't happen for
> any virtual backend.

Regardless of if an virtual backend reports a SPC-3 version (0x05) in
INQUIRY response, an initiator is still allowed to fall back to using
legacy SCSI-2 reservations instead of SPC-3 persistent reservations.  I
can think of at least one mainstream SCSI initiator that does this.

Also, I think your mistaken about ALUA and SCSI-2 compatibility.  ALUA
is an SPC-3 and above specific feature.

> In addition it also can't happen for pscsi as
> we don't wire up any command emulation but REPORT LUNS for it any more,
> effectively making it dead code.
> 

pSCSI has always NOP'ed the reservation + ALUA function pointers within
core_setup_reservations() and core_setup_alua().

--nab

Re: standards revisions

[Christoph Hellwig]

On Sun, Oct 07, 2012 at 10:31:37AM -0700, Nicholas A. Bellinger wrote:
> Regardless of if an virtual backend reports a SPC-3 version (0x05) in
> INQUIRY response, an initiator is still allowed to fall back to using
> legacy SCSI-2 reservations instead of SPC-3 persistent reservations.  I
> can think of at least one mainstream SCSI initiator that does this.

Yes, but we have a different code path for the mixed SCSI-2/SPC-3
reservations from the pure SCSI-2 mode, based on the function pointers
set up in core_setup_reservations().  As far as I can see we could
only reach the SCSI-2 path there for a pscsi device that has the
emulate_reservations flag set, and even then we would never actually hit
the code the function pointers point to as we don't actually support
emulated reservations for pscsi.

> Also, I think your mistaken about ALUA and SCSI-2 compatibility.  ALUA
> is an SPC-3 and above specific feature.

What I mean is that int core_setup_alua again has a special code path
for < SCSI-3 levels, which has the same reachability issues as for the
reservations above.

> pSCSI has always NOP'ed the reservation + ALUA function pointers within
> core_setup_reservations() and core_setup_alua().

Unless the emulate_reservations or emulate_alua flags are set, in which
case we will set the other functions pointers.  That being said I can't
see why the function pointers are even needed, as the non-noop versions
are careful enough to do the right thing for pscsi as we'll never have
registrations set.  I'll send a series of patches to clean all this up.

Re: standards revisions

[Nicholas A. Bellinger]

On Sun, 2012-10-07 at 23:44 -0400, Christoph Hellwig wrote:
> On Sun, Oct 07, 2012 at 10:31:37AM -0700, Nicholas A. Bellinger wrote:
> > Regardless of if an virtual backend reports a SPC-3 version (0x05) in
> > INQUIRY response, an initiator is still allowed to fall back to using
> > legacy SCSI-2 reservations instead of SPC-3 persistent reservations.  I
> > can think of at least one mainstream SCSI initiator that does this.
> 
> Yes, but we have a different code path for the mixed SCSI-2/SPC-3
> reservations from the pure SCSI-2 mode, based on the function pointers
> set up in core_setup_reservations().  As far as I can see we could
> only reach the SCSI-2 path there for a pscsi device that has the
> emulate_reservations flag set, and even then we would never actually hit
> the code the function pointers point to as we don't actually support
> emulated reservations for pscsi.

Ok, I see what your getting now wrt to dead code during ALUA/PR init..

So yes, the two SPC2_ALUA_DISABLED and SPC2_RESERVATIONS cases are not
ever reached, and where originally intended to disable this emulation
for virtual backends based upon se_subsystem_api->get_device_rev()

> 
> > Also, I think your mistaken about ALUA and SCSI-2 compatibility.  ALUA
> > is an SPC-3 and above specific feature.
> 
> What I mean is that int core_setup_alua again has a special code path
> for < SCSI-3 levels, which has the same reachability issues as for the
> reservations above.
> 
> > pSCSI has always NOP'ed the reservation + ALUA function pointers within
> > core_setup_reservations() and core_setup_alua().
> 
> Unless the emulate_reservations or emulate_alua flags are set, in which
> case we will set the other functions pointers.  That being said I can't
> see why the function pointers are even needed, as the non-noop versions
> are careful enough to do the right thing for pscsi as we'll never have
> registrations set.  I'll send a series of patches to clean all this up.
> 

<nod>

Thanks Christoph !