On 09/18/2013 12:37 AM, Jayamohan Kallickal wrote: > -----Original Message----- > From: Mike Christie [mailto:[email protected]] > Sent: Tuesday, September 17, 2013 2:16 PM > To: Jayamohan Kallickal > Cc: Jayamohan Kallickal; [email protected]; > [email protected]; Sony John-N > Subject: Re: [PATCH 04/22] be2iscsi: Fix negotiated parameters upload to FW > > On 09/16/2013 11:28 PM, Jayamohan Kallickal wrote: >> >> >> -----Original Message----- >> From: Mike Christie [mailto:[email protected]] >> Sent: Monday, September 16, 2013 7:59 PM >> To: Jayamohan Kallickal >> Cc: [email protected]; [email protected]; Jayamohan >> Kallickal; Sony John-N >> Subject: Re: [PATCH 04/22] be2iscsi: Fix negotiated parameters upload >> to FW >> >> On 09/13/2013 12:09 AM, Jayamohan Kallickal wrote: >>> - If target does not send MaxRecvDSL in login repsonse, then >>> initiator should consider the MaxRecvDSL for target is 8K. >>> In this scenario driver was setting the value to 64K and this >>> caused target to close cxn as data xfer was more than the >>> MaxRecvDSL >>> - Update connection offload data structure for SKH-R adapters. >>> >>> Signed-off-by: John Soni Jose <[email protected]> >>> Signed-off-by: Jayamohan Kallickal <[email protected]> >>> --- >>> drivers/scsi/be2iscsi/be_iscsi.c | 9 +++++++-- >>> drivers/scsi/be2iscsi/be_main.h | 29 ++++++++++++++++------------- >>> drivers/scsi/be2iscsi/be_mgmt.c | 8 +++----- >>> 3 files changed, 26 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/scsi/be2iscsi/be_iscsi.c >>> b/drivers/scsi/be2iscsi/be_iscsi.c >>> index 2496ea7..60c1dff 100644 >>> --- a/drivers/scsi/be2iscsi/be_iscsi.c >>> +++ b/drivers/scsi/be2iscsi/be_iscsi.c >>> @@ -672,9 +672,10 @@ int beiscsi_set_param(struct iscsi_cls_conn *cls_conn, >>> session->max_burst = 262144; >>> break; >>> case ISCSI_PARAM_MAX_XMIT_DLENGTH: >>> - if ((conn->max_xmit_dlength > 65536) || >>> - (conn->max_xmit_dlength == 0)) >>> + if (conn->max_xmit_dlength > 65536) >>> conn->max_xmit_dlength = 65536; >>> + else if (conn->max_xmit_dlength == 0) >>> + conn->max_xmit_dlength = 8192; >> >>> Was the target sending 0 or not sending anything at all? Userspace should >>> not be sending 0 if the target did not send >MaxRecvDSL. It looks like it >>> should be sending 8k for that case. It looks like there is a bug in the >>> tools where it will pass 0 >if the target sent 0. >> >>> It seems other drivers would be hitting this bug too and we should fix >>> everyone. >> >> This was an IET target that did not send any value and we were >> defaulting to 64K >> >> Here is a small userspace fix that should go along >> >> diff --git a/usr/be2iscsi.c b/usr/be2iscsi.c index ce8b719..ba4c29f >> 100644 >> --- a/usr/be2iscsi.c >> +++ b/usr/be2iscsi.c >> @@ -33,10 +33,6 @@ void be2iscsi_create_conn(struct iscsi_conn *conn) >> if (conn->max_xmit_dlength > 65536) >> conn->max_xmit_dlength = 65536; >> >> - if (!conn_rec->iscsi.MaxXmitDataSegmentLength || >> - conn_rec->iscsi.MaxXmitDataSegmentLength > 65536) >> - conn_rec->iscsi.MaxXmitDataSegmentLength = 65536; >> - >> session->erl = 0; >> session->initial_r2t_en = 1; >> } > >> For the case you are trying to fix are you getting 0 or 64K from userspace? >> In the kernel code you changed to set the <value to 8K it looks like you got >> 0 from userspace. Is that right? > > Before this fix, in the usespace we will hit > (!conn_rec->iscsi.MaxXmitDataSegmentLength) and will set > conn_rec->iscsi.MaxXmitDataSegmentLength = 65536. The userspace patch would > prevent that from happening. > > So, in the Kernel space we would get 0 and which would be set to 8K. > >> Are you setting node.conn[0].iscsi.MaxXmitDataSegmentLength in iscsif.conf >> to 64K or did you leave it as the default? >Just to make sure we are on the >> same page I really mean did you set it in iscsid.conf or with iscsiadm. I am >> not talking >about setting above in the be2iscsi create_conn callout. > > No changes were made. It was default values. > >> If it is the default of 0, then I think when the code above is called >> iscsi_copy_operational_params will have set conn->>max_xmit_dlength to >> ISCSI_DEF_MAX_RECV_SEG_LEN (8k). The conn->max_xmit_dlength value is the one >> we use for >final negotiated value so that is what gets passed to the kernel. > >> If the target does not negotiate for MRDSL then it should stay 8k. >> iscsi_session_set_params will then pass down conn->max_xmit_dlength when >> login is done. > >> If I am looking at the code right, the only way we can get 0 in the kernel >> is if the target sends 0 for MRDSL. iscsid was not >expecting that and will >> just set conn->max_xmit_dlength to 0 and that will get passed down to all >> drivers incorrectly. If I >am right then we need to do a fix for all drivers. > > As an experiment, we tried applying just the userspace patch I sent earlier > and we got the max_xmit_dlength as 8K. > So, the basic issue was that the user space code in usr/be2iscsi was setting > the conn->max_xmit_dlength to 64K when we hit > (!conn_rec->iscsi.MaxXmitDataSegmentLength) >
Ok. It now makes sense. > I will send out a separate patch for the userspace code, but feel we can > still keep the driver changes since the current code in Kernel sets > conn->max_xmit_dlength=64K if it cam down to driver as zero. I would like to > keep the check in driver as the user may apply different version of the > userspace code (Distro version, Top of your Tree). > It is not a question of it is needed. It is more where the zero check should go. I was saying that no driver can support a zero max_xmit_dlength and we should have some code in scsi_transport_iscsi:iscsi_set_param() to make sure all iscsi rfc settings are within the RFC defined limits. The driver set_param would then only have to check if the value passed in is within its fw/driver limits. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
