remove aux CQE caches

We can run normal task_work without locking the ctx, however we try to lock anyway and most handlers prefer or require it locked. It might have been interesting to multi-submitter ring with high contention completing async read/write requests via task_work, however that will still need to go through io_req_complete_post() and potentially take the lock for rsrc node putting or some other case.

In other words, it's hard to care about it, so alawys force the locking. The case described would also because of various io_uring caches.

Quoted 7 lines · Pavel Begunkov

We can run normal task_work without locking the ctx, however we try to lock anyway and most handlers prefer or require it locked. It might have been interesting to multi-submitter ring with high contention completing async read/write requests via task_work, however that will still need to go through io_req_complete_post() and potentially take the lock for rsrc node putting or some other case.

Quoted 2 lines

In other words, it's hard to care about it, so alawys force the locking. The case described would also because of various io_uring caches.

This is a good idea, I've had that thought myself too. The conditional aspect of it is annoying, and by far the most interesting use cases will do the locking anyway.

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

Quoted 4 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

Quoted 5 lines

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 11 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

Quoted 2 lines

This breaks the read-mshot test case, looking into what is going on there.

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

It does, this should fix it:

test/read-mshot.c +0 -0

index 8fcb79857bf0..501ca69a98dc 100644

Quoted 27 lines · Jens Axboe

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

It does, this should fix it:

test/read-mshot.c static int test(int first_good, int async, int overflow) · line 239

Quoted 10 lines

test/read-mshot.c +1 -1

