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

[Stefan Metzmacher <metze@samba.org>]

Am 17.09.22 um 11:16 schrieb Pavel Begunkov:
> On 9/16/22 22:36, Stefan Metzmacher wrote:
>> Hi Pavel, hi Jens,
>>
>> I did some initial testing with IORING_OP_SEND_ZC.
>> While reading the code I think I found a race that
>> can lead to IORING_CQE_F_MORE being missing even if
>> the net layer got references.
> 
> Hey Stefan,
> 
> Did you see some kind of buggy behaviour in userspace?

No I was just reading the code and found it a bit confusing,
and couldn't prove that we don't have a problem with loosing
a notif cqe.

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

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.

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

And don't we care about the other failure cases where req->cqe.flags gets overwritten?

metze