Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Sagi Grimberg <sagig@dev.mellanox.co.il>]

On 15/11/2015 23:10, Or Gerlitz wrote:
> On Sun, Nov 15, 2015, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>> On 15/11/2015 19:59, Christoph Hellwig wrote:
>
>>> Without this sg_dma_len will return 0 on architectures tha have
>>> the dma_length field.
>
> and what wrong with that?

Because it's not length 0. It's because I did a (documented) hack
with the new memory registration API. I hope we can fix it soon.

>
> Christoph, probably typo here? "tha" needs to be "that"
>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>    drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>>> b/drivers/infiniband/ulp/srp/ib_srp.c
>>> index 32f7962..445c0a6 100644
>>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>>> @@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch,
>>> struct srp_request *req,
>>>                  state.sg_nents = 1;
>>>                  sg_set_buf(idb_sg, req->indirect_desc, idb_len);
>>>                  idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
>>> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
>>> +               idb_sg->dma_length = idb_sg->length;          /* hack^2 */
>>> +#endif
>>
>>
>> :)
>>
>> We should really get this properly map/unmap per IO at some point.
>> Probably do it in both code paths...
>
> Sagi, can you please elaborate a little further on the problem, srpt
> WA,

It's srp initiator. This falls in the case where srp sends an indirect
buffer descriptor to the target (which is registered). For this
descriptor it uses a pre-dma-mapped buffer, so in order to fit it into
the new memory registration scheme (which works on SG lists), I hacked
a single entry SG and used the pre-dma-mapped address. What was missing
is the dma_length setting (because we didn't dma mapped the SG).

The proper fix is doing correct map/unmap per IO.

> what do we do in isert and what is the proposed not WA solution?

iser does not have indirect buffer descriptors, only a single rkey
(per-direction).