Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Nicholas A. Bellinger]

Greetings mnc and tomo,

So as per our discussions last week, I will be moving forward with the
conversion of the LIO-Target iSCSI fabric module v4 to use protocol and
PDU include/scsi/iscsi_proto.h.  As mnc and I agreed privately, this is
going to the right level of integration for v2.6.37, as making libiscsi
target mode aware does not really make for a target mode iSCSI stack
sense due to the amount of logic in place for kernel <-> user with the
Open-iSCSI initiator.

So aside from the main tedious work that will be involved in actually
producing a patch for this on my end, I did notice one immediate item
that will cause an extra headache..  This involves the struct
iscsi_init_* and struct iscsi_target* structure definitions in:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/lio-target/iscsi_protocol.h;hb=refs/heads/lio-4.0

namely that they all have trailing 'u32 header_digest' member that a
decent amount of utility code in iscsi_target.c and iscsi_target_util.c
currently depends upon.

So, assuming that the conversion of all struct iscsi_init_* and struct
iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
checking structure size and renaming the member use in
drivers/target/lio-target, would it be acceptable to do something like:

/* iSCSI PDU Header */
struct iscsi_cmd {
        uint8_t opcode;
        uint8_t flags;
        __be16 rsvd2;
        uint8_t hlength;
        uint8_t dlength[3];
        uint8_t lun[8];
        itt_t    itt;   /* Initiator Task Tag */
        __be32 data_length;
        __be32 cmdsn;
        __be32 exp_statsn;
        uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
        /* Additional Data (Command Dependent) */
#ifdef ISCSI_TARGET_MODE
        __be32 header_digest;
#endif
};

for the existing PDU defs in iscsi_proto.h in order to the pain
of having to convert this over in existing LIO-Target fabric module
code..?  This really help me alot.

Thanks!

--nab

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[FUJITA Tomonori]

On Thu, 19 Aug 2010 15:45:47 -0700
"Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

> So, assuming that the conversion of all struct iscsi_init_* and struct
> iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
> checking structure size and renaming the member use in
> drivers/target/lio-target, would it be acceptable to do something like:
> 
> /* iSCSI PDU Header */
> struct iscsi_cmd {
>         uint8_t opcode;
>         uint8_t flags;
>         __be16 rsvd2;
>         uint8_t hlength;
>         uint8_t dlength[3];
>         uint8_t lun[8];
>         itt_t    itt;   /* Initiator Task Tag */
>         __be32 data_length;
>         __be32 cmdsn;
>         __be32 exp_statsn;
>         uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
>         /* Additional Data (Command Dependent) */
> #ifdef ISCSI_TARGET_MODE
>         __be32 header_digest;
> #endif
> };

It's up to Mike but it looks hacky to me. struct iscsi_pdu is defined
in the exact way as the spec does.

I don't think that iscsi_proto.h conversion is a must for the mainline
inclusion. I prefer to let it alone for now.

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Nicholas A. Bellinger]

On Fri, 2010-08-20 at 16:32 +0900, FUJITA Tomonori wrote:
> On Thu, 19 Aug 2010 15:45:47 -0700
> "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
> 
> > So, assuming that the conversion of all struct iscsi_init_* and struct
> > iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
> > checking structure size and renaming the member use in
> > drivers/target/lio-target, would it be acceptable to do something like:
> > 
> > /* iSCSI PDU Header */
> > struct iscsi_cmd {
> >         uint8_t opcode;
> >         uint8_t flags;
> >         __be16 rsvd2;
> >         uint8_t hlength;
> >         uint8_t dlength[3];
> >         uint8_t lun[8];
> >         itt_t    itt;   /* Initiator Task Tag */
> >         __be32 data_length;
> >         __be32 cmdsn;
> >         __be32 exp_statsn;
> >         uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
> >         /* Additional Data (Command Dependent) */
> > #ifdef ISCSI_TARGET_MODE
> >         __be32 header_digest;
> > #endif
> > };
> 
> It's up to Mike but it looks hacky to me. struct iscsi_pdu is defined
> in the exact way as the spec does.
> 
> I don't think that iscsi_proto.h conversion is a must for the mainline
> inclusion. I prefer to let it alone for now.

