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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux