Re: [PATCH] iser-target: Handle errors from isert_put_datain and isert_get_dataout

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

On 3/7/2015 9:19 AM, Nicholas A. Bellinger wrote:
> On Sat, 2015-03-07 at 04:16 +0200, Sagi Grimberg wrote:
>> On 3/6/2015 7:56 PM, Chris Moore wrote:
>>> isert_put_datain() always returns 1 and isert_get_dataout() always returns 0, even if
>>> ib_post_send() fails.   They should return an error in this case so the caller can handle it.
>>> Also, in the case of an ib_post_send() failure, user isert_err instead of isert_warn.
>>>
>>> With these changes, these two functions handle errors from ib_post_send() in the
>>> same way as other functions within ib_isert.c
>>>
>>
>> Hi Chris,
>>
>> This is indeed needed, but I'm afraid this is not complete given the
>> rc is completely ignored by the callers (see
>> lio_queue_data_in/lio_write_pending).
>>
>
> So lio_write_pending() is propagating up the return back to
> transport_generic_new_cmd().  When the return is -EAGAIN or -ENOMEM,
> it triggers transport_handle_queue_full() to retry ->write_pending()
> from se_device->qf_work_queue context.

Ah, Right...

>
> It's lio_queue_data_in() + lio_queue_status() that aren't propagating up
> failures to trigger queue_full in target_complete_ok_work().  Looking at
> this code again for traditional iscsi-target, I don't see a reason why
> iscsit_add_cmd_to_response_queue() failure should not be triggering
> queue_full logic to kick in..
>
> On the iser-target side, is it OK for isert_put_datain() +
> isert_put_response() to be re-invoked from transport_complete_qf()
> context after ib_post_send() failure..?

Well, Generally the QP owner is obligated to not post more than the
QP size and/or request for send completion once every SQ size. If we got
ENOMEM from ib_post_send this usually indicates a bug, and there is no
sense in retrying later, and I'm not aware of any provider that may
return EAGAIN at the moment, but maybe this can happen theoretically...

But I think the correct behavior from iSCSI PoV is to have
ENOMEM/EAGAIN error codes from queue_data_in/queue_status trigger
queue_full logic and terminate the session for any other
(non-transient) error code.

Sagi.