Hmmmm, Ok..  Then I will defer to mnc's judgement here about what level
of integration of LIO-Target for protocol and PDU defs for what he
currently handles with Open-iSCSI.  Mike, do you have any more thoughts
here..?

I am pretty sure there are more important items to focus on inside from
this particuarly time-consuming and tedious cleanup, but am happy to
start a smaller cleanup of drivers/target/lio-target/iscsi_protocol.h
for starters..

Also, LIO-Target is still using an internal CRC32C implementation, which
needs to be converted to libcrypto and crc32c.ko.  Which also reminds
me, it would be nice to get the slicing by 8 CRC32C support into
libcrypto, and properly support for the Nehalem's CRC32C instruction
capabilities too.  Any takers..?  ;)

Best,

--nab

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[FUJITA Tomonori]

On Fri, 20 Aug 2010 02:14:34 -0700
"Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

> I am pretty sure there are more important items to focus on inside from
> this particuarly time-consuming and tedious cleanup, but am happy to

btw, it would be better to focus on the target core code first rather
than its iscsi target driver? The mainline supports only ibm vscsi
target now. So the target core code and its new vscsi driver can be
merged to replace the current mainline target infrastructure and its
vscsi driver, I think. the iscsi target driver is a "new" feature,
which can be added later. No need to merge it with the target core
code (of course, we can merge it together if possible).

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Nicholas A. Bellinger]

On Fri, 2010-08-20 at 18:41 +0900, FUJITA Tomonori wrote:
> On Fri, 20 Aug 2010 02:14:34 -0700
> "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
> 
> > I am pretty sure there are more important items to focus on inside from
> > this particuarly time-consuming and tedious cleanup, but am happy to
> 
> btw, it would be better to focus on the target core code first rather
> than its iscsi target driver?

Well, this is what was originally proposed for v2.6.35.  But considering
the LIO-Target namesake and it's full use of logic from the TCM/ConfigFS
v4 design, I believe that hch thought it made sense to include it
together moving forward with TCM Core now that we are moving forward
together.

> The mainline supports only ibm vscsi
> target now. So the target core code and its new vscsi driver can be
> merged to replace the current mainline target infrastructure and its
> vscsi driver, I think. the iscsi target driver is a "new" feature,
> which can be added later. No need to merge it with the target core
> code (of course, we can merge it together if possible).

I can plan to keep the posting of drivers/target/lio-target/ code
seperate, and after the upcoming public posting to linux-scsi for your
review of TCM core and TCM_Loop.   But perhaps at this point LIO-Target
should go in together with TCM Core and TCM_Loop once mnc can review the
software iSCSI target pieces..?

I am very happy to continue in the short term improving the LIO-Target
fabric module together with mnc to use mainline code and convention so
that we can ensure this piece is included together with TCM Core for
v2.6.37.  

Comments from other Linux/iSCSI folks is apperciated here..!

Best,

--nab

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[FUJITA Tomonori]

On Fri, 20 Aug 2010 03:02:40 -0700
"Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

> review of TCM core and TCM_Loop.   But perhaps at this point LIO-Target
> should go in together with TCM Core and TCM_Loop once mnc can review the
> software iSCSI target pieces..?

I think that James meant that we have only one scsi target
implementation and we need to replace the current in-kernel target
mode driver. So I don't think that we can merge TCM core without its
ibm vscsi driver. I might be wrong though.

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Nicholas A. Bellinger]

On Fri, 2010-08-20 at 19:43 +0900, FUJITA Tomonori wrote:
> On Fri, 20 Aug 2010 03:02:40 -0700
> "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
> 
> > review of TCM core and TCM_Loop.   But perhaps at this point LIO-Target
> > should go in together with TCM Core and TCM_Loop once mnc can review the
> > software iSCSI target pieces..?
> 
> I think that James meant that we have only one scsi target
> implementation and we need to replace the current in-kernel target
> mode driver. So I don't think that we can merge TCM core without its
> ibm vscsi driver. I might be wrong though.

