在 2021/9/3 下午8:27, Pavel Begunkov 写道:

On 9/3/21 1:08 PM, Hao Xu wrote:

quoted

在 2021/9/3 下午7:42, Pavel Begunkov 写道:

quoted

On 9/3/21 12:00 PM, Hao Xu wrote:

quoted

Though currently refcount of a req is always one when we flush inline

It can be refcounted and != 1. E.g. poll requests, or consider

It seems poll requests don't leverage comp cache, do I miss anything?

Hmm, looks so. Not great that it doesn't, but probably it's because
of trying to submit next reqs right in io_poll_task_func().

I'll be pushing for some changes around tw, with it should be easy
to hook poll completion batching with no drawbacks. Would be great
if you will be willing to take a shot on it.

Sure, I'll take a look.

quoted

quoted

that tw also flushes, and you may have a read that goes to apoll
and then get tw resubmitted from io_async_task_func(). And other

when it goes to apoll, (say no double wait entry) ref is 1, then read
completes inline and then the only ref is droped by flush.

Yep, but some might have double entry. It also will have elevated

double entry should be fine.

refs if there was a linked timeout. Another case is io-wq, which

Haven't dig into linked timeout yet.

takes a reference, and if iowq doesn't put it for a while (e.g. got
rescheduled) but requests goes to tw (resubmit, poll, whatever)
you have the same picture.

Gotcha, thanks.

quoted

quoted

cases.

quoted

completions, but still a chance there will be exception in the future.
Enhance the flush logic to make sure we maintain compl_nr correctly.

See below

quoted

Signed-off-by: Hao Xu <redacted>
---

we need to either removing the if check to claim clearly that the req's
refcount is 1 or adding this patch's logic.

   fs/io_uring.c | 6 ++++--
   1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2bde732a1183..c48d43207f57 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c

@@ -2291,7 +2291,7 @@ static void io_submit_flush_completions(struct io_ring_ctx *ctx)
       __must_hold(&ctx->uring_lock)
   {
       struct io_submit_state *state = &ctx->submit_state;
-    int i, nr = state->compl_nr;
+    int i, nr = state->compl_nr, remain = 0;
       struct req_batch rb;
         spin_lock(&ctx->completion_lock);

@@ -2311,10 +2311,12 @@ static void io_submit_flush_completions(struct io_ring_ctx *ctx)
             if (req_ref_put_and_test(req))
               io_req_free_batch(&rb, req, &ctx->submit_state);
+        else
+            state->compl_reqs[remain++] = state->compl_reqs[i];

Our ref is dropped, and something else holds another reference. That
"something else" is responsible to free the request once it put the last
reference. This chunk would make the following io_submit_flush_completions()
to underflow refcount and double free.

True, I see. Thanks Pavel.

quoted

quoted

       }
         io_req_free_batch_finish(ctx, &rb);
-    state->compl_nr = 0;
+    state->compl_nr = remain;
   }
     /*