On 12/4/21 22:21, Jens Axboe wrote:
On 12/4/21 1:49 PM, Pavel Begunkov wrote:
Currently, IOPOLL returns the number of completed requests, but with
REQ_F_CQE_SKIP there are not the same thing anymore. That may be
confusing as non-iopoll wait cares only about CQEs, so make io_do_iopoll
return the number of posted CQEs.
Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
---
fs/io_uring.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 64add8260abb..ea7a0daa0b3b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2538,10 +2538,10 @@ static int io_do_iopoll(struct io_ring_ctx *ctx, bool force_nonspin)
/* order with io_complete_rw_iopoll(), e.g. ->result updates */
if (!smp_load_acquire(&req->iopoll_completed))
break;
+ if (unlikely(req->flags & REQ_F_CQE_SKIP))
+ continue;
- if (!(req->flags & REQ_F_CQE_SKIP))
- __io_fill_cqe(ctx, req->user_data, req->result,
- io_put_kbuf(req));
+ __io_fill_cqe(ctx, req->user_data, req->result, io_put_kbuf(req));
nr_events++;
}
Not sure I follow the logic behind this change. Places like
io_iopoll_try_reap_events() just need a "did we find anything" return,
which is independent on whether or not we actually posted CQEs or not.
Other callers either don't care what the return value is or if it's < 0
or not (which this change won't affect).
I feel like I'm missing something here, or that the commit message
better needs to explain why this change is done.
I was wrong on how I described it, but it means that the problem is in
a different place.
int io_do_iopoll() {
return nr_events;
}
int io_iopoll_check() {
do {
nr_events += io_do_iopoll();
while (nr_events < min && ...);
}
And "events" there better to be CQEs, otherwise the semantics
of @min + CQE_SKIP is not very clear and mismatches non-IOPOLL.
--
Pavel Begunkov