index 8fcb79857bf0..501ca69a98dc 100644
@@ -236,7 +236,7 @@ static int test(int first_good, int async, int overflow)
          }
          if (!(cqe->flags & IORING_CQE_F_MORE)) {
              /* we expect this on overflow */
-            if (overflow && (i - 1 == NR_OVERFLOW))
+            if (overflow && i >= NR_OVERFLOW)

Which is not ideal either, e.g. I wouldn't mind if the kernel stops one entry before CQ is full, so that the request can complete w/o overflowing. Not supposing the change because it's a marginal case, but we shouldn't limit ourselves.

Quoted 28 lines · Pavel Begunkov

Quoted 27 lines · Jens Axboe

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

It does, this should fix it:

Quoted 15 lines

test/read-mshot.c static int test(int first_good, int async, int overflow) · line 239

Quoted 10 lines

test/read-mshot.c +1 -1

index 8fcb79857bf0..501ca69a98dc 100644
@@ -236,7 +236,7 @@ static int test(int first_good, int async, int overflow)
           }
           if (!(cqe->flags & IORING_CQE_F_MORE)) {
               /* we expect this on overflow */
-            if (overflow && (i - 1 == NR_OVERFLOW))
+            if (overflow && i >= NR_OVERFLOW)

Which is not ideal either, e.g. I wouldn't mind if the kernel stops one entry before CQ is full, so that the request can complete w/o overflowing. Not supposing the change because it's a marginal case, but we shouldn't limit ourselves.

But if the event keeps triggering we have to keep posting CQEs, otherwise we could get stuck.

As far as I'm concerned, the behavior with
the patch looks correct. The last CQE is overflown, and that terminates it, and it doesn't have MORE set. The one before that has MORE set, but it has to, unless you aborted it early. But that seems impossible, because what if that was indeed the last current CQE, and we reap CQEs before the next one is posted.

So unless I'm missing something, I don't think we can be doing any better.

Quoted 45 lines · Jens Axboe

Quoted 43 lines · Pavel Begunkov

Quoted 32 lines · Jens Axboe

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

It does, this should fix it:

test/read-mshot.c +0 -0

index 8fcb79857bf0..501ca69a98dc 100644

test/read-mshot.c static int test(int first_good, int async, int overflow) · line 239

Quoted 5 lines

test/read-mshot.c +1 -1

@@ -236,7 +236,7 @@ static int test(int first_good, int async, int overflow)
           }
           if (!(cqe->flags & IORING_CQE_F_MORE)) {
               /* we expect this on overflow */
-            if (overflow && (i - 1 == NR_OVERFLOW))
+            if (overflow && i >= NR_OVERFLOW)

Which is not ideal either, e.g. I wouldn't mind if the kernel stops one entry before CQ is full, so that the request can complete w/o overflowing. Not supposing the change because it's a marginal case, but we shouldn't limit ourselves.

Quoted 2 lines

But if the event keeps triggering we have to keep posting CQEs, otherwise we could get stuck.

Or we can complete the request, then the user consumes CQEs and restarts as usual

Quoted 9 lines

As far as I'm concerned, the behavior with
the patch looks correct. The last CQE is overflown, and that terminates it, and it doesn't have MORE set. The one before that has MORE set, but it has to, unless you aborted it early. But that seems impossible, because what if that was indeed the last current CQE, and we reap CQEs before the next one is posted.

So unless I'm missing something, I don't think we can be doing any better.

You can opportunistically try to avoid overflows, unreliably

bool io_post_cqe() {
    // Not enough space in the CQ left, so if there is a next
    // completion pending we'd have to overflow. Avoid that by
    // terminating it now.
    //
    // If there are no more CQEs after this one, we might
    // terminate a bit earlier, but that better because
    // overflows are so expensive and unhandy and so on.
    if (cq_space_left() <= 1)
        return false;
    fill_cqe();
    return true;
}

some_multishot_function(req) {
    if (!io_post_cqe(res))
        complete_req(req, res);
}

Again, not suggesting the change for all the obvious reasons, but I think semantically we should be able to do it.

Quoted 46 lines · Pavel Begunkov

Quoted 45 lines · Jens Axboe

Quoted 43 lines · Pavel Begunkov

Quoted 32 lines · Jens Axboe

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

It does, this should fix it:

test/read-mshot.c +0 -0

index 8fcb79857bf0..501ca69a98dc 100644

test/read-mshot.c static int test(int first_good, int async, int overflow) · line 239

Quoted 5 lines

test/read-mshot.c +1 -1

@@ -236,7 +236,7 @@ static int test(int first_good, int async, int overflow)
            }
            if (!(cqe->flags & IORING_CQE_F_MORE)) {
                /* we expect this on overflow */
-            if (overflow && (i - 1 == NR_OVERFLOW))
+            if (overflow && i >= NR_OVERFLOW)

Which is not ideal either, e.g. I wouldn't mind if the kernel stops one entry before CQ is full, so that the request can complete w/o overflowing. Not supposing the change because it's a marginal case, but we shouldn't limit ourselves.

Quoted 5 lines

Quoted 2 lines

But if the event keeps triggering we have to keep posting CQEs, otherwise we could get stuck.

Or we can complete the request, then the user consumes CQEs and restarts as usual

So you'd want to track if we'd overflow, wait for overflow to clear, and then restart that request?

I think that sounds a bit involved, no?
Particularly for a case like overflow, which generally should not occur. If it does, just terminate it, and have the user re-issue it. That seems like the simpler and better solution to me.

Quoted 31 lines

Quoted 9 lines

As far as I'm concerned, the behavior with
the patch looks correct. The last CQE is overflown, and that terminates it, and it doesn't have MORE set. The one before that has MORE set, but it has to, unless you aborted it early. But that seems impossible, because what if that was indeed the last current CQE, and we reap CQEs before the next one is posted.

So unless I'm missing something, I don't think we can be doing any better.

You can opportunistically try to avoid overflows, unreliably

bool io_post_cqe() {
     // Not enough space in the CQ left, so if there is a next
     // completion pending we'd have to overflow. Avoid that by
     // terminating it now.
     //
     // If there are no more CQEs after this one, we might
     // terminate a bit earlier, but that better because
     // overflows are so expensive and unhandy and so on.
     if (cq_space_left() <= 1)
         return false;
     fill_cqe();
     return true;
}

some_multishot_function(req) {
     if (!io_post_cqe(res))
         complete_req(req, res);
}

Quoted 2 lines

Again, not suggesting the change for all the obvious reasons, but I think semantically we should be able to do it.

Yeah not convinced this is worth looking at. If it was the case that the hot path would often see overflows and it'd help to avoid it, then probably it'd make sense. But I don't think that's the case.

Quoted 50 lines · Jens Axboe

Quoted 49 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

Quoted 27 lines · Jens Axboe

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

It does, this should fix it:

Quoted 15 lines

test/read-mshot.c static int test(int first_good, int async, int overflow) · line 239

Quoted 10 lines

test/read-mshot.c +1 -1

index 8fcb79857bf0..501ca69a98dc 100644
@@ -236,7 +236,7 @@ static int test(int first_good, int async, int overflow)
            }
            if (!(cqe->flags & IORING_CQE_F_MORE)) {
                /* we expect this on overflow */
-            if (overflow && (i - 1 == NR_OVERFLOW))
+            if (overflow && i >= NR_OVERFLOW)

Which is not ideal either, e.g. I wouldn't mind if the kernel stops one entry before CQ is full, so that the request can complete w/o overflowing. Not supposing the change because it's a marginal case, but we shouldn't limit ourselves.

But if the event keeps triggering we have to keep posting CQEs, otherwise we could get stuck.

Quoted 5 lines

Quoted 2 lines

Or we can complete the request, then the user consumes CQEs and restarts as usual

So you'd want to track if we'd overflow, wait for overflow to clear, and then restart that request?

No, the 2 line change in io_post_cqe() from the last email's snippet is the only thing you'd need.

I probably don't understand why and what tracking you mean, but fwiw we currently do track and account for overflows.

Quoted 36 lines

I think that sounds a bit involved, no?
Particularly for a case like overflow, which generally should not occur. If it does, just terminate it, and have the user re-issue it. That seems like the simpler and better solution to me.

Quoted 31 lines

Quoted 9 lines

As far as I'm concerned, the behavior with
the patch looks correct. The last CQE is overflown, and that terminates it, and it doesn't have MORE set. The one before that has MORE set, but it has to, unless you aborted it early. But that seems impossible, because what if that was indeed the last current CQE, and we reap CQEs before the next one is posted.

So unless I'm missing something, I don't think we can be doing any better.

You can opportunistically try to avoid overflows, unreliably

bool io_post_cqe() {
     // Not enough space in the CQ left, so if there is a next
     // completion pending we'd have to overflow. Avoid that by
     // terminating it now.
     //
     // If there are no more CQEs after this one, we might
     // terminate a bit earlier, but that better because
     // overflows are so expensive and unhandy and so on.
     if (cq_space_left() <= 1)
         return false;
     fill_cqe();
     return true;
}

some_multishot_function(req) {
     if (!io_post_cqe(res))
         complete_req(req, res);
}

Quoted 6 lines

Quoted 2 lines

Again, not suggesting the change for all the obvious reasons, but I think semantically we should be able to do it.

Yeah not convinced this is worth looking at. If it was the case that the hot path would often see overflows and it'd help to avoid it, then probably it'd make sense. But I don't think that's the case.

We're talking about different things. Seems you're discussing a particular implementation, its constraints and performance. I care purely about the semantics, the implicit uapi. And I define it as "multishot requests may decide to terminate at any point, the user should expect it and reissue when appropriate", not restricting it to "can only (normally) terminate when CQ is full".

We're changing tests from time to time, but the there is that "behaviour defines semantics", especially when it wasn't clear in advance and breaks someone's app, and people might be using assumptions in tests as the universal truth.

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Pavel Begunkov

Quoted 13 lines · Jens Axboe

Quoted 9 lines · Pavel Begunkov

io_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.

DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.

This breaks the read-mshot test case, looking into what is going on there.

Quoted 6 lines

Quoted 3 lines

I forgot to mention, yes it does, the test makes odd assumptions about overflows, IIRC it expects that the kernel allows one and only one aux CQE to be overflown. Let me double check

Yeah this is very possible, the overflow checking could be broken in there. I'll poke at it and report back.

test() {
    if (!(cqe->flags & IORING_CQE_F_MORE)) {
        /* we expect this on overflow */
        if (overflow && (i - 1 == NR_OVERFLOW))
            break;
        fprintf(stderr, "no more cqes\n");
        return 1;
    }
    ...
}

It's this chunk. I think I silenced it with

s/i - 1 == NR_OVERFLOW/i == NR_OVERFLOW/

but it should probably be i >= NR_OVERFLOW or so

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

The nvme cmd change is tested with io_uring_passthrough.c, however it doesn't seem there is anything in liburing exercising ublk paths. How do we test it? It'd be great to have at least some basic tests for it.

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Quoted 6 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

Quoted 5 lines

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

Quoted 3 lines

Quoted 1 line

[...]

Applied, thanks!

Hi Jens and Pavel,

Looks this patch causes hang when running './check ublk/002' in blktests.

Quoted 18 lines · Ming Lei

Quoted 15 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

Quoted 3 lines

Hi Jens and Pavel,

Looks this patch causes hang when running './check ublk/002' in blktests.

Not take close look, and I guess it hangs in

io_uring_cmd_del_cancelable() -> io_ring_submit_lock

Quoted 19 lines · Ming Lei

Quoted 18 lines · Ming Lei

Quoted 15 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

Quoted 7 lines

Quoted 3 lines

Hi Jens and Pavel,

Looks this patch causes hang when running './check ublk/002' in blktests.

Not take close look, and I guess it hangs in

io_uring_cmd_del_cancelable() -> io_ring_submit_lock

Thanks, the trace doesn't completely explains it, but my blind spot was io_uring_cmd_done() potentially grabbing the mutex. They're supposed to be irq safe mimicking io_req_task_work_add(), that's how nvme passthrough uses it as well (but at least it doesn't need the cancellation bits).