Ah yes the ibm vscsi driver.   Well for that part I suppose we will need
a seperate tcm_ibmvscsi fabric module moving forward.  This is where
that python script I mentioned when tcm_lpfc, tcm_qla2xxx, tcm_mvsas
where originally posted that would generates a new functioning configfs
skeleton with NOP I/O path handlers from a handful of user input.

Anyways, something like this would really come in handy for making ibm
vscsi go, and plus it makes life easier for me too.  I will try to get a
script along this lines coded up in the next week(s) so that we can
finally have a pain-free method of generation for new v4 kernel target
mode modules for new developers.

There will be no limit per customer.  ;)

--nab

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Mike Christie]

On 08/20/2010 02:32 AM, FUJITA Tomonori wrote:
> On Thu, 19 Aug 2010 15:45:47 -0700
> "Nicholas A. Bellinger"<nab@linux-iscsi.org>  wrote:
>
>> So, assuming that the conversion of all struct iscsi_init_* and struct
>> iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
>> checking structure size and renaming the member use in
>> drivers/target/lio-target, would it be acceptable to do something like:
>>
>> /* iSCSI PDU Header */
>> struct iscsi_cmd {
>>          uint8_t opcode;
>>          uint8_t flags;
>>          __be16 rsvd2;
>>          uint8_t hlength;
>>          uint8_t dlength[3];
>>          uint8_t lun[8];
>>          itt_t    itt;   /* Initiator Task Tag */
>>          __be32 data_length;
>>          __be32 cmdsn;
>>          __be32 exp_statsn;
>>          uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
>>          /* Additional Data (Command Dependent) */
>> #ifdef ISCSI_TARGET_MODE
>>          __be32 header_digest;
>> #endif
>> };
>
> It's up to Mike but it looks hacky to me. struct iscsi_pdu is defined
> in the exact way as the spec does.
>
> I don't think that iscsi_proto.h conversion is a must for the mainline
> inclusion. I prefer to let it alone for now.

For the iscsi target, I think it is. For iscsi initiator drivers like 
bnx2i and be2iscsi we have made them convert from their headers to 
common linux ones including switching from driver specific defs to 
iscsi_proto.h.

For the merging of any target core stuff though, I do not think it is 
critical. The core target stuff does not need to be merged with a 
software iscsi target. They can do in at different times.

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Nicholas A. Bellinger]

On Fri, 2010-08-20 at 16:41 -0500, Mike Christie wrote:
> On 08/20/2010 02:32 AM, FUJITA Tomonori wrote:
> > On Thu, 19 Aug 2010 15:45:47 -0700
> > "Nicholas A. Bellinger"<nab@linux-iscsi.org>  wrote:
> >
> >> So, assuming that the conversion of all struct iscsi_init_* and struct
> >> iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
> >> checking structure size and renaming the member use in
> >> drivers/target/lio-target, would it be acceptable to do something like:
> >>
> >> /* iSCSI PDU Header */
> >> struct iscsi_cmd {
> >>          uint8_t opcode;
> >>          uint8_t flags;
> >>          __be16 rsvd2;
> >>          uint8_t hlength;
> >>          uint8_t dlength[3];
> >>          uint8_t lun[8];
> >>          itt_t    itt;   /* Initiator Task Tag */
> >>          __be32 data_length;
> >>          __be32 cmdsn;
> >>          __be32 exp_statsn;
> >>          uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
> >>          /* Additional Data (Command Dependent) */
> >> #ifdef ISCSI_TARGET_MODE
> >>          __be32 header_digest;
> >> #endif
> >> };
> >
> > It's up to Mike but it looks hacky to me. struct iscsi_pdu is defined
> > in the exact way as the spec does.
> >
> > I don't think that iscsi_proto.h conversion is a must for the mainline
> > inclusion. I prefer to let it alone for now.
> 
> For the iscsi target, I think it is. For iscsi initiator drivers like 
> bnx2i and be2iscsi we have made them convert from their headers to 
> common linux ones including switching from driver specific defs to 
> iscsi_proto.h.

