fix(rdma): stage device memory through the host in the HTTP fallbacks - #239
Conversation
Client::GetObject and Client::PutObject attempt RDMA and, when the server declines or retries are exhausted, fall through to
an HTTP path that does
ss.rdbuf()->pubsetbuf(args.buf, size);
plus stream reads/writes. Those are ordinary host loads and stores. With a GPU-Direct caller args.buf is a CUDA device
pointer, so the fallback faults: SIGSEGV with sigcode 2 (SEGV_ACCERR) at exactly the buffer address, from inside
miniocpp_get_object/miniocpp_put_object. Concurrency only controls how often a transfer declines and therefore how often the
fallback is reached.
Stage through a host buffer when the caller's memory is not CUDA_MEMORY_SYSTEM: GET streams the body into host memory and
copies it up afterwards, PUT copies down before the upload reads it. The two driver-API copies are resolved with dlopen so
libminiocpp gains no link-time CUDA dependency — readelf shows the same NEEDED set as before, and a host without the driver
gets a clear error instead of a crash.
Measured against a 4-node AIStor cluster where RDMA PUT declines with IBV_WC_REM_OP_ERR (remote RDMA READ out of GPU VRAM is
unsupported on that platform): warp put --rdma=gpu went from every operation failing to 512 MiB/s over the fallback.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesCUDA-aware HTTP fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.cc (1)
627-664: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap the device-buffer staging allocations so an OOM reports an error instead of throwing uncaught. Both fallback paths size a
std::vector<char>to the full transfer size with no exception handling, unlike the rest of the file's allocation code which reports failure via return codes.
src/client.cc#L627-L664: guardstage.resize(size)in the GET fallback (Line 641) with a try/catch (or anothrow-checked allocation) and returnerror::make<GetObjectResponse>(...)on failure instead of lettingstd::bad_allocpropagate.src/client.cc#L1140-L1164: apply the same guard tostage.resize(size)in the PUT fallback (Line 1153), returningerror::make<PutObjectResponse>(...)on failure.🤖 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/client.cc` around lines 627 - 664, Guard the staging allocation in the GET fallback around stage.resize(size) within the device_buf path, catching allocation failure and returning an error::make<GetObjectResponse> result instead of propagating std::bad_alloc. Apply the same allocation-failure handling to stage.resize(size) in the PUT fallback, returning error::make<PutObjectResponse>; both affected sites are in src/client.cc ranges 627-664 and 1140-1164.
🤖 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/client.cc`:
- Around line 96-132: Ensure the RDMA fallback device-buffer copy path
establishes a valid CUDA context on the std::async worker before invoking the
functions resolved by GetCudaHostCopy. Update Client::GetObjectAsync and
Client::PutObjectAsync to retain and set the originating device context for
their staging copies, or explicitly detect and reject device buffers without a
current context while preserving the existing host-buffer behavior.
---
Outside diff comments:
In `@src/client.cc`:
- Around line 627-664: Guard the staging allocation in the GET fallback around
stage.resize(size) within the device_buf path, catching allocation failure and
returning an error::make<GetObjectResponse> result instead of propagating
std::bad_alloc. Apply the same allocation-failure handling to stage.resize(size)
in the PUT fallback, returning error::make<PutObjectResponse>; both affected
sites are in src/client.cc ranges 627-664 and 1140-1164.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80f728f7-1af3-4a39-878b-07fa1d638437
📒 Files selected for processing (1)
src/client.cc
The driver-API copies act on the calling thread's current context. GetObjectAsync/PutObjectAsync dispatch through std::async, so a fallback can run on a pool thread that never touched CUDA, where every copy fails with CUDA_ERROR_INVALID_CONTEXT. Borrow the primary context of the device the buffer belongs to for the duration of the copy and restore the thread afterwards; a thread that already has a context is left untouched.
Fixes Applied SuccessfullyFixed 1 file based on 1 CodeRabbit feedback item. Files modified:
Commit: Context-protect the RDMA fallback copies before async dispatchValid, and I had a live symptom for it: every Rather than documenting a caller constraint, the library now makes itself safe: both copies run under a Measured on the same 4-node cluster, where RDMA PUT always declines with
The latest autofix changes are on the |
check-style.sh runs clang-format-20 --style=Google; the two reinterpret_cast lines wrapped after the '=' rather than before it. No behaviour change.
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 `@src/client.cc`:
- Line 193: Replace the -1 sentinel used by retained_ordinal_ with
std::optional<int>, using an empty optional to represent “nothing retained.”
Update all reads, comparisons, assignments, and resets of retained_ordinal_ in
the surrounding client implementation to use optional presence and value access
while preserving the existing retained-ordinal behavior.
- Around line 183-188: Update the ScopedCudaContext destructor to check the
return codes from ctx_set(nullptr) and ctx_release(retained_ordinal_), logging
warnings for failures consistent with ScopedRDMARegistration::Release().
Preserve the existing cleanup order and only release when retained_ordinal_ is
valid.
- Around line 157-195: Update ScopedCudaContext so a null current context is
bootstrapped before calling fns_.ptr_attr: obtain and set a usable device
context first, then query the pointer’s device ordinal and switch to the correct
context when necessary. Track whether the class owns a retained context
separately from retained_ordinal_ so the no-ownership path never relies on
retaining or releasing ordinal -1, while preserving cleanup in the destructor.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 982d2bcf-c502-45ae-b360-02680363199d
📒 Files selected for processing (1)
src/client.cc
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/client.cc (3)
716-718: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplace the implementation-defined
stringstream::pubsetbuf()staging contract.
basic_stringbuf::setbuf()effects are implementation-defined, so this fallback is non-portable; MSVC’s default does not makestringstreamwrite into the user-supplied buffer. Use a bound check + explicit chunk copy for the GET fallback, and construct an input stream from the staged host data (or use an explicit span-backed buffer) for the PUT fallback.🤖 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/client.cc` around lines 716 - 718, Replace the implementation-defined stringstream::pubsetbuf staging in both GET and PUT fallback paths in src/client.cc:716-718 and src/client.cc:1245-1246. For GET, validate the requested size and explicitly copy chunks into the caller-provided buffer; for PUT, construct the input stream from the staged host data or use an explicit span-backed buffer. Do not rely on basic_stringbuf::setbuf behavior.
137-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoad the unsuffixed primary-context release symbol as a fallback.
GetCudaHostCopy().Ok()returns false whencuDevicePrimaryCtxRelease_v2is missing, even though the driver may support all the other loaded symbols. TrycuDevicePrimaryCtxRelease_v2first, then fall back tocuDevicePrimaryCtxRelease, or document/enforce the minimum CUDA driver version that requires_v2.🤖 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/client.cc` around lines 137 - 142, Update the CUDA symbol initialization in GetCudaHostCopy() to load cuDevicePrimaryCtxRelease_v2 first and fall back to cuDevicePrimaryCtxRelease when unavailable, so CudaHostCopy::Ok() remains true with older compatible drivers. Keep the existing function-pointer type and other symbol loading unchanged.
160-165: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftValidate that an existing context supports the device copy.
When
cuCtxGetCurrent()returns a non-null context, the thread already has a current CUDA context, but it is not checked against the allocation/device context. For multi-GPU callers, ensure that existing current contexts can cover thedevptrallocation; otherwise the Driver API copy may use an inappropriate context. Check the pointer’s owning context/device, or document that copies are only supported when the current context is compatible.🤖 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/client.cc` around lines 160 - 165, Update the existing-context branch in the relevant initialization method around fns.ctx_get and devptr so it verifies that the current CUDA context is compatible with the allocation’s owning context/device before setting ok_ and returning. Reject or otherwise handle incompatible contexts rather than treating every non-null current context as valid; preserve the existing success path for compatible contexts.
🤖 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.
Outside diff comments:
In `@src/client.cc`:
- Around line 716-718: Replace the implementation-defined
stringstream::pubsetbuf staging in both GET and PUT fallback paths in
src/client.cc:716-718 and src/client.cc:1245-1246. For GET, validate the
requested size and explicitly copy chunks into the caller-provided buffer; for
PUT, construct the input stream from the staged host data or use an explicit
span-backed buffer. Do not rely on basic_stringbuf::setbuf behavior.
- Around line 137-142: Update the CUDA symbol initialization in
GetCudaHostCopy() to load cuDevicePrimaryCtxRelease_v2 first and fall back to
cuDevicePrimaryCtxRelease when unavailable, so CudaHostCopy::Ok() remains true
with older compatible drivers. Keep the existing function-pointer type and other
symbol loading unchanged.
- Around line 160-165: Update the existing-context branch in the relevant
initialization method around fns.ctx_get and devptr so it verifies that the
current CUDA context is compatible with the allocation’s owning context/device
before setting ok_ and returning. Reject or otherwise handle incompatible
contexts rather than treating every non-null current context as valid; preserve
the existing success path for compatible contexts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15a39408-5f08-4b7c-9d9f-c8c57f2829da
📒 Files selected for processing (1)
src/client.cc
cuPointerGetAttribute can itself require a current context, so querying the buffer's device ordinal before establishing one could fail on a fresh worker thread and report 'unable to establish a CUDA context' instead of using the primary context. Borrow device 0's primary context to make the query legal, then move to the buffer's own device when it differs. Also track ownership with std::optional<int> rather than a -1 sentinel, and log ctx_set/ctx_release failures during teardown the way ScopedRDMARegistration already does, so a leaked primary-context reference leaves a trail.
Fixes Applied SuccessfullyFixed 1 file based on 3 CodeRabbit feedback items (all three inline threads). Files modified:
Commit: 🔴 Bootstrap a context before querying
|
| result | |
|---|---|
put --rdma=gpu |
1603 MiB/s, 0 errors, 0 warnings |
server-side rdma_write_ops delta |
1047 — every transfer took the RDMA path, none fell back |
get --rdma=gpu |
7513 MiB/s (unchanged) |
check-style.sh |
exit 0 |
Worth noting for context: the RDMA PUT declines that made this fallback hot turned out to be PCIe ACS Redirect left
enabled on the switch above that GPU, not a platform limitation. With ACS cleared these PUTs now go over RDMA directly, so
this guard is back to being the safety net it was meant to be.
1 participant
Problem
Client::GetObjectandClient::PutObjectattempt RDMA and, when the server declines or retries are exhausted, fall throughto an HTTP path that does:
Those are ordinary host loads and stores. With a GPU-Direct caller
args.bufis a CUDA device pointer, so the fallbackfaults:
sigcode=2isSEGV_ACCERR, at exactly the buffer address. Concurrency is not the cause — it only controls how often atransfer declines and therefore how often the fallback is reached. The same file already guards host access elsewhere with
cuObjClient::getMemoryType(buf) == CUOBJ_MEMORY_SYSTEM; the two fallbacks just never got that treatment.Fix
Stage through a host buffer whenever the caller's memory is not
CUOBJ_MEMORY_SYSTEM: GET streams the body into host memoryand copies it up afterwards, PUT copies down before the upload reads it.
The two driver-API copies are resolved with
dlopen("libcuda.so.1")rather than linked, so libminiocpp gains nolink-time CUDA dependency —
readelf -dshows the sameNEEDEDset as before:A host without the driver gets a clear error instead of a crash.
Testing
Built with
-DMINIO_CPP_ENABLE_RDMA=ONagainst a 4-node AIStor cluster over RoCE where RDMA PUT declines withIBV_WC_REM_OP_ERR(remote RDMA READ out of GPU VRAM is unsupported on that platform, so the fallback is always taken):warp put --rdma=gpuwarp get --rdma=gpuVerified the dlopen path is actually wired (
stringsshowslibcuda.so.1,cuMemcpyDtoH_v2,cuMemcpyHtoD_v2) and thatCPU-buffer callers are unaffected —
getMemoryTypeshort-circuits them to the original code path.One residual error per run remains, but it is caller-side rather than here:
cuMemcpyDtoHneeds a current CUDA context onthe calling thread, and the Go caller does not yet pin its worker to one OS thread on the PUT path (fixed separately in
minio/warp#499 for GET).
Summary by CodeRabbit