Re: [PATCH 04/22] be2iscsi: Fix negotiated parameters upload to FW

[Mike Christie <michaelc@cs.wisc.edu>]

On 09/18/2013 12:37 AM, Jayamohan Kallickal wrote:
> -----Original Message-----
> From: Mike Christie [mailto:michaelc@cs.wisc.edu] 
> Sent: Tuesday, September 17, 2013 2:16 PM
> To: Jayamohan Kallickal
> Cc: Jayamohan Kallickal; jbottomley@parallels.com; linux-scsi@vger.kernel.org; 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@cs.wisc.edu]
>> Sent: Monday, September 16, 2013 7:59 PM
>> To: Jayamohan Kallickal
>> Cc: jbottomley@parallels.com; linux-scsi@vger.kernel.org; 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@emulex.com>
>>> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
>>> ---
>>>  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.