This is kinda what I figured myself..  So in that case, I will go ahead
and plan on using the initial target mode stubs above to get up and
running, and start doing this sometime after the next (seperate) posting
of TCM_Core + TCM_Loop for Tomo-san's review. 

> 
> For the merging of any target core stuff though, I do not think it is 
> critical. The core target stuff does not need to be merged with a 
> software iscsi target. They can do in at different times.

I think in order to merge the software iSCSI target fabric bits for
v2.6.37, it would sense to do it initially with something similar to
these stubs.  From there I can have a more time to make necessary
modifications to LIO-Target and plan removing the header_digest target
stubs from iscsi_proto.h in for the .38 timeframe.

But really, since you so kindly volunteered to review the
drivers/target/lio-target code to start with, this decision is most
certainly yours to me made as the LIO-Target conversion to iscsi_proto.h
starts rolling..  8-)

Thanks for your comments Mike!

--nab

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Boaz Harrosh]

On 08/20/2010 01:45 AM, Nicholas A. Bellinger wrote:
> Greetings mnc and tomo,
> 
> So as per our discussions last week, I will be moving forward with the
> conversion of the LIO-Target iSCSI fabric module v4 to use protocol and
> PDU include/scsi/iscsi_proto.h.  As mnc and I agreed privately, this is
> going to the right level of integration for v2.6.37, as making libiscsi
> target mode aware does not really make for a target mode iSCSI stack
> sense due to the amount of logic in place for kernel <-> user with the
> Open-iSCSI initiator.
> 
> So aside from the main tedious work that will be involved in actually
> producing a patch for this on my end, I did notice one immediate item
> that will cause an extra headache..  This involves the struct
> iscsi_init_* and struct iscsi_target* structure definitions in:
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/lio-target/iscsi_protocol.h;hb=refs/heads/lio-4.0
> 
> namely that they all have trailing 'u32 header_digest' member that a
> decent amount of utility code in iscsi_target.c and iscsi_target_util.c
> currently depends upon.
> 
> So, assuming that the conversion of all struct iscsi_init_* and struct
> iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
> checking structure size and renaming the member use in
> drivers/target/lio-target, would it be acceptable to do something like:
> 
> /* iSCSI PDU Header */
> struct iscsi_cmd {
>         uint8_t opcode;
>         uint8_t flags;
>         __be16 rsvd2;
>         uint8_t hlength;
>         uint8_t dlength[3];
>         uint8_t lun[8];
>         itt_t    itt;   /* Initiator Task Tag */
>         __be32 data_length;
>         __be32 cmdsn;
>         __be32 exp_statsn;
>         uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
>         /* Additional Data (Command Dependent) */
> #ifdef ISCSI_TARGET_MODE
>         __be32 header_digest;
> #endif
> };
> 

No, Nic that will not work. struct iscsi_cmd is just part of a full header
that can have additional AHSs before the final header_digest that
signs them all. Here above is just the common first part.
(We will need these AHSs support sooner then later in LIO as well ;-))

There are other structs that do include the above + header_digest
as an aggregate that you can use.

two things:
1. Still jet-lagged / summer vacation family duties. Let me get
   up to speed, I'll even send some ruff sketches for you.

2. Please get me up to speed with a ruff code map (Which files?
   where?) for the iscsi code. I can do both the bidi and the AHSs
   for you, which are very related. Actually AHSs is the harder part
   (which is simple really).
   a. Any high level documentation.
   b. The best code for review. Both git-tree to pull, and git-web
      if available.

> for the existing PDU defs in iscsi_proto.h in order to the pain
> of having to convert this over in existing LIO-Target fabric module
> code..?  This really help me alot.
> 
> Thanks!
> 

Thanks
Boaz

> --nab
> 

Re: Conversion of LIO-Target to use include/scsi/iscsi_proto.h defs

[Nicholas A. Bellinger]

