diff mbox

[Query] iSER-Target: QP errors observed on increasing MaxXmitDataSegmentLength to 16384 (default = 8192)

Message ID 1488449150.21712.129.camel@haakon3.risingtidesystems.com (mailing list archive)
State Superseded
Headers show

Commit Message

Nicholas A. Bellinger March 2, 2017, 10:05 a.m. UTC
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..?

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.

If this is the case, then following (untested) patch should do the
trick.

Comments

Potnuri Bharat Teja March 2, 2017, 1:09 p.m. UTC | #1
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
Sagi Grimberg March 6, 2017, 3:20 p.m. UTC | #2
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 mbox

Patch

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)) {