feat(s3): add optional S3-over-RDMA (cuObject) data plane#87
feat(s3): add optional S3-over-RDMA (cuObject) data plane#87harshavardhana wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughAdds optional NVIDIA cuObject S3-over-RDMA support with native Rust/C++ bindings, Python transfer orchestration, a registered storage provider, schema and unit-test coverage, and an executable roundtrip example. ChangesS3-over-RDMA data plane
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant S3CuObjectStorageProvider
participant CuObjEngine
participant multistorageclient_rust
participant S3Endpoint
S3CuObjectStorageProvider->>CuObjEngine: register buffer and request token
CuObjEngine->>multistorageclient_rust: call cuObject token API
multistorageclient_rust-->>CuObjEngine: return RDMA token
CuObjEngine->>S3Endpoint: sign request with x-amz-rdma-token
S3CuObjectStorageProvider->>S3Endpoint: send empty-body S3 request
S3Endpoint-->>CuObjEngine: return x-amz-rdma-reply
CuObjEngine->>multistorageclient_rust: release token and deregister buffer
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 72f8703 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
multi-storage-client/src/multistorageclient/providers/_cuobj.py (1)
222-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required public-method docstring.
As per coding guidelines, “All public Python functions must have docstrings (Sphinx autodoc extracts them).”
🤖 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 `@multi-storage-client/src/multistorageclient/providers/_cuobj.py` at line 222, Add a Sphinx-compatible docstring directly to the public install_hooks method, documenting its purpose and s3_client parameter; leave the method’s behavior unchanged.Source: Coding guidelines
🤖 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 `@multi-storage-client/src/multistorageclient/providers/_cuobj.py`:
- Around line 223-225: The RDMA token hook is not registered for multipart
uploads. In multi-storage-client/src/multistorageclient/providers/_cuobj.py
lines 223-225, update install_hooks to register self._inject_token for
before-sign.s3.UploadPart alongside the existing PutObject and GetObject hooks;
verify the related handling in
multi-storage-client/src/multistorageclient/providers/s3.py lines 680-688
requires no direct change, and extend the multipart RDMA coverage in
multi-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
lines 195-221 to assert UploadPart receives the x-amz-rdma-token header.
- Around line 180-188: The buffer sizing logic in _buffer_address and the
corresponding RDMA registration path must use the full byte size of writable
memoryviews. Replace len(buffer) with memoryview(buffer).nbytes in both
locations, and add a regression test covering a writable multi-byte-format
memoryview to verify the complete region is registered/tokenized.
In `@multi-storage-client/src/multistorageclient/providers/cuobj_shim.cpp`:
- Around line 106-120: Keep tokenRegistry() entries available until
getClient()->cuMemObjPutRDMAToken succeeds in cuobj_put_rdma_token; only erase
the entry after successful release so failures remain retryable. In
multi-storage-client/src/multistorageclient/providers/_cuobj.py lines 268-271,
place deregister_buffer(addr) in its own finally block so it executes even when
put_rdma_token(token) raises.
In `@multi-storage-client/src/multistorageclient/providers/s3.py`:
- Around line 275-277: Reject non-positive values when initializing
_rdma_multipart_chunksize in
multi-storage-client/src/multistorageclient/providers/s3.py:275-277, and
constrain rdma.multipart_chunksize to a positive integer in
multi-storage-client/src/multistorageclient/schema.py:119-119. In
multi-storage-client/src/multistorageclient/providers/s3.py:671-672, retain the
multipart loop only under the guarantee that n is greater than zero.
- Around line 589-604: Update the full-object path in the surrounding read
method and _invoke_api so the metadata lookup and allocation are bound to the
same object version: capture the metadata identity/version, pass the
corresponding conditional version constraint to get_object, and retry the
metadata/allocation flow or fail cleanly when the GET detects a mismatch.
Preserve ranged-read behavior unless it also relies on the same metadata. Add a
unit test that replaces the object between metadata lookup and GET and verifies
the mismatch is retried or reported without returning a partially populated
buffer.
In
`@multi-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py`:
- Around line 195-221: Update the RDMA multipart upload setup used by
test_rdma_upload_multipart_splits_and_completes and the corresponding
CuObjEngine.install_hooks configuration to register the UploadPart operation
alongside PutObject and GetObject, ensuring multipart upload_part calls receive
RDMA token injection.
---
Nitpick comments:
In `@multi-storage-client/src/multistorageclient/providers/_cuobj.py`:
- Line 222: Add a Sphinx-compatible docstring directly to the public
install_hooks method, documenting its purpose and s3_client parameter; leave the
method’s behavior unchanged.
🪄 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: CHILL
Plan: Enterprise
Run ID: b5947bcd-0b89-4c5f-b2d1-945173ef4304
📒 Files selected for processing (7)
.release_notes/.unreleased.mdmulti-storage-client/examples/rdma_roundtrip.pymulti-storage-client/src/multistorageclient/providers/_cuobj.pymulti-storage-client/src/multistorageclient/providers/cuobj_shim.cppmulti-storage-client/src/multistorageclient/providers/s3.pymulti-storage-client/src/multistorageclient/schema.pymulti-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
Add an `rdma` option to the `s3` storage provider that routes object transfers through NVIDIA cuObject (libcuobjclient) instead of the HTTP body. When enabled, a contiguous host buffer is registered with cuObject and its RDMA descriptor is carried to the endpoint as the signed `x-amz-rdma-token` header; the RDMA-capable endpoint transfers the payload directly into or out of the buffer, leaving the HTTP body empty. This offloads the bulk transfer from the CPU and the HTTP/TLS path. The option mirrors the existing `rust_client` sub-option: it swaps only the data plane and is mutually exclusive with it. Enabling RDMA forces the empty-body wire contract (unsigned payload, checksums only when_required) and single-shot put/get, since multipart does not apply to a single registered-buffer transfer. cuObject is a C++ library, so a thin extern "C" shim (providers/cuobj_shim.cpp) wraps a process-wide cuObjClient and is loaded over ctypes (providers/_cuobj.py); the module is import-safe without the native library present, mirroring the torch.cuda.cuobj / BotoCuObjClient split in the PyTorch checkpoint backend. Test Plan: - Unit (native engine mocked), run with the boto3 extra: `uv run --extra boto3 pytest tests/test_multistorageclient/unit/providers/test_s3_rdma.py` - Regression on existing S3/schema unit tests: passing. - End-to-end against a live RDMA endpoint: `python examples/rdma_roundtrip.py` (see the script header for the required cuObject runtime and environment).
Two refinements to the S3-over-RDMA data plane: Reuse the caller's buffer for an RDMA PUT whenever it is writable (gating on `memoryview(body).readonly` instead of an `isinstance` bytearray check), so writable memoryviews are registered in place and only a genuinely read-only body (e.g. immutable `bytes`) is copied. Compute a CRC64NVME checksum of the payload on the client and send it as `x-amz-checksum-crc64nvme`. An RDMA-capable endpoint validates it against the bytes delivered over RDMA; an endpoint that ignores the RDMA token validates it against the empty HTTP body and rejects the upload with a checksum mismatch, instead of silently storing a 0-byte object. CRC64NVME is hardware-accelerated and computed via awscrt. Test Plan: uv run --extra boto3 pytest tests/test_multistorageclient/unit/providers/test_s3_rdma.py Verified the empty-body/RDMA-reader/checksum ordering against the MinIO AIStor PutObjectHandler (rr.Reader feeds the hash reader on the RDMA path; r.Body feeds it otherwise).
Split RDMA uploads larger than the part size (default 512 MiB, `rdma.multipart_chunksize`) into an RDMA multipart upload instead of a single PutObject. Each part is transferred as its own registered cuObject buffer with an empty HTTP body, its RDMA token, and a per-part CRC64NVME that the endpoint validates against the RDMA-delivered bytes, so the 0-byte-save guard holds per part. Parts are aborted on any failure. This is required past the single-PutObject size limit (5 GiB) that AI checkpoint and model shards routinely exceed, bounds the size of any one registered buffer / RDMA transfer, and lets AIStor spread parts across nodes. Small uploads and every GET still take the single-shot path. Verified the protocol against the MinIO AIStor multipart handlers: PutObjectPartHandler routes the RDMA reader through the same per-part hash/checksum reader (AddChecksum) as a normal part, and CompleteMultipartUploadHandler assembles by part ETag without requiring per-part checksums at completion. Test Plan: uv run --extra boto3 pytest tests/test_multistorageclient/unit/providers/test_s3_rdma.py
…ards - register before-sign.s3.UploadPart so multipart RDMA parts carry the token - size RDMA buffers by memoryview.nbytes, not len() (multi-byte formats) - release the RDMA token before dropping the shim registry entry (and deregister the buffer even if token release raises) - reject rdma.multipart_chunksize < 1 (runtime + schema) to avoid a non-terminating multipart loop - bind the full-object RDMA GET to the HEAD'd version via IfMatch - add unit tests for each
d95d553 to
ad1080c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
multi-storage-client/src/multistorageclient/providers/_cuobj.py (1)
223-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a docstring to the public
install_hooks.
UploadPartis now correctly wired alongsidePutObject/GetObject(resolves the earlier multipart-token gap), but this public method lacks a docstring.As per coding guidelines, "All public Python functions must have docstrings (Sphinx autodoc extracts them)".
📝 Proposed docstring
def install_hooks(self, s3_client) -> None: + """Register the RDMA token injector on the S3 client's ``before-sign`` events. + + Wires ``PutObject``, ``GetObject``, and ``UploadPart`` so the + ``x-amz-rdma-token`` header is present before SigV4 signing. + """ events = s3_client.meta.events🤖 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 `@multi-storage-client/src/multistorageclient/providers/_cuobj.py` around lines 223 - 227, Add a concise docstring to the public install_hooks method describing that it registers token-injection hooks for S3 PutObject, GetObject, and UploadPart operations. Preserve the existing event registrations and behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@multi-storage-client/src/multistorageclient/providers/_cuobj.py`:
- Around line 223-227: Add a concise docstring to the public install_hooks
method describing that it registers token-injection hooks for S3 PutObject,
GetObject, and UploadPart operations. Preserve the existing event registrations
and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fcfcb1a0-e6cb-4052-a510-f27b80c2633f
📒 Files selected for processing (7)
.release_notes/.unreleased.mdmulti-storage-client/examples/rdma_roundtrip.pymulti-storage-client/src/multistorageclient/providers/_cuobj.pymulti-storage-client/src/multistorageclient/providers/cuobj_shim.cppmulti-storage-client/src/multistorageclient/providers/s3.pymulti-storage-client/src/multistorageclient/schema.pymulti-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
🚧 Files skipped from review as they are similar to previous changes (5)
- .release_notes/.unreleased.md
- multi-storage-client/src/multistorageclient/schema.py
- multi-storage-client/examples/rdma_roundtrip.py
- multi-storage-client/src/multistorageclient/providers/cuobj_shim.cpp
- multi-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
|
Re-ran with the review fixes in — msc-benchmark (MB/s, 8 proc × 4 threads):
RDMA PUT beats TCP everywhere (+6–33%); GET is on par to +13%. Real RDMA confirmed on the wire ( These absolutes are client-capped, not fabric. A single MSC process is GIL/boto3-bound (~1.3 GB/s RDMA vs ~0.29 GB/s TCP, ~4.6×) and scales with processes, not threads. For the fabric ceiling, |
|
Good data here...I had a curiosity about whether or not these were encrypted measurements (TCP was the column head...but was this TLS?).
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Harshavardhana ***@***.***>
Sent: Monday, 20 July 2026 03:51:22
To: NVIDIA/multi-storage-client ***@***.***>
Cc: Ed McClanahan ***@***.***>; Review requested ***@***.***>
Subject: Re: [NVIDIA/multi-storage-client] feat(s3): add optional S3-over-RDMA (cuObject) data plane (PR #87)
[https://avatars.githubusercontent.com/u/622699?s=20&v=4]harshavardhana left a comment (NVIDIA/multi-storage-client#87)<#87 (comment)>
Re-ran with the review fixes in — msc-benchmark against an 8-node MinIO AIStor cluster (cuObject RDMA) from an H200 client, 400G RoCE, dual-rail. RDMA vs plain TCP, real CRC64NVME on both.
msc-benchmark (MB/s, 8 proc × 4 threads):
Size RDMA PUT TCP PUT RDMA GET TCP GET
1 MB 126 119 120 125
16 MB 527 409 527 472
64 MB 776 646 790 701
256 MB 1030 859 1020 980
1 GB 1180 884 1200 1100
RDMA PUT beats TCP everywhere (+6–33%); GET is on par to +13%. Real RDMA confirmed on the wire (x-amz-rdma-reply: 200, empty body).
These absolutes are client-capped, not fabric. A single MSC process is GIL/boto3-bound (~1.3 GB/s RDMA vs ~0.29 GB/s TCP, ~4.6×) and scales with processes, not threads. For the fabric ceiling, warp (Go, all 8 hosts, 256 MiB, c64–128) hits ~44 GiB/s GET / ~22 GiB/s PUT — there the client is no longer the bottleneck and RDMA/TCP both saturate the fabric (RDMA GET leads slightly; TCP PUT edges ahead at very high concurrency). So RDMA's real win here is CPU offload — exactly what a Python client like MSC needs.
—
Reply to this email directly, view it on GitHub<#87?email_source=notifications&email_token=AC2GMOP5MFNRTG6EU75VVHD5FX2SVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBSGE2DEMRZGIY2M4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5021422921>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC2GMOJRYM6VDPLGKQXG47T5FX2SVAVCNFSNUABFKJSXA33TNF2G64TZHM4DQNZTGY3DGNRWHNEXG43VMU5TINZVGQZDQMZXGMY2C5QC>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/AC2GMOODCAGBKN33ZLF7QJD5FX2SVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBSGE2DEMRZGIY2M4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android<https://github.com/notifications/mobile/android/AC2GMON7KFL4JEVGSLV4W735FX2SVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBSGE2DEMRZGIY2M4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because your review was requested.Message ID: ***@***.***>
|
|
/ok to test 188a37a |
@shunjiad, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
The main concern I have right now is the inability to test in CI/CD to ensure nothing breaks over time. There's 2 main blockers here:
The primary blocker for both of these is licensing + Nixpkgs availability (for the portable development/build shell). I believe NVIDIA is working on giving permission to the NixOS foundation to redistribute CUDA packages in the official Nixpkgs binary cache. MinIO we migrated away from for local testing (b94c5db) due to it being deprecated. MinIO AIStor is under a non-free license so even if it were in Nixpkgs, its pre-built binaries likely wouldn't be cached in the official Nixpkgs binary cache. Since we can't have full confidence without continuous testing, I think it's better if we create a new
from .s3 import S3StorageProvider
PROVIDER = "s3_cuobject"
class S3CuObjectStorageProvider(S3StorageProvider):
"""
A concrete implementation of the :py:class:`multistorageclient.types.StorageProvider` for interacting with
S3 via NVIDIA cuObject.
https://docs.nvidia.com/gpudirect-storage/cuobject
"""
# Only override methods related to object GET/PUT (single and ranged/multi-part).This approach is already used for the The other thing I'd like to see changed is moving the C++ shim to Rust since:
Granted, we need to import |
This was on non-TLS, but TLS won't change anything anyway since the RDMA control channel has no data transfer, so keep the cost the same to some extent. TLS was not used. If you expect the TLS stack to be slower, I can do those tests, but it will only matter for TCP/HTTP-based runs; RDMA numbers won't change in any meaningful manner. |
For local testing, MinIO does provide an AIStor Free Tier license if that is something that can be incorporated. However, even beyond that, to test the cuObject client, you do need an RDMA-capable runner; I have tried over soft_roce, but it doesn't work. So yes, if you want this to be part of the CI to be tested, I personally don't have a good solution.
Not really; cuobjClient is fairly simple to incorporate. The newer CUDA release already has the relevant bits, and no, it doesn't need a GPU server to build this- the build itself can happen even on your laptops, and you can test it without having access to GPUs. The main testing infrastructure requirement is an RDMA RoCEv2-capable runner.
Sure I am okay with this. |
Does NVIDIA qualify for the free tier? My impression was that we would need an enterprise license, unless there's some special exception for enterprise open source projects. We'll need sign-off from legal for this arrangement and would like MinIO to package AIStor in Nixpkgs with cached builds in the official Nixpkgs binary cache. For an RDMA-capable system, we can try a QEMU VM down the road (since for unit tests in CI/CD, we primarily care about functionality instead of performance). That would enable local testing as well without special hardware since it's a mix of Macs and Linux machines for local development. The main question is if we can use iWARP instead (like Soft-RoCE) since it looks like a VirtIO RDMA network device doesn't exist yet.
The issue isn't during the MSC build step, but rather sourcing Since none of the CUDA packages in Nixpkgs have pre-built copies cached in the offical Nixpkgs binary cache due to licensing issues, we need to build all the ones in |
…C++ build Replaces the standalone hand-built libmsc_cuobj.so with a cuObject binding inside the existing multistorageclient_rust crate, gated behind an optional `rdma` cargo feature (default off, so the normal wheel needs no C++ toolchain or cuObject SDK). build.rs compiles a thin extern-C shim via cc and resolves the cuObject SDK from the environment (MSC_CUOBJ_HOME / CUDA_HOME); nothing is vendored into the repo. _cuobj.py now binds to the Rust extension and stays import-safe when built without the feature. Removes cuobj_shim.cpp and the MSC_CUOBJ_SHIM env handling.
Moves the entire S3-over-RDMA (cuObject) data plane out of the s3 provider
into a new s3_cuobject provider that subclasses S3StorageProvider and overrides
only object GET/PUT. The base s3 provider is now byte-identical to main again,
so the RDMA path -- which cannot be exercised in CI without an RDMA NIC + an
RDMA-capable endpoint -- can never affect it. Mirrors the s8k/ais_s3/gcs_s3
subclass pattern. Enable with `type: s3_cuobject` instead of `type: s3` +
`rdma: {}`. Registered in config/schema/providers; RDMA unit tests migrated.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@multi-storage-client/rust/build.rs`:
- Around line 23-25: Add CUDA_ROOT to the cargo:rerun-if-env-changed
declarations in build.rs alongside MSC_CUOBJ_HOME, CUDA_HOME, and CUDA_PATH,
ensuring changes to every environment variable read by resolve_cuobj_dirs
trigger the build script to rerun.
In `@multi-storage-client/rust/src/cuobj.rs`:
- Around line 222-233: Update cuobj_put_rdma_token to atomically remove the
token and obtain its pointer in a single TOKEN_REGISTRY lock acquisition before
calling client.put_rdma_token. Return Ok(()) when the atomic removal finds no
entry, ensuring concurrent calls cannot release the same descriptor twice and
preserving already-released-token no-op behavior.
In `@multi-storage-client/src/multistorageclient/providers/s3_cuobject.py`:
- Around line 237-246: Update the multipart-selection logic around
_rdma_upload_multipart to only use RDMA multipart for binary-mode streams,
excluding all text-mode file-like objects rather than checking only io.StringIO.
Preserve the existing size threshold and normal _put_object path, including
UTF-8 encoding for text data.
- Line 66: Update the upload-path validation around _rdma_upload_multipart so
all text-mode streams, not only io.StringIO, are detected before multipart RDMA
handling. Encode string chunks appropriately or route text streams through the
existing single-shot upload path, ensuring chunk.extend receives bytes and
binary-stream behavior remains unchanged.
🪄 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: CHILL
Plan: Enterprise
Run ID: c36e9f6e-42c5-45fc-99e9-ed6f0898f56d
⛔ Files ignored due to path filters (1)
multi-storage-client/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.gitignoremulti-storage-client/examples/rdma_roundtrip.pymulti-storage-client/rust/Cargo.tomlmulti-storage-client/rust/build.rsmulti-storage-client/rust/csrc/cuobj_shim.ccmulti-storage-client/rust/csrc/cuobj_shim.hmulti-storage-client/rust/src/cuobj.rsmulti-storage-client/rust/src/lib.rsmulti-storage-client/src/multistorageclient/config.pymulti-storage-client/src/multistorageclient/providers/__init__.pymulti-storage-client/src/multistorageclient/providers/_cuobj.pymulti-storage-client/src/multistorageclient/providers/s3_cuobject.pymulti-storage-client/src/multistorageclient/schema.pymulti-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
🚧 Files skipped from review as they are similar to previous changes (2)
- multi-storage-client/src/multistorageclient/schema.py
- multi-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
|
Thanks, submitted the new changes as per your review @commiterate |
- cuobj_put_rdma_token: remove the registry entry under the lock before releasing, so concurrent releases of the same token can't double-free the descriptor (TOCTOU) - route any text-mode stream (not just StringIO) to the single-shot encode path; multipart reads raw bytes and would crash on str chunks - build.rs: declare CUDA_ROOT as a rerun-if-env-changed trigger - test: text-mode stream over the part size takes single-shot
There was a problem hiding this comment.
♻️ Duplicate comments (1)
multi-storage-client/rust/src/cuobj.rs (1)
226-231: 🩺 Stability & Availability | 🟠 MajorRestore the token when native release fails.
The atomic removal fixes the TOCTOU race, but a failed
put_rdma_tokennow permanently loses the registry entry, preventing retries and potentially leaking the cuObject/RDMA resource. Reinsert(token, ptr)before returning the error, or maintain an equivalent retryable release state. This failure-path issue was also noted in the prior review fix.#!/bin/bash set -euo pipefail rg -n -A16 -B4 'miniors_cuobj_put_rdma_token|put_rdma_token' \ multi-storage-client/rust/csrc \ multi-storage-client/rust/src🤖 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 `@multi-storage-client/rust/src/cuobj.rs` around lines 226 - 231, Update the token release flow around TOKEN_REGISTRY removal and client.put_rdma_token so that when native release fails, the removed (token, ptr) entry is reinserted before returning the error, preserving retryability while keeping the atomic removal behavior.
🤖 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.
Duplicate comments:
In `@multi-storage-client/rust/src/cuobj.rs`:
- Around line 226-231: Update the token release flow around TOKEN_REGISTRY
removal and client.put_rdma_token so that when native release fails, the removed
(token, ptr) entry is reinserted before returning the error, preserving
retryability while keeping the atomic removal behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8fd920b9-ed7b-485a-9aca-95e50763cbae
📒 Files selected for processing (4)
multi-storage-client/rust/build.rsmulti-storage-client/rust/src/cuobj.rsmulti-storage-client/src/multistorageclient/providers/s3_cuobject.pymulti-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
🚧 Files skipped from review as they are similar to previous changes (3)
- multi-storage-client/rust/build.rs
- multi-storage-client/tests/test_multistorageclient/unit/providers/test_s3_rdma.py
- multi-storage-client/src/multistorageclient/providers/s3_cuobject.py
4 participants
What
Optional S3-over-RDMA data plane for the
s3provider via NVIDIA cuObject. Setrdma: {}and payloads move over RDMA (signedx-amz-rdma-tokenheader, empty HTTP body) instead of the HTTP/TLS path — offloading bulk transfer from the CPU on RDMA-capable endpoints like MinIO AIStor.How
rust_client); mutually exclusive with it;type: s3unchanged.rdma.multipart_chunksize(default 512 MiB).extern "C"shim over a process-widecuObjClient, loaded via ctypes; import-safe without the native lib.Review fixes
before-sign.s3.UploadPartso multipart parts carry the token.memoryview.nbytes, notlen().rdma.multipart_chunksize < 1(runtime + schema).IfMatch.Validated
msc-benchmark1 MiB–1 GiB: RDMA PUT beats TCP at every size (+6–33%); GET on par to +13% (numbers in comment below).Summary by CodeRabbit
s3_cuobjectstorage provider for accelerated GET/PUT and multipart uploads.rdma.multipart_chunksize).