On Sun, 2010-08-22 at 22:06 +0300, Boaz Harrosh wrote:
> On 08/20/2010 01:45 AM, Nicholas A. Bellinger wrote:
> > Greetings mnc and tomo,
> > 
> > So as per our discussions last week, I will be moving forward with the
> > conversion of the LIO-Target iSCSI fabric module v4 to use protocol and
> > PDU include/scsi/iscsi_proto.h.  As mnc and I agreed privately, this is
> > going to the right level of integration for v2.6.37, as making libiscsi
> > target mode aware does not really make for a target mode iSCSI stack
> > sense due to the amount of logic in place for kernel <-> user with the
> > Open-iSCSI initiator.
> > 
> > So aside from the main tedious work that will be involved in actually
> > producing a patch for this on my end, I did notice one immediate item
> > that will cause an extra headache..  This involves the struct
> > iscsi_init_* and struct iscsi_target* structure definitions in:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/lio-target/iscsi_protocol.h;hb=refs/heads/lio-4.0
> > 
> > namely that they all have trailing 'u32 header_digest' member that a
> > decent amount of utility code in iscsi_target.c and iscsi_target_util.c
> > currently depends upon.
> > 
> > So, assuming that the conversion of all struct iscsi_init_* and struct
> > iscsi_target* to use include/scsi/iscsi_proto.h defs just involves
> > checking structure size and renaming the member use in
> > drivers/target/lio-target, would it be acceptable to do something like:
> > 
> > /* iSCSI PDU Header */
> > struct iscsi_cmd {
> >         uint8_t opcode;
> >         uint8_t flags;
> >         __be16 rsvd2;
> >         uint8_t hlength;
> >         uint8_t dlength[3];
> >         uint8_t lun[8];
> >         itt_t    itt;   /* Initiator Task Tag */
> >         __be32 data_length;
> >         __be32 cmdsn;
> >         __be32 exp_statsn;
> >         uint8_t cdb[ISCSI_CDB_SIZE];    /* SCSI Command Block */
> >         /* Additional Data (Command Dependent) */
> > #ifdef ISCSI_TARGET_MODE
> >         __be32 header_digest;
> > #endif
> > };
> > 
> 
> No, Nic that will not work. struct iscsi_cmd is just part of a full header
> that can have additional AHSs before the final header_digest that
> signs them all. Here above is just the common first part.
> (We will need these AHSs support sooner then later in LIO as well ;-))
>
> There are other structs that do include the above + header_digest
> as an aggregate that you can use.
> 

Hmmm good point..  Perhaps the choice of using struct iscsi_cmd as the
example w/ __be32 header_digest was made a little hastely on my part.
8-)

So in that case, I can focus making this struct iscsi_cmd function W/O
the header_digest member first, and have the temporary stubs in the
lio-core-2.6.git tree for the other PDUs as required until the other non
SCSI PDU handlers can be converted and the stubs removed.

> two things:
> 1. Still jet-lagged / summer vacation family duties. Let me get
>    up to speed, I'll even send some ruff sketches for you.
> 

Great!  I am looking forward to those.

> 2. Please get me up to speed with a ruff code map (Which files?
>    where?) for the iscsi code. I can do both the bidi and the AHSs
>    for you, which are very related. Actually AHSs is the harder part
>    (which is simple really).

<nod> I would like to start adding BIDI into TCM Core + TCM_Loop very
soon (mabye by this weekend..?)  Ditto on the extended > 16-byte CDB
support bit, which should be straight-forward enough in TCM Core, and on
the backend for the TCM/pSCSI using struct request passthrough..

>    a. Any high level documentation.

Ok, I need start to putting this together in Documentation/target/ for
the lio-target iSCSI specific pieces..  My plan is to start working on
the struct target_core_fabric_ops API documentation first to go with
this tcm_mod_builder.py configfs skeleton fabric module generation
script that I am considering.

>From there, it would next make sense to produce high level docs for
struct se_subsystem_api and primary Linux storage subsystem plugins eg:
IBLOCK, pSCSI, FILEIO for folks interested in that area.

I will be sure to CC you for input on these doc commits.

>    b. The best code for review. Both git-tree to pull, and git-web
>       if available.
> 

This is the lio-core-2.6.git/lio-4.0 branch which is currently still at
v2.6.35.  Just FYI I plan to jump to .36-rc2 at some point during the
week.

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=shortlog;h=refs/heads/lio-4.0

Thanks for your comments Boaz!

--nab