fix(posix): recover io_uring after I/O failures#1942
Conversation
|
👋 Hi lluki! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
lluki
force-pushed
the
enospc-uring-fix
branch
2 times, most recently
from
f267f5f to
a470f68
Compare
lluki
requested review from
a team,
brminich,
ofer,
vvenkates27 and
w1ldptr
as code owners
|
/build |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe POSIX I/O queues now expose cancellation and pending-cleanup status. The io_uring implementation separates submission, cancellation, and completion draining, while backend requests propagate failures until cleanup finishes. Tests and CI exercise io_uring submission failures, partial submissions, and repost behavior. POSIX I/O lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant nixlPosixBackendReqH
participant nixlPosixIOQueueUring
participant io_uring
nixlPosixBackendReqH->>nixlPosixIOQueueUring: post or poll transfer
nixlPosixIOQueueUring->>io_uring: prepare and submit SQEs
io_uring-->>nixlPosixIOQueueUring: completion CQEs
nixlPosixBackendReqH->>nixlPosixIOQueueUring: cancel(ctx)
nixlPosixIOQueueUring->>io_uring: submit cancellation SQEs
io_uring-->>nixlPosixIOQueueUring: cancellation CQEs
nixlPosixIOQueueUring-->>nixlPosixBackendReqH: report final cleanup status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/plugins/posix/nixl_posix_test.cpp`:
- Line 928: Update the local rlimit variable in the test to be const-qualified,
preserving its existing initialization and use when passed to setrlimit.
- Around line 93-108: Add a null check immediately after
`nixlPosixIOQueue::instantiate` in `UringTest` and fail cleanly if it returns
null, using the test’s existing exception/error-handling convention before any
queue operations occur.
- Around line 916-943: Update the SIGXFSZ handling in the use_uring branch to
save the previous handler returned by signal(SIGXFSZ, SIG_IGN), then restore
that handler after the transfer status check alongside restoring the saved
RLIMIT_FSIZE. Preserve the existing error and short-write behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f8ad2e16-7983-4b6a-9918-aebbde1a474a
📒 Files selected for processing (6)
src/plugins/posix/io_queue.hsrc/plugins/posix/io_uring_io_queue.cppsrc/plugins/posix/posix_backend.cppsrc/plugins/posix/posix_backend.htest/unit/plugins/posix/meson.buildtest/unit/plugins/posix/nixl_posix_test.cpp
|
🤖 CI Triage Agent — TL;DR: Both sanitizer matrix jobs (tsan and asan_ubsan) fail at the Full analysisSummary: The Root cause:
Implicated commit: No NIXL code change caused this. The stage was introduced/reshaped by NirWolfer in the sanitizer-CI series ([REDACTED:Hex High Entropy String] "ci: add AddressSanitizer/UBSanitizer/ThreadSanitizer builds" #1709; e0053e8 #1874). The actual trigger is the sanitizer container's memlock limit, not a source commit. File: Suggested fix: On the sanitizer CI runners, raise the locked-memory limit for the test container — e.g. run with Related: PR #1709 (add sanitizer builds), PR #1874 (sanitizer stage tuning), PR #1605 (POSIX backend queue fallback handling), PR #1577 (liburing wrap).
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: PR #1942 ("fix(posix): recover io_uring after I/O failures") adds a new io_uring runtime probe. In the CI container (kernel Implicated commit: PR #1942 by File: Suggested fix: Distinguish "io_uring compiled in" from "io_uring usable at runtime." When the runtime probe ( Related: PR #1942 (#1942); references prior work PR #1697 and POSIX queue-fallback PR #1605.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/posix/io_queue.h`:
- Around line 55-64: Document the public virtual APIs cancel and
hasPendingCleanup in io_queue.h with Doxygen-style block comments. Describe
cancel’s cancellation scope, asynchronous cleanup behavior, ctx ownership, and
possible return values; describe hasPendingCleanup’s cleanup-state meaning and
boolean result. Keep the existing implementations unchanged.
In `@src/plugins/posix/io_uring_io_queue.cpp`:
- Around line 60-63: Update hasPendingCleanup() to report pending cleanup when
either submission_failure_draining_ or cancel_sqes_outstanding_ is active.
Preserve the existing const override and ensure cancellation CQEs keep the
backend in cleanup until they are drained.
In `@test/unit/plugins/posix/nixl_posix_test.cpp`:
- Around line 921-924: Bound the polling loop following agent.postXferReq in the
short-write recovery test: limit iterations or enforce a deadline while status
remains NIXL_IN_PROG, then report a test failure when the bound is exceeded.
Preserve the existing getXferStatus polling and success behavior when completion
occurs within the limit.
- Around line 185-190: Update runUringSubmissionTests() and its main() handling
so io_uring initialization errors indicating unavailable support (-ENOMEM,
-EPERM, or -ENOSYS) propagate a distinct skip/fallback result rather than
failure. Reserve the existing failure result for unexpected probe errors,
allowing main() to continue to Linux AIO or skip the io_uring tests.
- Line 59: Rename the Buffers type alias to the repository-compliant lower_case
name with a _t suffix, and update every reference to the alias in the
surrounding test code accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 282eca62-1143-4fe0-8f66-6165582a820e
📒 Files selected for processing (9)
.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.gitlab/test_cpp.shsrc/plugins/posix/io_queue.hsrc/plugins/posix/io_uring_io_queue.cppsrc/plugins/posix/posix_backend.cppsrc/plugins/posix/posix_backend.htest/unit/plugins/posix/meson.buildtest/unit/plugins/posix/nixl_posix_test.cpp
lluki
force-pushed
the
enospc-uring-fix
branch
3 times, most recently
from
f3962ea to
f6166ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/posix/io_uring_io_queue.cpp`:
- Around line 136-138: Update the cancellation flow around the io_uring
submission logic and driveSubmissions() to retain cancellation contexts when
io_uring_get_sqe() returns null, then retry those contexts until every matching
in-flight I/O is canceled or completed. Ensure hasPendingCleanup() reports
unprepared cancellation work, and keep cancellation latched while deferred
retries are pending.
In `@src/plugins/posix/posix_backend.cpp`:
- Around line 167-179: Update nixlPosixBackendReqH::queueResult so
NIXL_ERR_NOT_SUPPORTED from requestCancellationOnce() is not returned as a
terminal result while callbacks or outstanding I/Os still require progress.
Continue polling via needsQueueProgress() first, and only return a terminal
backend or queue result once no callbacks remain, preserving existing failure
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 095de234-0c79-4000-bb06-eff53096d224
📒 Files selected for processing (9)
.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.gitlab/test_cpp.shsrc/plugins/posix/io_queue.hsrc/plugins/posix/io_uring_io_queue.cppsrc/plugins/posix/posix_backend.cppsrc/plugins/posix/posix_backend.htest/unit/plugins/posix/meson.buildtest/unit/plugins/posix/nixl_posix_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/plugins/posix/posix_backend.cpp (1)
172-174:⚠️ Potential issue | 🟠 MajorStill keep polling when cancellation is unsupported but callbacks remain.
requestCancellationOnce()can returnNIXL_ERR_NOT_SUPPORTED, but this branch returns a terminal status before checkingneedsQueueProgress(). If callbacks are still outstanding, the request may be destroyed whileioDoneClb()still referencesthis, causing a use-after-free. ReturnNIXL_IN_PROGuntil all I/Os are accounted for.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/posix/posix_backend.cpp` around lines 172 - 174, Update the requestCancellationOnce() unsupported path to check needsQueueProgress() before returning a terminal status. Return NIXL_IN_PROG while callbacks or outstanding I/O remain, and only preserve the existing queue_result/NIXL_ERR_BACKEND result after all I/Os are accounted for.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/posix/io_uring_io_queue.cpp`:
- Around line 198-205: Update the fatal-error path after io_uring_submit in the
submission logic to retire and recreate the io_uring instance instead of calling
beginSubmissionFailureDrain(). Ensure submission_failure_draining_ cannot remain
stuck and return the existing NIXL_ERR_BACKEND result after the ring replacement
attempt.
In `@src/plugins/posix/posix_backend.h`:
- Around line 42-63: Reorder the class declaration containing allIOsAccountedFor
and needsQueueProgress to follow the required layout: place the public section
first, then the private section, with private methods before private data
members. Keep the existing API, fields, and helper implementations unchanged
aside from their declaration order.
---
Duplicate comments:
In `@src/plugins/posix/posix_backend.cpp`:
- Around line 172-174: Update the requestCancellationOnce() unsupported path to
check needsQueueProgress() before returning a terminal status. Return
NIXL_IN_PROG while callbacks or outstanding I/O remain, and only preserve the
existing queue_result/NIXL_ERR_BACKEND result after all I/Os are accounted for.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0329e8c6-7cc6-4ccb-a5ad-d3614b753785
📒 Files selected for processing (9)
.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.gitlab/test_cpp.shsrc/plugins/posix/io_queue.hsrc/plugins/posix/io_uring_io_queue.cppsrc/plugins/posix/posix_backend.cppsrc/plugins/posix/posix_backend.htest/unit/plugins/posix/meson.buildtest/unit/plugins/posix/nixl_posix_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/posix/io_uring_io_queue.cpp`:
- Around line 84-85: Rename the private members in
src/plugins/posix/io_uring_io_queue.cpp lines 84-85 to
submissionFailureDraining_ and cancelSqesOutstanding_, updating all references;
also rename the corresponding fields and usages in
src/plugins/posix/posix_backend.cpp lines 133-134 to transferFailed_ and
cancellationRequested_ to satisfy the required camelBack trailing-underscore
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7864ef7d-0962-4389-8866-5010589329ed
📒 Files selected for processing (9)
.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.gitlab/test_cpp.shsrc/plugins/posix/io_queue.hsrc/plugins/posix/io_uring_io_queue.cppsrc/plugins/posix/posix_backend.cppsrc/plugins/posix/posix_backend.htest/unit/plugins/posix/meson.buildtest/unit/plugins/posix/nixl_posix_test.cpp
|
/ok to test 9a7009e |
|
/build |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" gtest stage failed because UCX could not open the RDMA device ( Full analysisSummary: Both Root cause: Two coupled environment problems on GPU node
These are symptoms of a broken test image/runtime environment (missing Implicated commit: unknown (no code commit implicated; this is an environment/image regression on node File: Suggested fix:
Related: none found. |
2 participants
What?
Fix
io_uringsubmission failures so partial submissions remain in progress and are resubmitted, while complete submission failures cancel and drain all outstanding requests. This is a uring focused version of PR #1697, I'll send follow up PR's for the other POSIX modes.Why?
A single failed I/O or a single failed
io_uring_submit()could leave requests queued indefinitely and disallow the queue to be reused.How?
Posix Backend changes
Changes to the generic POSIX part that interfaces with a
io_engine(aio, uring, ...).nixlPosixBackendReqHenqueues transfers with completion callbacks, then drives progress throughio_queue::post()andpoll(). If a transfer fails,nixlPosixBackendReqHcalls the newly addedcancel(ctx)once to request cancellation of that transfer’s remaining I/Os. Even after all callbacks arrive,hasPendingCleanup()may require continued polling while the queue finishes internal cleanup.io_uringimplements both functions because cancellation SQEs and their CQEs must be drained before queue reuse. AIO behavior remains unchanged because its defaultcancel()is unsupported andhasPendingCleanup()is false, so failures retain their previous immediate-return semantics.IO_URING changes
There are two failure modes: A failed I/O will cause all the I/Os of that batch to be cancelled, then failure is communicated to the submitter of that batch. If the global
io_uring_submit()fails with a permanent failure, all current xfers are cancelled and become failing. This is because the submission ring contains SQEs from different requests. Transient failures report IN_PROG, requesting to be polled again.Scope of this PR
It is deliberately focused on
io_uring. AIO will be treated in a follow up PR, also user invoked cancellation is out of scope (#1955)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
--enable-uring(and re-enabled the POSIX test).