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

[ronnie sahlberg]

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.

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.



regards
ronnie sahlberg
--
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