Re: [PATCH for-6.0 0/5] IORING_OP_SEND_ZC improvements

[Stefan Metzmacher <metze@samba.org>]

Hi Pavel,

>>> If network sends anything it should return how many bytes
>>> it queued for sending, otherwise there would be duplicated
>>> packets / data on the other endpoint in userspace, and I
>>> don't think any driver / lower layer would keep memory
>>> after returning an error.
>>
>> As I'm also working on a socket driver for smbdirect,
>> I already thought about how I could hook into
>> IORING_OP_SEND[MSG]_ZC, and for sendmsg I'd have
>> a loop sending individual fragments, which have a reference,
>> but if I find a connection drop after the first one, I'd
>> return ECONNRESET or EPIPE in order to get faster recovery
>> instead of announcing a short write to the caller.
> 
> I doesn't sound right for me, but neither I know samba to
> really have an opinion. In any case, I see how it may be
> more robust if we always try to push a notification cqe.
> Will you send a patch?

You mean the IORING_OP_SEND_ZC should always
issue a NOTIF cqe, one it passed the io_sendzc_prep stage?

>> If we would take my 5/5 we could also have a different
>> strategy to check decide if MORE/NOTIF is needed.
>> If notif->cqe.res is still 0 and io_notif_flush drops
>> the last reference we could go without MORE/NOTIF at all.
>> In all other cases we'd either set MORE/NOTIF at the end
>> of io_sendzc of in the fail hook.
> 
> I had a similar optimisation, i.e. when io_notif_flush() in
> the submission path is dropping the last ref, but killed it
> as it was completely useless, I haven't hit this path even
> once even with UDP, not to mention TCP.

If I remember correctly I hit it all the time on loopback,
but I'd have to recheck.

>>> In any case, I was looking on a bit different problem, but
>>> it should look much cleaner using the same approach, see
>>> branch [1], and patch [3] for sendzc in particular.
>>>
>>> [1] https://github.com/isilence/linux.git partial-fail
>>> [2] https://github.com/isilence/linux/tree/io_uring/partial-fail
>>> [3] https://github.com/isilence/linux/commit/acb4f9bf869e1c2542849e11d992a63d95f2b894
>>
>>      const struct io_op_def *def = &io_op_defs[req->opcode];
>>
>>      req_set_fail(req);
>>      io_req_set_res(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
>>      if (def->fail)
>>          def->fail(req);
>>      io_req_complete_post(req);
>>
>> Will loose req->cqe.flags, but the fail hook in general looks like a good idea.
> 
> I just don't like those sporadic changes all across core io_uring
> code also adding some overhead.
> 
>> And don't we care about the other failure cases where req->cqe.flags gets overwritten?
> 
> We don't usually carry them around ->issue handler boundaries,
> e.g. directly do io_post_aux_cqe(res, IORING_CQE_F_MORE);
> 
> IORING_CQE_F_BUFFER is a bit more trickier, but there is
> special handling for this one and it wouldn't fit "set cflags
> in advance" logic anyway.
> 
> iow, ->fail callback sounds good enough for now, we'll change
> it later if needed.

The fail hook should re-add the MORE flag?

So I'll try to do the following changes:

1. take your ->fail() patches

2. once io_sendzc_prep() is over always trigger MORE/NOFIF cqes
    (But the documentation should still make it clear that
     userspace have to cope with just a single cqe (without MORE)
     for both successs and failure, so that we can improve things later)

3. Can I change the cqe.res of the NOTIF cqe to be 0xffffffff ?
    That would indicate to userspace that we don't give any information
    if zero copy was actually used or not. This would present someone
    from relying on cqe.res == 0 and we can improve it by providing
    more useful values in future.

Are you ok with that plan for 6.0?

metze