[stgt] [PATCH] ISCSI: Honour MaxRecvDataSegmentLength for NORMAL sessions

[FUJITA Tomonori]

On Sat, 18 Aug 2012 18:56:09 +1000
ronnie sahlberg <ronniesahlberg at gmail.com> wrote:

> On Sat, Aug 18, 2012 at 6:32 PM, FUJITA Tomonori
> <fujita.tomonori at lab.ntt.co.jp> wrote:
>> On Sat, 18 Aug 2012 17:18:54 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
>>
>>> On Sat, 18 Aug 2012 18:13:05 +1000
>>> ronnie sahlberg <ronniesahlberg at gmail.com> wrote:
>>>
>>> > On Sat, Aug 18, 2012 at 6:06 PM, FUJITA Tomonori
>>> > <fujita.tomonori at lab.ntt.co.jp> wrote:
>>> >> On Sat, 18 Aug 2012 17:04:51 +0900 (JST)
>>> >> FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
>>> >>
>>> >>> On Sat, 18 Aug 2012 09:03:54 +1000
>>> >>> Ronnie Sahlberg <ronniesahlberg at gmail.com> wrote:
>>> >>>
>>> >>>> Fix a bug in iscsid.c with regards to MaxRecvDataSegmentLength.
>>> >>>> The parsing of this login key only updated the settings for how large PDUs we can send for DISCOVERY sessions.
>>> >>>
>>> >>> Really? I thought that we call param_set_val() for
>>> >>> ISCSI_PARAM_MAX_XMIT_DLENGTH in SESSION_NORMAL. If you configure
>>> >>> ISCSI_PARAM_MAX_XMIT_DLENGTH, tgtd honors MaxRecvDataSegmentLength
>>> >>> that an initiator sends?
>>> >>>
>>> >>> tgtadm --op show --mode target --tid 1 MaxRecvDataSegmentLength=8192
>>> >>
>>> >> Oops, this should be something like:
>>> >>
>>> >> tgtadm --op update --mode target --tid 1 --name MaxXmitDataSegmentLength --value 16384
>>> >
>>> > Why set it manually from the tgtadm command line?
>>> > The initiator already told tgtd what the size is, so only thing
>>> > required is to fix the bug to make the target honour what the
>>> > initiator aleady sent to the target.
>>>
>>> Yeah, but tgtd still needs to check if the value that an initiator
>>> told is acceptable from the iscsi spec (512 - 2^24 -1).
>>>
>>> I agree that we had better to improve the current code. Setting
>>> MaxXmitDataSegmentLength by hand is not a good idea.
>>
>> The following patch might work. I'm not sure this patch works if an
>> initiator doesn't send MaxRecvDataSegmentLength. IIRC, open-iscsi
>> always send.
>>
>>
>> diff --git a/usr/iscsi/target.c b/usr/iscsi/target.c
>> index 3a91404..aad1556 100644
>> --- a/usr/iscsi/target.c
>> +++ b/usr/iscsi/target.c
>> @@ -433,7 +433,7 @@ int iscsi_target_create(struct target *t)
>>         struct iscsi_target *target;
>>         struct param default_tgt_session_param[] = {
>>                 [ISCSI_PARAM_MAX_RECV_DLENGTH] = {0, 8192},
>> -               [ISCSI_PARAM_MAX_XMIT_DLENGTH] = {0, 8192},  /* do not edit */
>> +               [ISCSI_PARAM_MAX_XMIT_DLENGTH] = {0, 262144},  /* do not edit */
>>                 [ISCSI_PARAM_HDRDGST_EN] = {0, DIGEST_NONE},
>>                 [ISCSI_PARAM_DATADGST_EN] = {0, DIGEST_NONE},
>>                 [ISCSI_PARAM_INITIAL_R2T_EN] = {0, 1},
> 
> I dont think this is right.
> The default is 8kb, and as long as the initiator  has not said
> otherwise, the target should NOT send more than 8kb in a DATA-IN.
> That part is right in tgtd.

I thought that the part is handled in text_check_param()? Have you
tried the patch? I might be wrong. I can't recall much about tgt
parameter negotiation code.

> The bug is that IF the initiator does state it can handle larger
> DATA-IN PDUs than 8kb,   like open-iscsi does, and all other iniators
> also does.
> The b\ug in tgtd in processing the login is that tgtd ONLY parses this
> "lets use bigger than 8kb pdu" IFF the session is a discovery session,
> but not for NORMAL sessions.
> For all normal sessions tgtd will be clamped down to maximum 8kb  per
> data-in pdu due to this bug.
> 
> 
> Please compare open-iscsi to  lio vs open-iscsi vs stgt.
> With stgt any large read will be split in a train of 8kb data-in pdus.
> 
> 
> A network trace comparing open-iscsi reading from lio vs stgt will
> make the problem easy to spot. This hurts if the initiator/application
> tried to read >8k at a time.

I happy to apply a patch that fixes the above problem and still checks
if the value that an initiator old is acceptable.

--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html