| Message ID | 1488449150.21712.129.camel@haakon3.risingtidesystems.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thursday, March 03/02/17, 2017 at 15:35:50 +0530, Nicholas A. Bellinger wrote: > On Thu, 2017-03-02 at 14:31 +0530, Potnuri Bharat Teja wrote: > > On Thursday, March 03/02/17, 2017 at 14:00:33 +0530, Nicholas A. > Bellinger wrote: > > > Hi Bharat & Co, > > > > > > Adding Jenny + Or CC', as I believe they are still the main point > people > > > for iser-target related items at Mellanox. > > > > > > On Fri, 2017-02-24 at 14:44 +0530, Potnuri Bharat Teja wrote: > > > > Hi Sagi/Nicholas, > > > > > > > > When tried changing the MaxXmitDataSegmentLength to 16384 > (default = > > > > 8192), by changing it from targetcli on target and iscsd.conf on > > > > initiator, I observe the following errors. > > > > > > > > cxgb4 0000:06:00.4: AE qpid 1024 opcode 3 status 0x6 type 0 len > 0x5c > > > wrid.hi 0x0 wrid.lo 0x136 > > > > isert: isert_qp_event_callback: QP access error (3): conn > > > ffff8807da7b6000 > > > > Aligning ISER MaxRecvDataSegmentLength: 4096 down to PAGE_SIZE > > > > > > IIRC, this message in > iscsi_target_login.c:iscsi_login_zero_tsih_s2() > > > indicates the initiator presented MRDSL was either not PAGE_SIZED > > > aligned, or less than PAGE_SIZE.. > > > > > > Not sure why this is happening with a MRDSL=16384..? Can you > confirm > > > what MRDSL came across the wire..? > > Hi Nicholas, > > Thanks for the reply! > > I saw the same MRDSL=16384 over the wire. > > > > > > This would explain the iw_cxgb4 errors about the receive buffers > posted > > > by target are insufficient for incoming data. > > > > > > > cxgb4 0000:06:00.4: AE qpid 1026 opcode 3 status 0x6 type 0 len > 0x5c > > > wrid.hi 0x0 wrid.lo 0x2 > > > > isert: isert_qp_event_callback: QP access error (3): conn > > > ffff88053a2ee000 > > > > > > > > From the error status of iw_cxgb4 the receive buffers posted by > target > > > > are unsufficient for the > > > > incoming data to be placed/DMAed by the HW/adapter. > > > > Apparently, from the iSER-target code the rx buffers are > acclocated > > > > for a fixed size of 8192. from isert_alloc_rx_descriptors() in > > > > drivers/infiniband/ulp/isert/ib_isert.c > > > > rx_sg->length = ISER_RX_PAYLOAD_SIZE; > > > > > > > > I confirmed the same by increasing the ISER_RX_PAYLOAD_SIZE to > 16384 > > > > and the errors arent seen. > > > > > > > > > > Mmmm. > > > > > > > As far as i could see, from the iSER target code, > > > > MaxXmitDataSegmentLength should not be changed according to the > > > > targetcli/openiscsi parameters and should countinue based on iSER > > > > specific Initiator/targetrecvdatasegmentlength and so does the > > > > MaxrecvDatasegmentLength. > > > > > > > > Please let me know if my observations are right and what could be > done > > > > to fix this. > > > > > > > > > > OK, it sounds like a reasonable approach to always enforce the > default > > > MXDSL=8192 from the target side. > > > > > > The issue of ignoring what the initiator sent for MRDSL, and always > > > enforcing MRDSL=8192 would be OK if the initiator is presenting a > MRDSL > > > larger than 8192, but could be problematic if it is presenting a > MRDSL > > > smaller than 8192. > > > > > > So I think the right approach would be: > > > > > > - Always use MXDSL = 8192 in iser-target, regardless of what's > > > configured by targetcli via configfs, et al. > > > - Always honor MRDSL = 8192 in iser-target, when the initiator > presents > > > a MRDSL >= 8192 during login. > > > - If the initiator is attempting to present a MRDSL < 8192, fail > > > the login with ISCSI_LOGIN_STATUS_INIT_ERR > > Agreed! > > After reading your email again, I may have mistaken one important bit.. > > If the initiator is proposing MRDSL != 8192, are there still problems > with iser-initiator hard-coded limits vs. what iser-target is actually > posting for outgoing non RDMA_WRITE transfers of != 8192..? > > Or rather, does initiator proposed MRDSL != 8192 work as expected with > existing iser-initiator + iser-target code..? > Here are my Initiator MRDSL and MXDSL node.conn[0].iscsi.MaxRecvDataSegmentLength = 8192 node.conn[0].iscsi.MaxXmitDataSegmentLength = 16384 and the Target MRDSL and MXDSL: /> iscsi/iqn.2016-17.org1/tpg1/ get parameter MaxXmitDataSegmentLength MaxXmitDataSegmentLength=16384 /> iscsi/iqn.2016-17.org1/tpg1/ get parameter MaxRecvDataSegmentLength MaxRecvDataSegmentLength=8192 To my knowledge, MaxXmitDataSegmentLength should be negotiated to min(8192, 16384). But the session info shows 16384: #iscsiadm -m session -P3 - ************************ Negotiated iSCSI params: ************************ HeaderDigest: None DataDigest: None MaxRecvDataSegmentLength: 128 MaxXmitDataSegmentLength: 16384 FirstBurstLength: 65536 MaxBurstLength: 262144 ImmediateData: Yes InitialR2T: Yes MaxOutstandingR2T: 1 - with default LIO and openiscsi configuration I see: MaxXmitDataSegmentLength: 8192 > If this is true and it's only a iser-target + MXDSL specific bug vs. > hard-coded ISER_RX_PAYLOAD_SIZE definitions mentioned above, then we can > avoid messing with the initiator proposed MRDSL. > From the set-up perspective, I was able to alter the negotiated MaxXmitDataSegmentLength to 16384 only if I set the MaxXmitDataSegmentLength to 16384 on Target. So, There is high chance the target makeing wrong calculation > If this is the case, then following (untested) patch should do the > trick. Below Patch isnt holding good, I see the issue still and MXDSL set to 16384. # iscsiadm -m session -P3 Target: iqn.2016-17.chelsio.org8 (non-flash) Current Portal: 10.0.2.2:3260,1 Persistent Portal: 10.0.2.2:3260,1 - ************************ Negotiated iSCSI params: ************************ HeaderDigest: None DataDigest: None MaxRecvDataSegmentLength: 128 MaxXmitDataSegmentLength: 16384 FirstBurstLength: 65536 MaxBurstLength: 262144 ImmediateData: Yes InitialR2T: Yes MaxOutstandingR2T: 1 ************************ Attached SCSI devices: ************************ Host Number: 13 State: running scsi13 Channel 00 Id 0 Lun: 0 Attached scsi disk sdj State: running - Shall debug further and let you know. Thanks! > > diff --git a/drivers/target/iscsi/iscsi_target_login.c > b/drivers/target/iscsi/iscsi_target_login.c > index 303cb65..db811a6 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -427,49 +427,31 @@ static int iscsi_login_zero_tsih_s2( > * Set RDMAExtensions=Yes by default for iSER enabled network > portals > */ > if (iser) { > - struct iscsi_param *param; > - unsigned long mrdsl, off; > + unsigned long mxdsl; > int rc; > > if (iscsi_change_param_sprintf(conn, > "RDMAExtensions=Yes")) > return -1; > - > /* > - * Make MaxRecvDataSegmentLength PAGE_SIZE aligned for > - * Immediate Data + Unsolicited Data-OUT if necessary.. > + * Due to the hardcoded assumptions about MXDSL for > iser-target > + * in ISER_RX_PAYLOAD_SIZE, we must always enforce the > default > + * INITIAL_TARGETRECVDATASEGMENTLENGTH = 8192, regardless > of > + * what this value may have been set to by user via > configfs > + * for traditional iscsi-target operation. > */ > - param = > iscsi_find_param_from_key("MaxRecvDataSegmentLength", > - conn->param_list); > - if (!param) { > - iscsit_tx_login_rsp(conn, > ISCSI_STATUS_CLS_TARGET_ERR, > - ISCSI_LOGIN_STATUS_NO_RESOURCES); > - return -1; > - } > - rc = kstrtoul(param->value, 0, &mrdsl); > + rc = kstrtoul(INITIAL_TARGETRECVDATASEGMENTLENGTH, 0, > &mxdsl); > if (rc < 0) { > iscsit_tx_login_rsp(conn, > ISCSI_STATUS_CLS_TARGET_ERR, > ISCSI_LOGIN_STATUS_NO_RESOURCES); > return -1; > } > - off = mrdsl % PAGE_SIZE; > - if (!off) > - goto check_prot; > - > - if (mrdsl < PAGE_SIZE) > - mrdsl = PAGE_SIZE; > - else > - mrdsl -= off; > - > - pr_warn("Aligning ISER MaxRecvDataSegmentLength: %lu down" > - " to PAGE_SIZE\n", mrdsl); > - > - if (iscsi_change_param_sprintf(conn, > "MaxRecvDataSegmentLength=%lu\n", mrdsl)) > + if (iscsi_change_param_sprintf(conn, > "MaxXmitDataSegmentLength=%lu\n", mxdsl)) > return -1; > + > /* > * ISER currently requires that ImmediateData + > Unsolicited > * Data be disabled when protection / signature MRs are > enabled. > */ > -check_prot: > if (sess->se_sess->sup_prot_ops & > (TARGET_PROT_DOUT_STRIP | TARGET_PROT_DOUT_PASS | > TARGET_PROT_DOUT_INSERT)) { > -- > 1.9.1 -- 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
Hey Nic, > After reading your email again, I may have mistaken one important bit.. > > If the initiator is proposing MRDSL != 8192, are there still problems > with iser-initiator hard-coded limits vs. what iser-target is actually > posting for outgoing non RDMA_WRITE transfers of != 8192..? > > Or rather, does initiator proposed MRDSL != 8192 work as expected with > existing iser-initiator + iser-target code..? > > If this is true and it's only a iser-target + MXDSL specific bug vs. > hard-coded ISER_RX_PAYLOAD_SIZE definitions mentioned above, then we can > avoid messing with the initiator proposed MRDSL. Reading this thread and revisiting the code it looks like both the target and the initiator and not broken at the same time. Starting from 7145: 6.2. MaxRecvDataSegmentLength For an iSCSI connection belonging to a session in which RDMAExtensions=Yes was negotiated on the leading connection of the session, MaxRecvDataSegmentLength need not be declared in the Login Phase, and MUST be ignored if it is declared. Instead, InitiatorRecvDataSegmentLength (as described in Section 6.5) and TargetRecvDataSegmentLength (as described in Section 6.4) keys are negotiated. The values of the local and remote MaxRecvDataSegmentLength are derived from the InitiatorRecvDataSegmentLength and TargetRecvDataSegmentLength keys. In the Full Feature Phase, the initiator MUST consider the value of its local MaxRecvDataSegmentLength (that it would have declared to the target) as having the value of InitiatorRecvDataSegmentLength, and the value of the remote MaxRecvDataSegmentLength (that would have been declared by the target) as having the value of TargetRecvDataSegmentLength. Similarly, the target MUST consider the value of its local MaxRecvDataSegmentLength (that it would have declared to the initiator) as having the value of TargetRecvDataSegmentLength, and the value of the remote MaxRecvDataSegmentLength (that would have been declared by the initiator) as having the value of InitiatorRecvDataSegmentLength. So both the target and the initiator ignores MaxRecvDataSegmentLength value :) And: 6.4. TargetRecvDataSegmentLength Use: IO (Initialize only) Senders: Initiator and Target Scope: CO (connection-only) Irrelevant when: RDMAExtensions=No TargetRecvDataSegmentLength=<numerical-value-512-to-(2**24-1)> Default is 8192 bytes Result function is minimum 6.5. InitiatorRecvDataSegmentLength Use: IO (Initialize only) Senders: Initiator and Target Scope: CO (connection-only) Irrelevant when: RDMAExtensions=No InitiatorRecvDataSegmentLength=<numerical-value-512-to-(2**24-1)> Default is 8192 bytes On the other hand, not only they never negotiate InitiatorRecvDataSegmentLength, TargetMaxRecvDataSegmentLength but its not exposed at all. At least the default are correct :) I think that iscsi_target_parameters.c almost got it right, it needs to respond with TargetMaxRecvDataSegmentLength in response to InitiatorRecvDataSegmentLength and both should respect it. > > If this is the case, then following (untested) patch should do the > trick. I think we are better off by doing this the right way. Have open-iscsi negotiate InitiatorMaxRecvDataSegmentLength and have the target reply TargetMaxRecvDataSegmentLength and have both sides honor the replies. Would be nice to expose user config knob for it. -- 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
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 303cb65..db811a6 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -427,49 +427,31 @@ static int iscsi_login_zero_tsih_s2( * Set RDMAExtensions=Yes by default for iSER enabled network portals */ if (iser) { - struct iscsi_param *param; - unsigned long mrdsl, off; + unsigned long mxdsl; int rc; if (iscsi_change_param_sprintf(conn, "RDMAExtensions=Yes")) return -1; - /* - * Make MaxRecvDataSegmentLength PAGE_SIZE aligned for - * Immediate Data + Unsolicited Data-OUT if necessary.. + * Due to the hardcoded assumptions about MXDSL for iser-target + * in ISER_RX_PAYLOAD_SIZE, we must always enforce the default + * INITIAL_TARGETRECVDATASEGMENTLENGTH = 8192, regardless of + * what this value may have been set to by user via configfs + * for traditional iscsi-target operation. */ - param = iscsi_find_param_from_key("MaxRecvDataSegmentLength", - conn->param_list); - if (!param) { - iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, - ISCSI_LOGIN_STATUS_NO_RESOURCES); - return -1; - } - rc = kstrtoul(param->value, 0, &mrdsl); + rc = kstrtoul(INITIAL_TARGETRECVDATASEGMENTLENGTH, 0, &mxdsl); if (rc < 0) { iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, ISCSI_LOGIN_STATUS_NO_RESOURCES); return -1; } - off = mrdsl % PAGE_SIZE; - if (!off) - goto check_prot; - - if (mrdsl < PAGE_SIZE) - mrdsl = PAGE_SIZE; - else - mrdsl -= off; - - pr_warn("Aligning ISER MaxRecvDataSegmentLength: %lu down" - " to PAGE_SIZE\n", mrdsl); - - if (iscsi_change_param_sprintf(conn, "MaxRecvDataSegmentLength=%lu\n", mrdsl)) + if (iscsi_change_param_sprintf(conn, "MaxXmitDataSegmentLength=%lu\n", mxdsl)) return -1; + /* * ISER currently requires that ImmediateData + Unsolicited * Data be disabled when protection / signature MRs are enabled. */ -check_prot: if (sess->se_sess->sup_prot_ops & (TARGET_PROT_DOUT_STRIP | TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_INSERT)) {