[stgt] [PATCH 14/15] Fix iSCSI Data-Out solicitation

[Pete Wyckoff]

arne.redlich at googlemail.com wrote on Tue, 16 Jun 2009 16:45 +0200:
> Am Dienstag, den 16.06.2009, 06:24 -0400 schrieb Pete Wyckoff:
> > arne.redlich at googlemail.com wrote on Tue, 09 Jun 2009 18:22 +0200:
> > > Currently the iSCSI driver only solicits MaxXmitDSL(!) bytes with each R2T,
> > > although it could solicit MaxBurstLength bytes.
> > 
> > This fix makes sense.  R2T should ask for a whole burst.
> > 
> > > This was introduced with the iSER driver and a RDMA_TRANSFER_SIZE constant to
> > > limit the size of RDMA transfers. However, MaxBurstLength can and should be
> > > used for that.
> > 
> > But RDMA should not necessarily use MAX_BURST for its data transfer
> > sizes.  This size is completely controlled by the target and not
> > even visible to the initiator.  The initiator should not be involved
> > in the negotiation of the size.
> 
> I don't agree. MaxBurstLength is the upper limit for the amount of data
> an R2T can solicit, so it's also the upper limit for the RDMA read the
> R2T is translated to. You can certainly request smaller sizes, but
> soliciting / RDMA reading data sizes > MaxBurstLength is a violation of
> the iSCSI spec.

Please search the old stgt-devel list at berlios for

    Message-ID: <5eb093080708120312i1287f1f7p2fcaed789bf70cc7 at mail.gmail.com>
    Date: Sun, 12 Aug 2007 12:12:32 +0200
    From: Alexander Nezhinsky

In which he says, in part:

> iSER spec is silent about the granularity of RDMA transfers
> because it says nothing about the meaning of MaxBurstLength and
> MaxRecvDataSegmentLength, when applied to the solicited data of
> a write op, and to the entire data of a read op.
> 
> On the other hand, it maps R2T PDUs to RDMA Reads,
> and Data-IN to RDMA Writes (changing their meaning), but does not
> require that their sizes must be governed by either
> MaxBurstLength (for R2Ts) or MaxRecvDataSegmentLength
> (for Data-INs).
> 
> Thus we can interpret them freely, from the formal point of view.
> Moreover, this does not contradict the spirit of the protocol,
> which makes all RDMA transfers a target's responsibility.

I found that a convincing argument and implemented accordingly.

Your interpretation makes some amount of conceptual sense, but is
not really correct.  The initiator truly has no interest in the size
of the RDMA transfers and should not be involved in negotiating an
upper limit on their lengths.

Hope you will find acceptable my earlier suggestion of a minimal
patch to get non-RDMA r2t working better (below).  Especially since
neither you nor I can test it.

> > There was a longish discussion of this value in the archive
> > somewhere around the time iser was getting added to tgt.
> > 
> > Unfortunately I don't have gear to test this anymore.  One risk is
> > that iser initiators will negotiate a small MaxBurstLength and get
> > bad performance.
> 
> See above - if this is negotiated it has to be used.
> 
> >   Another is they negotiate something bigger than
> > the mempool_size constant and break the iser code, if someone tries
> > to set target's MaxBurstLength bigger hoping for faster transfers.
> 
> I agree.
> 
> > If you want to be safe, you can leave data_inout_max_length alone.
> > Just change iscsi_r2t_build to "if (rdma)" to select max_burst or
> > data_in_out_len.
> > 
> > If I know Fujita code style, he won't like the "if (rdma)" you added
> > here or the one I'm suggesting.  In which case you might add a
> > second conn parameter, data_r2t_max_length, that gets set appropriately.
> > And rename data_inout_max_length to remove the "in" part now that
> > you've figured out the R2T situation.
> 
> Hmm, it would actually seem cleanest to me if the iSER driver would just
> limit MaxBurstLength (and other parameters if required) before login
> negotiation - iser_accept_connection() looks like the right place for
> that.

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