Re: [PATCH 10/12] IB/srpt: convert to the generic RDMA READ/WRITE API

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On 04/14/2016 09:32 AM, Christoph Hellwig wrote:
> On Wed, Apr 13, 2016 at 11:57:57AM -0700, Bart Van Assche wrote:
>> On 04/11/2016 02:32 PM, Christoph Hellwig wrote:
>>>   static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>>> -			     struct srp_cmd *srp_cmd,
>>> -			     enum dma_data_direction *dir, u64 *data_len)
>>> +		struct srp_cmd *srp_cmd, enum dma_data_direction *dir,
>>> +		struct scatterlist **sg, unsigned *sg_cnt, u64 *data_len)
>>>   {
>> [ ... ]
>>>
>>> -		db = idb->desc_list;
>>> -		memcpy(ioctx->rbufs, db, ioctx->n_rbuf * sizeof(*db));
>>>   		*data_len = be32_to_cpu(idb->len);
>>> +		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
>>> +				sg, sg_cnt);
>>> +	} else {
>>> +		*data_len = 0;
>>> +		return 0;
>>>   	}
>>> -out:
>>> -	return ret;
>>>   }
>>
>> srpt_get_desc_tbl() only has one caller. Have you considered to move 
>> srpt_alloc_rw_ctxs() from this function to the caller of 
>> srpt_get_desc_tbl()?
> 
> I looked into a couple options.  srpt_alloc_rw_ctxs needs the
> pointer to the srp_direct_buf array, and the number of buffers, so we'd
> need two more output arguments to srpt_get_desc_tbl, so it didn't
> seem worthwhile to me.  If you want me to make the change anyway, I can
> update the patch.
> 

Hi Christoph,

I see you responded to Bart's comment above, but in the same email he
had a second comment on this patch (that the logic was incorrect in part
of it), and I've not seen a response to that.  Here's the comment I'm
referring to:

>> -    if (srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &data_len)) {
>> -        pr_err("0x%llx: parsing SRP descriptor table failed.\n",
>> -               srp_cmd->tag);
>> +    rc = srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &sg, &sg_cnt,
>> +            &data_len);
>> +    if (rc) {
>> +        if (rc != -EAGAIN) {
>> +            pr_err("0x%llx: parsing SRP descriptor table failed.\n",
>> +                   srp_cmd->tag);
>> +        } else {
>> +            printk_ratelimited("out of MRs for 0x%llx\n", srp_cmd->tag);
>> +        }
>>           goto release_ioctx;
>>       }
> 
> Sorry but releasing an I/O context if srpt_alloc_rw_ctxs() returns
> -EAGAIN looks wrong to me. If this happens the I/O context should be
> added to the wait list without releasing it. Additionally,
> srpt_recv_done() will have to be modified such that newly received
> commands are added to the wait list if this list is not empty to
> prevent that starvation of a postponed request occurs due to new
> incoming requests.



-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]