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

[Stefan Metzmacher <metze@samba.org>]

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];
+       }

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