Repository navigation
Conversation
| // commits than completed entries, so submitting must | ||
| // eventually make room. | ||
| ring.submit()?; | ||
| if let Err(e) = ring.submit() { |
There was a problem hiding this comment.
nit: submit_and_wait(1) now tolerates EINTR, but both plain ring.submit() sites still stop the worker on any errno. A submit-only io_uring_enter never sleeps, so EINTR isn't reachable here today — but the policy now differs across three sites and rests on an unstated crate/kernel property, while the PR text claims "an interrupted wait no longer stops the worker at all". Matching the EINTR arm at both sites would make that claim unconditional.
There was a problem hiding this comment.
Done. Every io_uring_enter() now goes through submit() or submit_and_wait(), and both treat EINTR as no error. I also dropped the submit at the end of the serve loop, because the wait at the top of the loop submits the queued commits. The description now says that a signal doesn't stop the worker, whether it was waiting or only submitting.
|
Please help to rebase this PR：） |
| e.raw_os_error(), | ||
| Some(libc::ETIME | libc::EINTR | libc::EBUSY | libc::EAGAIN) | ||
| ) => {} | ||
| Err(_) => break, |
There was a problem hiding this comment.
How about adding a logging message here?
Err(e) => {
warn!("uring worker: waiting for entries failed: {e}");
break;
}
There was a problem hiding this comment.
Added, with your message.
A uring worker freed its ring entries as soon as it stopped. When it stopped on an error while the connection was still up, such as a failed command or an interrupted io_uring_enter(), its other entries were still registered, and the kernel could copy the next request into freed memory. Closing the ring doesn't help in time, because the kernel cancels the commands later, from a workqueue. The worker now counts the entries whose command the kernel hasn't completed, and waits for those completions before it frees them. Once the connection is torn down they arrive at once. If they don't arrive within a second, the worker leaks the entries instead of freeing memory the kernel may still write to. A signal that interrupts io_uring_enter() no longer stops the worker, whether it was waiting or only submitting. If the wait in the drain fails with another error, the worker logs that error. The new mount test serves a PassthroughFs over io_uring and checks that dropping the serving layer still returns well within that second. It can't reproduce the race itself. Signed-off-by: Ilan Joselevich <personal@ilanjoselevich.com>
5d04f81 to
c4efc19
Compare
A FUSE-over-io_uring worker freed its ring entries as soon as it stopped. When it stopped on an error while the connection was still up, such as a failed command or an
io_uring_enter()that a signal interrupted, its other entries were still registered with the kernel, which could copy the next request into the freed memory. Closing the ring doesn't prevent that in time, because the kernel cancels the commands later, from a workqueue.The worker now counts the entries whose command the kernel hasn't completed and waits for those completions before it frees the entries. Once the connection is torn down they arrive right away. If they don't arrive within a second, the worker leaks the entries rather than free memory the kernel may still write to. A signal that interrupts
io_uring_enter()no longer stops the worker, whether it was waiting or only submitting. If the drain's wait fails with another error, the worker logs that error and leaks the entries.The new mount test serves a
PassthroughFsover io_uring and checks that dropping the serving layer still returns well within that second. It can't reproduce the race itself.Two problems are left for #284, because fixing them means ending the connection when a worker fails. A worker that stops on an error while nothing else is stopping leaves its queues without entries, so later requests from those CPUs hang. And a request already delivered in the same batch as the failing completion gets no reply.
The test suite passes in a user namespace on Linux 7.3-rc7 on both async runtimes, apart from the 14 passthrough tests that need real root and also fail on master.