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 17:56, Pavel Begunkov wrote:
> On 9/28/22 16:23, Stefan Metzmacher wrote:
>> 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?

Right

>> 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()

Took a closer look, that chunk above looks good and matches
io_req_map_rw() apart from non essential differences. Can you
send a patch?


>> 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?

We can reuse it, we own it and it's immutable from
the iter perspective.

>> BTW: I tested with 5 vectors with length like this 4, 0, 64, 32, 8388608
>> and got a short write with about ~ 2000000.

Which is interesting to know. What does 2M here mean? Is it
consistently retries when sending more than 2M bytes?

-- 
Pavel Begunkov