| Message ID | 1531696591-8558-3-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > The isid is 48 bits, and in hex string format it's 12 bytes. > We are currently copying the 12 byte hex string to a u64 > so we can easily compare it, but this has the problem that > only 8 bytes of the 12 bytes are copied. > > The next patches will want to print se_session sess_bin_isid > so this has us use hex2bin to when converting from the hex > sting to the bin value. ^^^^^ string? Which spec defines that an ISID is 48 bits? All I know about SCSI registrations is that these involve a transport ID and that that transport ID can be up to 228 bytes long for iSCSI. > if (isid != NULL) { > - pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid); > + if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) { > + pr_err("Invalid isid %s\n", isid); > + goto free_reg; > + } Why is it necessary to convert the ISID from hex to binary format? If this conversion is necessary, isn't that something that should be handled by the iSCSI target driver instead of the target core? Thanks, Bart.
On 07/18/2018 05:09 PM, Bart Van Assche wrote: > On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: >> The isid is 48 bits, and in hex string format it's 12 bytes. >> We are currently copying the 12 byte hex string to a u64 >> so we can easily compare it, but this has the problem that >> only 8 bytes of the 12 bytes are copied. >> >> The next patches will want to print se_session sess_bin_isid >> so this has us use hex2bin to when converting from the hex >> sting to the bin value. > ^^^^^ > string? > > Which spec defines that an ISID is 48 bits? All I know about SCSI registrations The iscsi spec defines it as 48 bits. > is that these involve a transport ID and that that transport ID can be up to 228 > bytes long for iSCSI. I am talking about the Initiator Session ID above. That along with the iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout table 508 or in SAM 5r21 checkout table A.4. So in the SCSI specs as part of the Initiator Port Transport ID we have this from that SAM table: The Initiator Session Identifier (ISID) portion of the string is a UTF-8 encoded hexadecimal representation of a six byte binary value. --- In the PR parts of SPC it sometimes mentions only "Transport ID" but then clarifies the initiator port so I am assuming in those cases it means "Initiator Port Transport ID" so it is both the name and isid for iscsi. > >> if (isid != NULL) { >> - pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid); >> + if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) { >> + pr_err("Invalid isid %s\n", isid); >> + goto free_reg; >> + } > > Why is it necessary to convert the ISID from hex to binary format? If this > conversion is necessary, isn't that something that should be handled by the > iSCSI target driver instead of the target core? > The target drivers get the value as a 48 bit binary value. The PR code gets it in the string format as describe above. I had thought the code preferred to store it in the binary format because it was easier to test than comparing strings or maybe for speed, so I just fixed that code. I think we can just keep it in string format in __transport_register_session then just compare strings in target_core_pr.c.
On 07/18/2018 07:03 PM, Mike Christie wrote: > On 07/18/2018 05:09 PM, Bart Van Assche wrote: >> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: >>> The isid is 48 bits, and in hex string format it's 12 bytes. >>> We are currently copying the 12 byte hex string to a u64 >>> so we can easily compare it, but this has the problem that >>> only 8 bytes of the 12 bytes are copied. >>> >>> The next patches will want to print se_session sess_bin_isid >>> so this has us use hex2bin to when converting from the hex >>> sting to the bin value. >> ^^^^^ >> string? >> >> Which spec defines that an ISID is 48 bits? All I know about SCSI registrations > > The iscsi spec defines it as 48 bits. > >> is that these involve a transport ID and that that transport ID can be up to 228 >> bytes long for iSCSI. > > I am talking about the Initiator Session ID above. That along with the > iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout > table 508 or in SAM 5r21 checkout table A.4. > > So in the SCSI specs as part of the Initiator Port Transport ID we have > this from that SAM table: > > The Initiator Session Identifier (ISID) portion of the string is a UTF-8 > encoded hexadecimal representation of a six byte binary value. > > --- > > In the PR parts of SPC it sometimes mentions only "Transport ID" but > then clarifies the initiator port so I am assuming in those cases it > means "Initiator Port Transport ID" so it is both the name and isid for > iscsi. It looks like we are supposed to go by what the initiator specifies in the TPID field, so it can be either the Transport ID or Initiator Port Transport ID.
On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: > On 07/18/2018 07:03 PM, Mike Christie wrote: > > On 07/18/2018 05:09 PM, Bart Van Assche wrote: > > > [ ... ] > > > is that these involve a transport ID and that that transport ID can be up to 228 > > > bytes long for iSCSI. > > > > I am talking about the Initiator Session ID above. That along with the > > iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout > > table 508 or in SAM 5r21 checkout table A.4. > > > > So in the SCSI specs as part of the Initiator Port Transport ID we have > > this from that SAM table: > > > > The Initiator Session Identifier (ISID) portion of the string is a UTF-8 > > encoded hexadecimal representation of a six byte binary value. > > > > --- > > > > In the PR parts of SPC it sometimes mentions only "Transport ID" but > > then clarifies the initiator port so I am assuming in those cases it > > means "Initiator Port Transport ID" so it is both the name and isid for > > iscsi. > > It looks like we are supposed to go by what the initiator specifies in > the TPID field, so it can be either the Transport ID or Initiator Port > Transport ID. Hello Mike, Since the ISID is iSCSI-specific I think that all code that knows about the ISID and its encoding should be in the iSCSI target driver instead of the target core. Do you think an approach similar to that of the SCST function iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller of that function is in source file scst/src/scst_targ.c: res = sess->tgt->tgtt->get_initiator_port_transport_id( sess->tgt, sess, &sess->transport_id); Thanks, Bart.
On 07/19/2018 10:15 AM, Bart Van Assche wrote: > On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: >> On 07/18/2018 07:03 PM, Mike Christie wrote: >>> On 07/18/2018 05:09 PM, Bart Van Assche wrote: >>>> [ ... ] >>>> is that these involve a transport ID and that that transport ID can be up to 228 >>>> bytes long for iSCSI. >>> >>> I am talking about the Initiator Session ID above. That along with the >>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout >>> table 508 or in SAM 5r21 checkout table A.4. >>> >>> So in the SCSI specs as part of the Initiator Port Transport ID we have >>> this from that SAM table: >>> >>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8 >>> encoded hexadecimal representation of a six byte binary value. >>> >>> --- >>> >>> In the PR parts of SPC it sometimes mentions only "Transport ID" but >>> then clarifies the initiator port so I am assuming in those cases it >>> means "Initiator Port Transport ID" so it is both the name and isid for >>> iscsi. >> >> It looks like we are supposed to go by what the initiator specifies in >> the TPID field, so it can be either the Transport ID or Initiator Port >> Transport ID. > > Hello Mike, > > Since the ISID is iSCSI-specific I think that all code that knows about the > ISID and its encoding should be in the iSCSI target driver instead of the > target core. Do you think an approach similar to that of the SCST function > iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller Yeah, I can make that work. > of that function is in source file scst/src/scst_targ.c: > > res = sess->tgt->tgtt->get_initiator_port_transport_id( > sess->tgt, sess, &sess->transport_id); > > Thanks, > > Bart.-- > To unsubscribe from this list: send the line "unsubscribe target-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 07/19/2018 11:13 AM, Mike Christie wrote: > On 07/19/2018 10:15 AM, Bart Van Assche wrote: >> On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: >>> On 07/18/2018 07:03 PM, Mike Christie wrote: >>>> On 07/18/2018 05:09 PM, Bart Van Assche wrote: >>>>> [ ... ] >>>>> is that these involve a transport ID and that that transport ID can be up to 228 >>>>> bytes long for iSCSI. >>>> >>>> I am talking about the Initiator Session ID above. That along with the >>>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout >>>> table 508 or in SAM 5r21 checkout table A.4. >>>> >>>> So in the SCSI specs as part of the Initiator Port Transport ID we have >>>> this from that SAM table: >>>> >>>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8 >>>> encoded hexadecimal representation of a six byte binary value. >>>> >>>> --- >>>> >>>> In the PR parts of SPC it sometimes mentions only "Transport ID" but >>>> then clarifies the initiator port so I am assuming in those cases it >>>> means "Initiator Port Transport ID" so it is both the name and isid for >>>> iscsi. >>> >>> It looks like we are supposed to go by what the initiator specifies in >>> the TPID field, so it can be either the Transport ID or Initiator Port >>> Transport ID. >> >> Hello Mike, >> >> Since the ISID is iSCSI-specific I think that all code that knows about the >> ISID and its encoding should be in the iSCSI target driver instead of the >> target core. Do you think an approach similar to that of the SCST function >> iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller > > Yeah, I can make that work. Hey Bart and Christoph, Bart, I noticed we basically had what you are requesting but Christoph had moved the id code from the fabric drivers to lio core in this commit: commit 2650d71e244fb3637b5f58a0080682a8bf9c7091 Author: Christoph Hellwig <hch@lst.de> Date: Fri May 1 17:47:58 2015 +0200 target: move transport ID handling to the core So what do you guys want to do here? Revert Christoph's patch and go back to that style or have lio core do the transport ID processing? > >> of that function is in source file scst/src/scst_targ.c: >> >> res = sess->tgt->tgtt->get_initiator_port_transport_id( >> sess->tgt, sess, &sess->transport_id); >> >> Thanks, >> >> Bart.-- >> To unsubscribe from this list: send the line "unsubscribe target-devel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- > To unsubscribe from this list: send the line "unsubscribe target-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 07/20/2018 04:08 PM, Mike Christie wrote: > On 07/19/2018 11:13 AM, Mike Christie wrote: >> > On 07/19/2018 10:15 AM, Bart Van Assche wrote: >>> >> On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: >>>> >>> On 07/18/2018 07:03 PM, Mike Christie wrote: >>>>> >>>> On 07/18/2018 05:09 PM, Bart Van Assche wrote: >>>>>> >>>>> [ ... ] >>>>>> >>>>> is that these involve a transport ID and that that transport ID can be up to 228 >>>>>> >>>>> bytes long for iSCSI. >>>>> >>>> >>>>> >>>> I am talking about the Initiator Session ID above. That along with the >>>>> >>>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout >>>>> >>>> table 508 or in SAM 5r21 checkout table A.4. >>>>> >>>> >>>>> >>>> So in the SCSI specs as part of the Initiator Port Transport ID we have >>>>> >>>> this from that SAM table: >>>>> >>>> >>>>> >>>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8 >>>>> >>>> encoded hexadecimal representation of a six byte binary value. >>>>> >>>> >>>>> >>>> --- >>>>> >>>> >>>>> >>>> In the PR parts of SPC it sometimes mentions only "Transport ID" but >>>>> >>>> then clarifies the initiator port so I am assuming in those cases it >>>>> >>>> means "Initiator Port Transport ID" so it is both the name and isid for >>>>> >>>> iscsi. >>>> >>> >>>> >>> It looks like we are supposed to go by what the initiator specifies in >>>> >>> the TPID field, so it can be either the Transport ID or Initiator Port >>>> >>> Transport ID. >>> >> >>> >> Hello Mike, >>> >> >>> >> Since the ISID is iSCSI-specific I think that all code that knows about the >>> >> ISID and its encoding should be in the iSCSI target driver instead of the >>> >> target core. Do you think an approach similar to that of the SCST function >>> >> iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller >> > >> > Yeah, I can make that work. > Hey Bart and Christoph, > > Bart, I noticed we basically had what you are requesting but Christoph > had moved the id code from the fabric drivers to lio core in this commit: > > commit 2650d71e244fb3637b5f58a0080682a8bf9c7091 > Author: Christoph Hellwig <hch@lst.de> > Date: Fri May 1 17:47:58 2015 +0200 > > target: move transport ID handling to the core > > > So what do you guys want to do here? Revert Christoph's patch and go > back to that style or have lio core do the transport ID processing? Oh yeah for the latter, we can make it so at least target_core_pr.c does not have to do any isid conversions. We could just rename sess_bin_isid to sess_str_isid, store the isid as a string, and then never convert it between the binary and string format. All the isid tests in target_core_pr.c would be strcmps.
On Fri, 2018-07-20 at 16:08 -0500, Mike Christie wrote: > Hey Bart and Christoph, > > Bart, I noticed we basically had what you are requesting but Christoph > had moved the id code from the fabric drivers to lio core in this commit: > > commit 2650d71e244fb3637b5f58a0080682a8bf9c7091 > Author: Christoph Hellwig <hch@lst.de> > Date: Fri May 1 17:47:58 2015 +0200 > > target: move transport ID handling to the core Hello Mike, I'm not in favor of reverting Christoph's patch because that patch simplified the target code significantly. On the other hand, it's inconvenient that with the current approach there is some code and knowledge in the target core that should be in target drivers. I think that the code for parsing the initiator name in srp_get_pr_transport_id() should be in the SRP target driver instead of the core. When I added support for a new initiator name format in the SRP target driver I overlooked that I had to update srp_get_pr_transport_id() because that function is in the core instead of ib_srpt.c. See also commit 2dc98f09f9e6 ("IB/srpt: Use the source GUID as session name"). Christoph, do you want me to add support for the new ib_srpt initiator name format in drivers/target/target_core_fabric_lib.c or should I find a way to move the code for parsing the initiator name into ib_srpt.c? Thanks, Bart.
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c index 01ac306..65e5253 100644 --- a/drivers/target/target_core_pr.c +++ b/drivers/target/target_core_pr.c @@ -662,8 +662,7 @@ static struct t10_pr_registration *__core_scsi3_do_alloc_registration( rcu_read_unlock(); pr_err("Unable to locate PR deve %s mapped_lun: %llu\n", nacl->initiatorname, mapped_lun); - kmem_cache_free(t10_pr_reg_cache, pr_reg); - return NULL; + goto free_reg; } kref_get(&pr_reg->pr_reg_deve->pr_kref); rcu_read_unlock(); @@ -679,12 +678,20 @@ static struct t10_pr_registration *__core_scsi3_do_alloc_registration( * save it to the registration now. */ if (isid != NULL) { - pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid); + if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) { + pr_err("Invalid isid %s\n", isid); + goto free_reg; + } + snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid); pr_reg->isid_present_at_reg = 1; } return pr_reg; + +free_reg: + kmem_cache_free(t10_pr_reg_cache, pr_reg); + return NULL; } static int core_scsi3_lunacl_depend_item(struct se_dev_entry *); @@ -873,7 +880,12 @@ int core_scsi3_alloc_aptpl_registration( * SCSI Initiator Port, restore it now. */ if (isid != NULL) { - pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid); + if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) { + pr_err("Invalid isid %s\n", isid); + kmem_cache_free(t10_pr_reg_cache, pr_reg); + return -EINVAL; + } + snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid); pr_reg->isid_present_at_reg = 1; } diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7261561..6324743 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -380,7 +380,8 @@ void __transport_register_session( memset(&buf[0], 0, PR_REG_ISID_LEN); se_tpg->se_tpg_tfo->sess_get_initiator_sid(se_sess, &buf[0], PR_REG_ISID_LEN); - se_sess->sess_bin_isid = get_unaligned_be64(&buf[0]); + + WARN_ON(hex2bin((u8 *)&se_sess->sess_bin_isid, buf, 6)); } spin_lock_irq(&se_nacl->nacl_sess_lock);
The isid is 48 bits, and in hex string format it's 12 bytes. We are currently copying the 12 byte hex string to a u64 so we can easily compare it, but this has the problem that only 8 bytes of the 12 bytes are copied. The next patches will want to print se_session sess_bin_isid so this has us use hex2bin to when converting from the hex sting to the bin value. Signed-off-by: Mike Christie <mchristi@redhat.com> --- drivers/target/target_core_pr.c | 20 ++++++++++++++++---- drivers/target/target_core_transport.c | 3 ++- 2 files changed, 18 insertions(+), 5 deletions(-)