On 09/18/2013 12:37 AM, Jayamohan Kallickal wrote: > -----Original Message----- > From: Mike Christie [mailto:michaelc@xxxxxxxxxxx] > Sent: Tuesday, September 17, 2013 2:16 PM > To: Jayamohan Kallickal > Cc: Jayamohan Kallickal; jbottomley@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; 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:michaelc@xxxxxxxxxxx] >> Sent: Monday, September 16, 2013 7:59 PM >> To: Jayamohan Kallickal >> Cc: jbottomley@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; 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 <sony.john-n@xxxxxxxxxx> >>> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@xxxxxxxxxx> >>> --- >>> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
