Re: [PATCH for-6.1] io_uring/net: don't skip notifs for failed requests

[Pavel Begunkov <asml.silence@gmail.com>]

On 9/28/22 16:23, Stefan Metzmacher wrote:
> 
> Hi Pavel,
> 
>> We currently only add a notification CQE when the send succeded, i.e.
>> cqe.res >= 0. However, it'd be more robust to do buffer notifications
>> for failed requests as well in case drivers decide do something fanky.
>>
>> Always return a buffer notification after initial prep, don't hide it.
>> This behaviour is better aligned with documentation and the patch also
>> helps the userspace to respect it.
> 
> Just as reference, this was the version I was testing with:
> https://git.samba.org/?p=metze/linux/wip.git;a=commitdiff;h=7ffb896cdb8ccd55065f7ffae9fb8050e39211c7
> 
>>   void io_sendrecv_fail(struct io_kiocb *req)
>>   {
>>       struct io_sr_msg *sr = io_kiocb_to_cmd(req, struct io_sr_msg);
>> -    int res = req->cqe.res;
>>       if (req->flags & REQ_F_PARTIAL_IO)
>> -        res = sr->done_io;
>> +        req->cqe.res = sr->done_io;
>> +
>>       if ((req->flags & REQ_F_NEED_CLEANUP) &&
>> -        (req->opcode == IORING_OP_SEND_ZC || req->opcode == IORING_OP_SENDMSG_ZC)) {
>> -        /* preserve notification for partial I/O */
>> -        if (res < 0)
>> -            sr->notif->flags |= REQ_F_CQE_SKIP;
>> -        io_notif_flush(sr->notif);
>> -        sr->notif = NULL;
> 
> Here we rely on io_send_zc_cleanup(), correct?
> 
> Note that I hit a very bad problem during my tests of SENDMSG_ZC.
> BUG(); in first_iovec_segment() triggered very easily.
> The problem is io_setup_async_msg() in the partial retry case,
> which seems to happen more often with _ZC.
> 
>         if (!async_msg->free_iov)
>                 async_msg->msg.msg_iter.iov = async_msg->fast_iov;
> 
> Is wrong it needs to be something like this:
> 
> +       if (!kmsg->free_iov) {
> +               size_t fast_idx = kmsg->msg.msg_iter.iov - kmsg->fast_iov;
> +               async_msg->msg.msg_iter.iov = &async_msg->fast_iov[fast_idx];
> +       }

I agree, it doesn't look right. It indeed needs sth like
io_uring/rw.c:io_req_map_rw()


> As iov_iter_iovec_advance() may change i->iov in order to have i->iov_offset
> being only relative to the first element.
> 
> I'm not sure about the 'kmsg->free_iov' case, do we reuse the
> callers memory or should we make a copy?
> I initially used this
> https://git.samba.org/?p=metze/linux/wip.git;a=commitdiff;h=e1d3a9f5c7708a37172d258753ed7377eaac9e33
> But I didn't test with the non-fast_iov case.
> 
> BTW: I tested with 5 vectors with length like this 4, 0, 64, 32, 8388608
> and got a short write with about ~ 2000000.
> 
> I'm not sure if it was already a problem before:
> 
> commit 257e84a5377fbbc336ff563833a8712619acce56
> io_uring: refactor sendmsg/recvmsg iov managing
> 
> But I guess it was a potential problem before starting with
> 7ba89d2af17aa879dda30f5d5d3f152e587fc551 where io_net_retry()
> was introduced.
> 
> metze

-- 
Pavel Begunkov