On Sat, 18 Aug 2012 18:56:09 +1000
ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
> On Sat, Aug 18, 2012 at 6:32 PM, FUJITA Tomonori
> <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>> On Sat, 18 Aug 2012 17:18:54 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>>
>>> On Sat, 18 Aug 2012 18:13:05 +1000
>>> ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
>>>
>>> > On Sat, Aug 18, 2012 at 6:06 PM, FUJITA Tomonori
>>> > <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>>> >> On Sat, 18 Aug 2012 17:04:51 +0900 (JST)
>>> >> FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>>> >>
>>> >>> On Sat, 18 Aug 2012 09:03:54 +1000
>>> >>> Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