One option is to replace it with a spinlock, the other is to delay the io_uring_cmd_del_cancelable() call to the task_work callback. The latter would be cleaner and more preferable, but I'm lacking context to tell if that would be correct. Ming, what do you think?

Quoted 34 lines · Pavel Begunkov

Quoted 19 lines · Ming Lei

Quoted 18 lines · Ming Lei

Quoted 15 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

Quoted 7 lines

Quoted 3 lines

Hi Jens and Pavel,

Looks this patch causes hang when running './check ublk/002' in blktests.

Not take close look, and I guess it hangs in

io_uring_cmd_del_cancelable() -> io_ring_submit_lock

Thanks, the trace doesn't completely explains it, but my blind spot was io_uring_cmd_done() potentially grabbing the mutex. They're supposed to be irq safe mimicking io_req_task_work_add(), that's how nvme passthrough uses it as well (but at least it doesn't need the cancellation bits).

Quoted 4 lines

One option is to replace it with a spinlock, the other is to delay the io_uring_cmd_del_cancelable() call to the task_work callback. The latter would be cleaner and more preferable, but I'm lacking context to tell if that would be correct. Ming, what do you think?

I prefer to the latter approach because the two cancelable helpers are run in fast path.

Looks all new io_uring_cmd_complete() in ublk have this issue, and the following patch should avoid them all.

Quoted 40 lines · Ming Lei

Quoted 34 lines · Pavel Begunkov

Quoted 19 lines · Ming Lei

Quoted 18 lines · Ming Lei

Quoted 15 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

Quoted 7 lines

Quoted 3 lines

Hi Jens and Pavel,

Looks this patch causes hang when running './check ublk/002' in blktests.

Not take close look, and  I guess it hangs in

io_uring_cmd_del_cancelable() -> io_ring_submit_lock

Thanks, the trace doesn't completely explains it, but my blind spot was io_uring_cmd_done() potentially grabbing the mutex. They're supposed to be irq safe mimicking io_req_task_work_add(), that's how nvme passthrough uses it as well (but at least it doesn't need the cancellation bits).

Quoted 4 lines

One option is to replace it with a spinlock, the other is to delay the io_uring_cmd_del_cancelable() call to the task_work callback. The latter would be cleaner and more preferable, but I'm lacking context to tell if that would be correct. Ming, what do you think?

Quoted 5 lines

I prefer to the latter approach because the two cancelable helpers are run in fast path.

Looks all new io_uring_cmd_complete() in ublk have this issue, and the following patch should avoid them all.

The one I have in mind on top of the current tree would be like below. Untested, and doesn't allow this cancellation thing for iopoll. I'll prepare something tomorrow.

io_uring/uring_cmd.c +10 -7

index e45d4cd5ef82..000ba435451c 100644
@@ -14,19 +14,15 @@
  #include "rsrc.h"
  #include "uring_cmd.h"
 
