Re: [PATCH v7 1/4] io_uring: Add io_uring04 test for vectored I/O operations

Hi Sebastian
> I see some fundamental problems here which admittedly started before this
> series. I believe these need to be fixed before we apply more tests on top
> of it.
>
> In io_uring_submit_sqe() and io_uring_submit_sqe_vec(), opcode is passed as
> user_data:
> io_uring_submit_sqe_internal(s, fd, opcode, (unsigned long)buf,
>                               len, offset, opcode);
>
> user_data is supposed to be a unique per-request identifier for correlating
> CQEs back to their SQEs. Using opcode there is meaningless as a tag.
>
> The existing tests happen to pass because io_uring_do_io_op() and
> io_uring_do_vec_io_op() always submit exactly one SQE and immediately
> block waiting for its CQE, so correlation is never actually needed.
> However, any test with multiple in-flight SQEs of the same opcode would
> pass;
> the user_data check in io_uring_wait-cqe() would match regardless of which
> CQE corresponds to which request.
>
> Please hold off on applying this series until the underlying issue is
> fixed. I will send a patch to correct the user_data handling.
You're right.

In io_uring_submit_sqe() and io_uring_submit_sqe_vec(), we currently 
pass the
opcode as user_data through io_uring_submit_sqe_internal().
As you pointed out, that is not a meaningful per-request identifier and it
breaks CQE-to-SQE correlation once there is more than one in-flight request
with the same opcode.

> I also think io_uring_do_io_op() and io_uring_do_vec_io_op() should not
> tightly couple SQE submission with waiting for a CQE.
> Keeping them as a single helper makes the tests effectively synchronous and
> does not model real io_uring usage.
> It would be cleaner to have separate helpers for submitting SQEs and
> waiting/reaping CQEs, and let each test handle CQE validation and
> result reporting in its own body.
> Having basic smoke tests, sending single SQE and checking CQE makes sense
> in LTP, but I doubt we shall have this as a helper.

The existing helpers
io_uring_do_io_op() and io_uring_do_vec_io_op() hide this because they
submit a single SQE and immediately wait for a single CQE, so the tests
are effectively synchronous. That makes the current code work for simple
smoke coverage, but it is not a good base for adding more
io_uring tests.

I agree that we should fix this first before applying more tests on top.

The right direction seems to be:

- stop using opcode as user_data
- make user_data a caller-provided per-request tag/context
- separate SQE submission from CQE waiting/reaping
- let each test validate CQEs and report results explicitly as needed

I'll wait for your patch and rework the helpers/tests on top of that 
approach.
> Thanks and let me know if I misunderstand something here. It's the first
> time I look at this code really.
> Sebastian
>

Nope, you are spot-on with the analysis. Let me know if I can help in 
any way.

-- 
Thanks
- Sachin



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp