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@xxxxxxxxxx>
GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