-static void io_uring_cmd_del_cancelable(struct io_uring_cmd *cmd,
-        unsigned int issue_flags)
+static void io_uring_cmd_del_cancelable(struct io_uring_cmd *cmd)
  {
      struct io_kiocb *req = cmd_to_io_kiocb(cmd);
-    struct io_ring_ctx *ctx = req->ctx;
 
      if (!(cmd->flags & IORING_URING_CMD_CANCELABLE))
          return;
 
      cmd->flags &= ~IORING_URING_CMD_CANCELABLE;
-    io_ring_submit_lock(ctx, issue_flags);
      hlist_del(&req->hash_node);
-    io_ring_submit_unlock(ctx, issue_flags);
  }
 
  /*

@@ -80,6 +76,15 @@ static inline void io_req_set_cqe32_extra(struct io_kiocb *req,
      req->big_cqe.extra2 = extra2;
  }
 
+static void io_req_task_cmd_complete(struct io_kiocb *req,
+                     struct io_tw_state *ts)
+{
+    struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
+
+    io_uring_cmd_del_cancelable(ioucmd);
+    io_req_task_complete(req, ts);
+}
+
  /*
   * Called by consumers of io_uring_cmd, if they originally returned
   * -EIOCBQUEUED upon receiving the command.

@@ -89,8 +94,6 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,
  {
      struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
 
-    io_uring_cmd_del_cancelable(ioucmd, issue_flags);
-
      if (ret < 0)
          req_set_fail(req);
 

io_uring/uring_cmd.c +1 -1

          io_req_complete_defer(req);
      } else {
-        req->io_task_work.func = io_req_task_complete;
+        req->io_task_work.func = io_req_task_cmd_complete;
          io_req_task_work_add(req);
      }
  }

Quoted 102 lines · Pavel Begunkov

Quoted 40 lines · Ming Lei

Quoted 34 lines · Pavel Begunkov

Quoted 19 lines · Ming Lei

Quoted 18 lines · Ming Lei

Quoted 15 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

Quoted 7 lines

Quoted 3 lines

Hi Jens and Pavel,

Looks this patch causes hang when running './check ublk/002' in blktests.

Not take close look, and  I guess it hangs in

io_uring_cmd_del_cancelable() -> io_ring_submit_lock

Thanks, the trace doesn't completely explains it, but my blind spot was io_uring_cmd_done() potentially grabbing the mutex. They're supposed to be irq safe mimicking io_req_task_work_add(), that's how nvme passthrough uses it as well (but at least it doesn't need the cancellation bits).

Quoted 4 lines

One option is to replace it with a spinlock, the other is to delay the io_uring_cmd_del_cancelable() call to the task_work callback. The latter would be cleaner and more preferable, but I'm lacking context to tell if that would be correct. Ming, what do you think?

Quoted 5 lines

I prefer to the latter approach because the two cancelable helpers are run in fast path.

Looks all new io_uring_cmd_complete() in ublk have this issue, and the following patch should avoid them all.

The one I have in mind on top of the current tree would be like below. Untested, and doesn't allow this cancellation thing for iopoll. I'll prepare something tomorrow.

io_uring/uring_cmd.c +10 -7

index e45d4cd5ef82..000ba435451c 100644
@@ -14,19 +14,15 @@
  #include "rsrc.h"
  #include "uring_cmd.h"
 
-static void io_uring_cmd_del_cancelable(struct io_uring_cmd *cmd,
-        unsigned int issue_flags)
+static void io_uring_cmd_del_cancelable(struct io_uring_cmd *cmd)
  {
      struct io_kiocb *req = cmd_to_io_kiocb(cmd);
-    struct io_ring_ctx *ctx = req->ctx;
 
      if (!(cmd->flags & IORING_URING_CMD_CANCELABLE))
          return;
 
      cmd->flags &= ~IORING_URING_CMD_CANCELABLE;
-    io_ring_submit_lock(ctx, issue_flags);
      hlist_del(&req->hash_node);
-    io_ring_submit_unlock(ctx, issue_flags);
  }
 
  /*

@@ -80,6 +76,15 @@ static inline void io_req_set_cqe32_extra(struct io_kiocb *req,
      req->big_cqe.extra2 = extra2;
  }
 
+static void io_req_task_cmd_complete(struct io_kiocb *req,
+                     struct io_tw_state *ts)
+{
+    struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
+
+    io_uring_cmd_del_cancelable(ioucmd);
+    io_req_task_complete(req, ts);
+}
+
  /*
   * Called by consumers of io_uring_cmd, if they originally returned
   * -EIOCBQUEUED upon receiving the command.

@@ -89,8 +94,6 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,
  {
      struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
 
-    io_uring_cmd_del_cancelable(ioucmd, issue_flags);
-
      if (ret < 0)
          req_set_fail(req);
 

io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 108

Quoted 2 lines

io_uring/uring_cmd.c +0 -0

@@ -105,7 +108,7 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,
              return;

Not very well thought through... Here should be a *del_cancelable call as well

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

[02/11] io_uring/cmd: kill one issue_flags to tw conversion

        commit: 31ab0342cf6434e1e2879d12f0526830ce97365d
[03/11] io_uring/cmd: fix tw <-> issue_flags conversion
        commit: b48f3e29b89055894b3f50c657658c325b5b49fd
[04/11] io_uring/cmd: introduce io_uring_cmd_complete
        commit: c5b4c92ca69215c0af17e4e9d8c84c8942f3257d
[05/11] ublk: don't hard code IO_URING_F_UNLOCKED
        commit: c54cfb81fe1774231fca952eff928389bfc3b2e3
[06/11] nvme/io_uring: don't hard code IO_URING_F_UNLOCKED
        commit: 800a90681f3c3383660a8e3e2d279e0f056afaee
[07/11] io_uring/rw: avoid punting to io-wq directly
        commit: 56d565d54373c17b7620fc605c899c41968e48d0

[08/11] io_uring: force tw ctx locking

        commit: f087cdd065af0418ffc8a9ed39eadc93347efdd5
[09/11] io_uring: remove struct io_tw_state::locked
        commit: 339f8d66e996ec52b47221448ff4b3534cc9a58d
[10/11] io_uring: refactor io_fill_cqe_req_aux
        commit: 7b31c3964b769a6a16c4e414baa8094b441e498e
[11/11] io_uring: get rid of intermediate aux cqe caches
        commit: 5a475a1f47412a44ed184aac04b9ff0aeaa31d65

Quoted 28 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

[02/11] io_uring/cmd: kill one issue_flags to tw conversion

         commit: 31ab0342cf6434e1e2879d12f0526830ce97365d
[03/11] io_uring/cmd: fix tw <-> issue_flags conversion
         commit: b48f3e29b89055894b3f50c657658c325b5b49fd
[04/11] io_uring/cmd: introduce io_uring_cmd_complete
         commit: c5b4c92ca69215c0af17e4e9d8c84c8942f3257d
[05/11] ublk: don't hard code IO_URING_F_UNLOCKED
         commit: c54cfb81fe1774231fca952eff928389bfc3b2e3
[06/11] nvme/io_uring: don't hard code IO_URING_F_UNLOCKED
         commit: 800a90681f3c3383660a8e3e2d279e0f056afaee
[07/11] io_uring/rw: avoid punting to io-wq directly
         commit: 56d565d54373c17b7620fc605c899c41968e48d0

Quoted 8 lines

[08/11] io_uring: force tw ctx locking

         commit: f087cdd065af0418ffc8a9ed39eadc93347efdd5
[09/11] io_uring: remove struct io_tw_state::locked
         commit: 339f8d66e996ec52b47221448ff4b3534cc9a58d
[10/11] io_uring: refactor io_fill_cqe_req_aux
         commit: 7b31c3964b769a6a16c4e414baa8094b441e498e
[11/11] io_uring: get rid of intermediate aux cqe caches
         commit: 5a475a1f47412a44ed184aac04b9ff0aeaa31d65

Hi Jens and Pavel,

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

1) error 1

[  318.843517] ------------[ cut here ]------------
[  318.843937] kernel BUG at mm/slub.c:553!
[  318.844235] invalid opcode: 0000 [#1] SMP NOPTI
[  318.844580] CPU: 7 PID: 1475 Comm: kworker/u48:13 Not tainted 6.8.0_io_uring_6.10+ #14
[  318.845133] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc37 04/01/2014
[  318.845732] Workqueue: events_unbound io_ring_exit_work
[  318.846104] RIP: 0010:__slab_free+0x152/0x2f0
[  318.846434] Code: 00 4c 89 ff e8 ef 41 bc 00 48 8b 14 24 48 8b 4c 24 20 48 89 44 24 08 48 8b 03 48 c1 e8 09 83 e0 01 88 44 24 13 e9 71 ff4
[  318.851192] RSP: 0018:ffffb490411abcb0 EFLAGS: 00010246
[  318.851574] RAX: ffff8b0e871e44f0 RBX: fffff113841c7900 RCX: 0000000000200010
[  318.852032] RDX: ffff8b0e871e4400 RSI: fffff113841c7900 RDI: ffffb490411abd20
[  318.852521] RBP: ffffb490411abd50 R08: 0000000000000001 R09: ffffffffa17e4deb
[  318.852981] R10: 0000000000200010 R11: 0000000000000024 R12: ffff8b0e80292c00
[  318.853472] R13: ffff8b0e871e4400 R14: ffff8b0e80292c00 R15: ffffffffa17e4deb
[  318.853911] FS:  0000000000000000(0000) GS:ffff8b13e7b80000(0000) knlGS:0000000000000000
[  318.854448] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  318.854831] CR2: 00007fbce5249298 CR3: 0000000363020002 CR4: 0000000000770ef0
[  318.855291] PKRU: 55555554
[  318.855533] Call Trace:
[  318.855724]  <TASK>
[  318.855898]  ? die+0x36/0x90
[  318.856121]  ? do_trap+0xdd/0x100
[  318.856389]  ? __slab_free+0x152/0x2f0
[  318.856674]  ? do_error_trap+0x6a/0x90
[  318.856939]  ? __slab_free+0x152/0x2f0
[  318.857202]  ? exc_invalid_op+0x50/0x70
[  318.857505]  ? __slab_free+0x152/0x2f0
[  318.857770]  ? asm_exc_invalid_op+0x1a/0x20
[  318.858056]  ? io_req_caches_free+0x9b/0x100
[  318.858439]  ? io_req_caches_free+0x9b/0x100
[  318.858961]  ? __slab_free+0x152/0x2f0
[  318.859466]  ? __memcg_slab_free_hook+0xd9/0x130
[  318.859941]  ? io_req_caches_free+0x9b/0x100
[  318.860395]  kmem_cache_free+0x2eb/0x3b0
[  318.860826]  io_req_caches_free+0x9b/0x100
[  318.861190]  io_ring_exit_work+0x105/0x5c0
[  318.861496]  ? __schedule+0x3d4/0x1510
[  318.861761]  process_one_work+0x181/0x350
[  318.862042]  worker_thread+0x27e/0x390
[  318.862307]  ? __pfx_worker_thread+0x10/0x10
[  318.862621]  kthread+0xbb/0xf0
[  318.862854]  ? __pfx_kthread+0x10/0x10
[  318.863124]  ret_from_fork+0x31/0x50
[  318.863397]  ? __pfx_kthread+0x10/0x10
[  318.863665]  ret_from_fork_asm+0x1a/0x30
[  318.863943]  </TASK>
[  318.864122] Modules linked in: isofs binfmt_misc xfs vfat fat raid0 iTCO_wdt intel_pmc_bxt iTCO_vendor_support virtio_net net_failover i2g
[  318.865638] ---[ end trace 0000000000000000 ]---
[  318.865966] RIP: 0010:__slab_free+0x152/0x2f0
[  318.866267] Code: 00 4c 89 ff e8 ef 41 bc 00 48 8b 14 24 48 8b 4c 24 20 48 89 44 24 08 48 8b 03 48 c1 e8 09 83 e0 01 88 44 24 13 e9 71 ff4
[  318.867622] RSP: 0018:ffffb490411abcb0 EFLAGS: 00010246
[  318.868103] RAX: ffff8b0e871e44f0 RBX: fffff113841c7900 RCX: 0000000000200010
[  318.868602] RDX: ffff8b0e871e4400 RSI: fffff113841c7900 RDI: ffffb490411abd20
[  318.869051] RBP: ffffb490411abd50 R08: 0000000000000001 R09: ffffffffa17e4deb
[  318.869544] R10: 0000000000200010 R11: 0000000000000024 R12: ffff8b0e80292c00
[  318.870028] R13: ffff8b0e871e4400 R14: ffff8b0e80292c00 R15: ffffffffa17e4deb
[  318.870550] FS:  0000000000000000(0000) GS:ffff8b13e7b80000(0000) knlGS:0000000000000000
[  318.871080] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  318.871509] CR2: 00007fbce5249298 CR3: 0000000363020002 CR4: 0000000000770ef0
[  318.871974] PKRU: 55555554

2) error 2

[ 2833.161174] ------------[ cut here ]------------
[ 2833.161527] WARNING: CPU: 11 PID: 22867 at kernel/fork.c:969 __put_task_struct+0x10c/0x180
[ 2833.162114] Modules linked in: isofs binfmt_misc vfat fat xfs raid0 iTCO_wdt intel_pmc_bxt iTCO_vendor_support i2c_i801 virtio_net i2c_smbus net_failover failover lpc_ich ublk_drv loop zram nvme nvme_core usb_storage crc32c_intel virtio_scsi virtio_blk fuse qemu_fw_cfg
[ 2833.163650] CPU: 11 PID: 22867 Comm: kworker/11:0 Tainted: G      D W          6.8.0_io_uring_6.10+ #14
[ 2833.164289] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc37 04/01/2014
[ 2833.164860] Workqueue: events io_fallback_req_func
[ 2833.165224] RIP: 0010:__put_task_struct+0x10c/0x180
[ 2833.165586] Code: 48 85 d2 74 05 f0 ff 0a 74 44 48 8b 3d d5 b6 c7 02 48 89 ee e8 65 b7 2d 00 eb ac be 03 00 00 00 48 89 ef e8 36 82 70 00 eb 9d <0f> 0b 8b 43 28 85 c0 0f 84 0e ff ff ff 0f 0b 65 48 3b 1d 5d d2 f2
[ 2833.166819] RSP: 0018:ffffb89da07a7df8 EFLAGS: 00010246
[ 2833.167210] RAX: 0000000000000000 RBX: ffff97d7d9332ec0 RCX: 0000000000000000
[ 2833.167685] RDX: 0000000000000001 RSI: 0000000000000246 RDI: ffff97d7d9332ec0
[ 2833.168167] RBP: ffff97d6cd9cc000 R08: 0000000000000000 R09: 0000000000000000
[ 2833.168664] R10: ffffb89da07a7db0 R11: 0000000000000100 R12: ffff97d7dee497f0
[ 2833.169161] R13: ffff97d7dee497f0 R14: ffff97d7400e9d00 R15: ffff97d6cd9cc410
[ 2833.169621] FS:  0000000000000000(0000) GS:ffff97dc27d80000(0000) knlGS:0000000000000000
[ 2833.170196] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2833.170578] CR2: 00007fa731cfe648 CR3: 0000000220b30004 CR4: 0000000000770ef0
[ 2833.171158] PKRU: 55555554
[ 2833.171394] Call Trace:
[ 2833.171596]  <TASK>
[ 2833.171779]  ? __warn+0x80/0x110
[ 2833.172044]  ? __put_task_struct+0x10c/0x180
[ 2833.172367]  ? report_bug+0x150/0x170
[ 2833.172637]  ? handle_bug+0x41/0x70
[ 2833.172899]  ? exc_invalid_op+0x17/0x70
[ 2833.173203]  ? asm_exc_invalid_op+0x1a/0x20
[ 2833.173522]  ? __put_task_struct+0x10c/0x180
[ 2833.173826]  ? io_put_task_remote+0x80/0x90
[ 2833.174153]  __io_submit_flush_completions+0x2bd/0x380
[ 2833.174509]  io_fallback_req_func+0xa3/0x130
[ 2833.174806]  process_one_work+0x181/0x350
[ 2833.175105]  worker_thread+0x27e/0x390
[ 2833.175394]  ? __pfx_worker_thread+0x10/0x10
[ 2833.175690]  kthread+0xbb/0xf0
[ 2833.175920]  ? __pfx_kthread+0x10/0x10
[ 2833.176226]  ret_from_fork+0x31/0x50
[ 2833.176485]  ? __pfx_kthread+0x10/0x10
[ 2833.176751]  ret_from_fork_asm+0x1a/0x30
[ 2833.177044]  </TASK>
[ 2833.177256] ---[ end trace 0000000000000000 ]---
[ 2833.177586] BUG: kernel NULL pointer dereference, address: 00000000000000e8
[ 2833.178054] #PF: supervisor read access in kernel mode
[ 2833.178424] #PF: error_code(0x0000) - not-present page
[ 2833.178776] PGD 21f4f9067 P4D 21f4f9067 PUD 21f4fa067 PMD 0
[ 2833.179182] Oops: 0000 [#3] SMP NOPTI
[ 2833.179464] CPU: 11 PID: 22867 Comm: kworker/11:0 Tainted: G      D W          6.8.0_io_uring_6.10+ #14
[ 2833.180110] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc37 04/01/2014
[ 2833.180692] Workqueue: events io_fallback_req_func
[ 2833.181042] RIP: 0010:percpu_counter_add_batch+0x19/0x80
[ 2833.181430] Code: 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 41 55 41 54 55 53 9c 58 0f 1f 40 00 49 89 c5 fa 0f 1f 44 00 00 <48> 8b 4f 20 48 63 d2 65 48 63 19 49 89 dc 48 01 f3 48 89 d8 48 f7
[ 2833.182623] RSP: 0018:ffffb89da07a7dd0 EFLAGS: 00010006
[ 2833.186362] RAX: 0000000000000206 RBX: ffff97d7d9332ec0 RCX: 0000000000000000
[ 2833.186825] RDX: 0000000000000020 RSI: ffffffffffffffff RDI: 00000000000000c8
[ 2833.187326] RBP: 0000000000000000 R08: 0000000000000246 R09: 0000000000020001
[ 2833.187783] R10: 0000000000020001 R11: 0000000000000032 R12: ffff97d7dee497f0
[ 2833.188284] R13: 0000000000000206 R14: ffff97d7400e9d00 R15: ffff97d6cd9cc410
[ 2833.188741] FS:  0000000000000000(0000) GS:ffff97dc27d80000(0000) knlGS:0000000000000000
[ 2833.189310] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2833.189709] CR2: 00000000000000e8 CR3: 0000000220b30004 CR4: 0000000000770ef0
[ 2833.190205] PKRU: 55555554
[ 2833.190418] Call Trace:
[ 2833.190615]  <TASK>
[ 2833.190795]  ? __die+0x23/0x70
[ 2833.191053]  ? page_fault_oops+0x173/0x4f0
[ 2833.191362]  ? exc_page_fault+0x76/0x150
[ 2833.191654]  ? asm_exc_page_fault+0x26/0x30
[ 2833.191968]  ? percpu_counter_add_batch+0x19/0x80
[ 2833.192313]  io_put_task_remote+0x2a/0x90
[ 2833.192594]  __io_submit_flush_completions+0x2bd/0x380
[ 2833.192944]  io_fallback_req_func+0xa3/0x130
[ 2833.193273]  process_one_work+0x181/0x350
[ 2833.193550]  worker_thread+0x27e/0x390
[ 2833.193813]  ? __pfx_worker_thread+0x10/0x10
[ 2833.194123]  kthread+0xbb/0xf0
[ 2833.194369]  ? __pfx_kthread+0x10/0x10
[ 2833.194638]  ret_from_fork+0x31/0x50
[ 2833.194899]  ? __pfx_kthread+0x10/0x10
[ 2833.195213]  ret_from_fork_asm+0x1a/0x30
[ 2833.195484]  </TASK>
[ 2833.195661] Modules linked in: isofs binfmt_misc vfat fat xfs raid0 iTCO_wdt intel_pmc_bxt iTCO_vendor_support i2c_i801 virtio_net i2c_smbus net_failover failover lpc_ich ublk_drv loop zram nvme nvme_core usb_storage crc32c_intel virtio_scsi virtio_blk fuse qemu_fw_cfg
[ 2833.197148] CR2: 00000000000000e8
[ 2833.197400] ---[ end trace 0000000000000000 ]---
[ 2833.197714] RIP: 0010:percpu_counter_add_batch+0x19/0x80
[ 2833.198078] Code: 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 41 55 41 54 55 53 9c 58 0f 1f 40 00 49 89 c5 fa 0f 1f 44 00 00 <48> 8b 4f 20 48 63 d2 65 48 63 19 49 89 dc 48 01 f3 48 89 d8 48 f7
[ 2833.199261] RSP: 0018:ffffb89d8b0f7dd0 EFLAGS: 00010006
[ 2833.199599] RAX: 0000000000000206 RBX: ffff97d77b830000 RCX: 0000000080020001
[ 2833.200051] RDX: 0000000000000020 RSI: ffffffffffffffff RDI: 00000000000000c8
[ 2833.200515] RBP: 0000000000000000 R08: ffff97d77b830000 R09: 0000000080020001
[ 2833.200956] R10: 0000000080020001 R11: 0000000000000016 R12: ffff97d75210c6c0
[ 2833.201439] R13: 0000000000000206 R14: ffff97d7518f3800 R15: ffff97d6c304bc10
[ 2833.201894] FS:  0000000000000000(0000) GS:ffff97dc27d80000(0000) knlGS:0000000000000000
[ 2833.202455] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2833.202832] CR2: 00000000000000e8 CR3: 0000000220b30004 CR4: 0000000000770ef0
[ 2833.203316] PKRU: 55555554
[ 2833.203524] note: kworker/11:0[22867] exited with irqs disabled

Thanks,
Ming

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 4 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

Quoted 2 lines

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

You may need to run this test multiple times for triggering the problem:

1) git clone https://github.com/ublk-org/ublksrv.git

2) cd ublksrv

3) make test T=generic/004

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

I'll repost it with the locking fix for reference, would make more sense retesting ublk after figuring out what's up with the branch.

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 2 lines

I'll repost it with the locking fix for reference, would make more sense retesting ublk after figuring out what's up with the branch.

Yep if you repost it with the fix, I'll rebase for-6.10/io_uring.

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Did you run with KASAN enabled

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 3 lines

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Quoted 49 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Been running on for-6.10/io_uring, and the only odd thing I see is that the test output tends to stall here:

Running test read-before-exit.t

which then either leads to a connection disconnect from my ssh into that vm, or just a long delay and then it picks up again. This did not happen with io_uring-6.9.

Maybe related? At least it's something new. Just checked again, and yeah it seems to totally lock up the vm while that is running. Will try a quick bisect of that series.

Quoted 67 lines · Jens Axboe

Quoted 49 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Been running on for-6.10/io_uring, and the only odd thing I see is that the test output tends to stall here:

Running test read-before-exit.t

which then either leads to a connection disconnect from my ssh into that vm, or just a long delay and then it picks up again. This did not happen with io_uring-6.9.

Quoted 3 lines

Maybe related? At least it's something new. Just checked again, and yeah it seems to totally lock up the vm while that is running. Will try a quick bisect of that series.

Seems to be triggered by the top of branch patch in there, my poll and timeout special casing. While the above test case runs with that commit, it'll freeze the host.

Quoted 68 lines · Jens Axboe

Quoted 67 lines · Jens Axboe

Quoted 49 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Been running on for-6.10/io_uring, and the only odd thing I see is that the test output tends to stall here:

Running test read-before-exit.t

which then either leads to a connection disconnect from my ssh into that vm, or just a long delay and then it picks up again. This did not happen with io_uring-6.9.

Quoted 7 lines

Quoted 3 lines

Maybe related? At least it's something new. Just checked again, and yeah it seems to totally lock up the vm while that is running. Will try a quick bisect of that series.

Seems to be triggered by the top of branch patch in there, my poll and timeout special casing. While the above test case runs with that commit, it'll freeze the host.

Had a feeling this was the busy looping off cancelations, and flushing the fallback task_work seems to fix it. I'll check more tomorrow.

io_uring/io_uring.c +0 -0

index a2cb8da3cc33..f1d3c5e065e9 100644

Quoted 86 lines · Jens Axboe

Quoted 68 lines · Jens Axboe

Quoted 67 lines · Jens Axboe

Quoted 49 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset
when running some ublk stress test(io vs. deletion). And not see
such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Been running on for-6.10/io_uring, and the only odd thing I see is that the test output tends to stall here:

Running test read-before-exit.t

which then either leads to a connection disconnect from my ssh into that vm, or just a long delay and then it picks up again. This did not happen with io_uring-6.9.

Quoted 7 lines

Quoted 3 lines

Maybe related? At least it's something new. Just checked again, and yeah it seems to totally lock up the vm while that is running. Will try a quick bisect of that series.

Seems to be triggered by the top of branch patch in there, my poll and timeout special casing. While the above test case runs with that commit, it'll freeze the host.

Had a feeling this was the busy looping off cancelations, and flushing the fallback task_work seems to fix it. I'll check more tomorrow.

io_uring/io_uring.c +0 -0

index a2cb8da3cc33..f1d3c5e065e9 100644

io_uring/io_uring.c static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx, · line 3248

Quoted 7 lines

io_uring/io_uring.c +2 -0

@@ -3242,6 +3242,8 @@ static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
 	ret |= io_kill_timeouts(ctx, task, cancel_all);
 	if (task)
 		ret |= io_run_task_work() > 0;
+	else if (ret)
+		flush_delayed_work(&ctx->fallback_work);
 	return ret;
 }

Still can trigger the warning with above patch:

[  446.275975] ------------[ cut here ]------------
[  446.276340] WARNING: CPU: 8 PID: 731 at kernel/fork.c:969 __put_task_struct+0x10c/0x180

Quoted 15 lines

io_uring/io_uring.c static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx, · line 3248

Quoted 12 lines

io_uring/io_uring.c +2 -0

index a2cb8da3cc33..f1d3c5e065e9 100644
@@ -3242,6 +3242,8 @@ static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
 	ret |= io_kill_timeouts(ctx, task, cancel_all);
 	if (task)
 		ret |= io_run_task_work() > 0;
+	else if (ret)
+		flush_delayed_work(&ctx->fallback_work);
 	return ret;
 }

Still can trigger the warning with above patch:

Quoted 2 lines

[  446.275975] ------------[ cut here ]------------
[  446.276340] WARNING: CPU: 8 PID: 731 at kernel/fork.c:969 __put_task_struct+0x10c/0x180

And this is running that test case you referenced? I'll take a look, as it seems related to the poll kill rather than the other patchset.

Quoted 97 lines · Ming Lei

Quoted 86 lines · Jens Axboe

Quoted 68 lines · Jens Axboe

Quoted 67 lines · Jens Axboe

Quoted 49 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

On 3/16/24 11:52, Ming Lei wrote:
On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

The following two error can be triggered with this patchset
when running some ublk stress test(io vs. deletion). And not see
such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I
reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Did you run with KASAN enabled

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Been running on for-6.10/io_uring, and the only odd thing I see is that the test output tends to stall here:

Running test read-before-exit.t

which then either leads to a connection disconnect from my ssh into that vm, or just a long delay and then it picks up again. This did not happen with io_uring-6.9.

Quoted 7 lines

Quoted 3 lines

Maybe related? At least it's something new. Just checked again, and yeah it seems to totally lock up the vm while that is running. Will try a quick bisect of that series.

Seems to be triggered by the top of branch patch in there, my poll and timeout special casing. While the above test case runs with that commit, it'll freeze the host.

Had a feeling this was the busy looping off cancelations, and flushing the fallback task_work seems to fix it. I'll check more tomorrow.

io_uring/io_uring.c +0 -0

index a2cb8da3cc33..f1d3c5e065e9 100644

io_uring/io_uring.c static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx, · line 3248

Quoted 7 lines

io_uring/io_uring.c +2 -0

@@ -3242,6 +3242,8 @@ static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
 	ret |= io_kill_timeouts(ctx, task, cancel_all);
 	if (task)
 		ret |= io_run_task_work() > 0;
+	else if (ret)
+		flush_delayed_work(&ctx->fallback_work);
 	return ret;
 }

Still can trigger the warning with above patch:

Quoted 2 lines

[  446.275975] ------------[ cut here ]------------
[  446.276340] WARNING: CPU: 8 PID: 731 at kernel/fork.c:969 __put_task_struct+0x10c/0x180

And this is running that test case you referenced? I'll take a look, as it seems related to the poll kill rather than the other patchset.

Quoted 51 lines · Pavel Begunkov

Quoted 50 lines · Pavel Begunkov

Quoted 48 lines · Jens Axboe

Quoted 32 lines · Pavel Begunkov

Quoted 26 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Ming Lei

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Ming Lei

On Fri, Mar 15, 2024 at 04:53:21PM -0600, Jens Axboe wrote:

...

Quoted 6 lines

Quoted 3 lines

The following two error can be triggered with this patchset when running some ublk stress test(io vs. deletion). And not see such failures after reverting the 11 patches.

I suppose it's with the fix from yesterday. How can I reproduce it, blktests?

Yeah, it needs yesterday's fix.

Quoted 8 lines

Quoted 1 line

You may need to run this test multiple times for triggering the problem:

Thanks for all the testing. I've tried it, all ublk/generic tests hang in userspace waiting for CQEs but no complaints from the kernel. However, it seems the branch is buggy even without my patches, I consistently (5-15 minutes of running in a slow VM) hit page underflow by running liburing tests. Not sure what is that yet, but might also be the reason.

Quoted 4 lines

Hmm odd, there's nothing in there but your series and then the io_uring-6.9 bits pulled in. Maybe it hit an unfortunate point in the merge window -git cycle? Does it happen with io_uring-6.9 as well? I haven't seen anything odd.

Quoted 7 lines

Need to test io_uring-6.9. I actually checked the branch twice, both with the issue, and by full recompilation and config prompts I assumed you pulled something in between (maybe not).

And yeah, I can't confirm it's specifically an io_uring bug, the stack trace is usually some unmap or task exit, sometimes it only shows when you try to shutdown the VM after tests.

Funky. I just ran a bunch of loops of liburing tests and Ming's ublksrv test case as well on io_uring-6.9 and it all worked fine. Trying liburing tests on for-6.10/io_uring as well now, but didn't see anything the other times I ran it. In any case, once you repost I'll rebase and then let's see if it hits again.

Did you run with KASAN enabled

Quoted 5 lines

Quoted 1 line

Yes, it's a debug kernel, full on KASANs, lockdeps and so

And another note, I triggered it once (IIRC on shutdown) with ublk tests only w/o liburing/tests, likely limits it to either the core io_uring infra or non-io_uring bugs.

Been running on for-6.10/io_uring, and the only odd thing I see is that the test output tends to stall here:

Quoted 38 lines · Ming Lei

Quoted 36 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Patch 1 is a fix.

Patches 2-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.

8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.

[...]

Applied, thanks!

[02/11] io_uring/cmd: kill one issue_flags to tw conversion

         commit: 31ab0342cf6434e1e2879d12f0526830ce97365d
[03/11] io_uring/cmd: fix tw <-> issue_flags conversion
         commit: b48f3e29b89055894b3f50c657658c325b5b49fd
[04/11] io_uring/cmd: introduce io_uring_cmd_complete
         commit: c5b4c92ca69215c0af17e4e9d8c84c8942f3257d
[05/11] ublk: don't hard code IO_URING_F_UNLOCKED
         commit: c54cfb81fe1774231fca952eff928389bfc3b2e3
[06/11] nvme/io_uring: don't hard code IO_URING_F_UNLOCKED
         commit: 800a90681f3c3383660a8e3e2d279e0f056afaee
[07/11] io_uring/rw: avoid punting to io-wq directly
         commit: 56d565d54373c17b7620fc605c899c41968e48d0
[08/11] io_uring: force tw ctx locking
         commit: f087cdd065af0418ffc8a9ed39eadc93347efdd5
[09/11] io_uring: remove struct io_tw_state::locked
         commit: 339f8d66e996ec52b47221448ff4b3534cc9a58d
[10/11] io_uring: refactor io_fill_cqe_req_aux
         commit: 7b31c3964b769a6a16c4e414baa8094b441e498e
[11/11] io_uring: get rid of intermediate aux cqe caches
         commit: 5a475a1f47412a44ed184aac04b9ff0aeaa31d65

Quoted 1 line

Hi Jens and Pavel,

Jens, I hope you already dropped the series for now, right?