Skip to content

Commit 176575f

Browse files
authored
feat(io): S3 LIST + footer probe + kvikIO backend, and io-layer fixes (#172)
Adds three capabilities to the `io` datasource layer, fixes several latent defects found while auditing it, and gives the layer its first test coverage. ## Bug fixes - **`io_request`: `copy_async` validates its host source before forming the pointer.** A null `host_buffer`, or an out-of-range `[src_off, src_off + size)`, previously produced UB (`nullptr + offset`) or a wild in-range pointer that the near-null check could not catch. Now returns `cudaErrorInvalidValue`. The existing asserts compile out in release, so the check has to happen before the pointer is formed. - **`prefetching_cache`: mark the *request* state load-failed on the `prefetch_loop` cancel/stop path.** Only the chunk states were being marked, leaving the request-level state inconsistent on cancellation. - **Four empty `catch` blocks were silently swallowing exceptions** — the `rest_reactor` and `uring_reactor` worker loops, and two in `templated_ioctx::shutdown`. Restoring the log call sites fills them. `CUCASCADE_LOG_*` are no-ops today, so this costs nothing at runtime but keeps the diagnostics in place for when logging is wired up. ## Behavioral fixes - **`rest_reactor::preferred_prefetching_stage`: `opportunistic` → `just_in_time`.** `prefetching_cache` branches on exactly this value, so S3 reads were being eagerly prefetched into cache buffers rather than read straight into the caller's buffer. Network round-trips are high-latency; read ahead on demand rather than prefilling the working set. - **`uri_parser`: S3 object keys are literal bytes.** `%`, `?` and `#` are ordinary key bytes, matching AWS CLI semantics, so `s3://b/a%20b` now opens the key `a%20b`. Previously the key was percent-decoded and split on `?`, which opened the wrong object. ## New capability - **S3 ListObjectsV2** — a hand-rolled, fail-closed list parser (no XML dependency); `presign_url(extra_canonical_query)` so caller query params participate in the signature (required for S3 to accept a presigned LIST); `authorize_list` on the authorizer interface and both SigV4 implementations; an `X-Amz-*` injection guard so callers cannot smuggle or override signing params; `rest_reactor::list_page`; and `rest_ioctx::list_objects_paged` / `list_objects` / `list_max_matches`. Pagination is guarded against non-conforming backends: truncated-without-token, truncated-but-empty, and non-advancing continuation token all throw rather than loop or silently truncate. - **Parquet footer probe** — `open_hint` plus `create_io_object(path, hint)`; a suffix-range GET that resolves the object size *and* stashes its trailing bytes in one round-trip; and a stash-hit fast path in `rest_reactor::host_read` so the footer reads that follow are served from memory instead of costing extra round-trips. Falls back to a plain HEAD on any unusable response (200 full body, 416, missing/unsatisfied `Content-Range`). - **Known-size open** — `create_io_object(path, known_size)`, so a size already learned from a LIST response builds the io_object with zero network: no HEAD, no probe. - **`kvikio_context`** — a local-file fallback backend built directly on `kvikio::FileHandle` rather than `cudf::io::datasource`, which keeps the io library cudf-free. Registered as a catch-all that `lookup_path` defers behind the explicit uring/rest backends, so `s3://` never resolves to it and a local file still routes to uring first. - **`kvikio_config`** — optional-per-field tunables: `nthreads`, `task_size`, `gds_threshold`, `bounce_buffer_size`, O_DIRECT reads and overread, per-block-device pools, and compat mode. An unset field means "leave kvikIO's own default alone", so the `KVIKIO_*` environment variables keep working as the outer default and this is an explicit in-process override layered on top. Every field except `compat_mode` maps to a setter on kvikIO's process-global `defaults` singleton and is documented as such — last context constructed wins. `compat_mode` is the exception: it rides the `FileHandle` constructor, so it scopes to files this ioctx opens and mutates nothing global. ## Cleanup - `metadata_store::get_metadata(cache_key)` overload, for callers that know the path but have not built an io_object yet. - Dropped `object_store_config::s3_use_async_backend` — no consumers, and it referenced types that do not exist. - Renamed `io_context_registry::lookup` → `lookup_path`. It is passed a full path, not a bare scheme (the checkers parse the URI / stat the filesystem themselves), and the docs claimed otherwise. ## Tests New `cucascade_io_tests` target — **732 assertions across 71 cases**, covering `uri_parser`, sigv4, the list parser, both authorizers, static credentials and `kvikio_config`. **The io layer had no tests at all before this.** Full suite: **6,330 assertions / 343 cases, 0 failures.** | Binary | Result | |---|---| | `cucascade_tests` | 3225 / 89 | | `cucascade_io_tests` (new) | 732 / 71 | | `cucascade_cudf_tests` | 2326 / 177 | | `cucascade_topology_discovery_tests` | 47 / 6 | Builds clean with `CUCASCADE_WARNINGS_AS_ERRORS=ON`; `pre-commit run -a` passes. ## Not verified The two S3 benchmarks are gated behind `CUCASCADE_BUILD_S3_BENCHMARK`, which requires aws-sdk-cpp (present only in the `s3-bench` pixi environment), so **they were not compiled**. By inspection they use only the single-argument `open_datasource` / `open_io_object` overloads — unchanged, since the new behavior was added as overloads rather than signature changes — and nothing switches over `io_context_type`, so the new `kvikio` enumerator cannot trip `-Werror=switch`. ## Follow-up REST perf instrumentation (per-chunk timings, queue wait, TTFB, retry/terminal counters, pool aggregation) is deliberately not part of this PR. The control-plane paths added here leave room for it and it layers on cleanly; the existing S3 benchmarks are its natural consumer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Authors: - Amin Aramoon (https://github.com/aminaramoon) Approvers: - https://github.com/felipeblazing URL: #172
1 parent cc4ca57 commit 176575f

42 files changed

Lines changed: 3915 additions & 80 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

CMakeLists.txt

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,11 @@ if(NOT CUCASCADE_TOPOLOGY_ONLY)
158158
pkg_check_modules(CURL REQUIRED IMPORTED_TARGET libcurl)
159159
find_package(OpenSSL REQUIRED)
160160

161+
# kvikIO — backs the local-file fallback ioctx (kvikio_context). Used
162+
# directly (not via cudf) so the io library stays cudf-free. Not swappable:
163+
# unlike moodycamel/invocable below there is no in-tree stand-in to replace.
164+
find_package(kvikio REQUIRED CONFIG)
165+
161166
# cucascade_io_thirdparty carries the swappable moodycamel + invocable
162167
# (abseil) usage requirements from a single place; the io object library,
163168
# its installable static/shared variants, and their in-tree consumers
@@ -402,7 +407,7 @@ if(CUCASCADE_BUILD_IO)
402407
# side by cuCascadeConfig.cmake (same names), mirroring the Numa::Numa
403408
# approach.
404409
set(CUCASCADE_IO_LINK_LIBS PkgConfig::LIBURING PkgConfig::CURL
405-
OpenSSL::Crypto)
410+
OpenSSL::Crypto kvikio::kvikio)
406411

407412
target_link_libraries(
408413
cucascade_io_objects PUBLIC cucascade_objects ${CUCASCADE_IO_LINK_LIBS}

include/cucascade/cudf/datasource.hpp

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,4 +159,19 @@ class datasource : public cudf::io::datasource {
159159
[[nodiscard]] std::unique_ptr<datasource> open_datasource(std::shared_ptr<ioctx> io_ctx,
160160
std::string path);
161161

162+
/// As above, forwarding @p hint to the backend's io_object resolution so it can,
163+
/// e.g., prefetch a parquet footer in the same round-trip as the size
164+
/// (@c open_hint::parquet_footer_probe). Backends that cannot act on the hint
165+
/// fall back to the plain open.
166+
[[nodiscard]] std::unique_ptr<datasource> open_datasource(std::shared_ptr<ioctx> io_ctx,
167+
std::string path,
168+
open_hint hint);
169+
170+
/// As above, with the object's size already known (e.g. from an S3
171+
/// ListObjectsV2 response), so a backend that can act on it skips its size
172+
/// discovery entirely (no HEAD for object stores).
173+
[[nodiscard]] std::unique_ptr<datasource> open_datasource(std::shared_ptr<ioctx> io_ctx,
174+
std::string path,
175+
std::uint64_t known_size);
176+
162177
} // namespace cucascade::io

include/cucascade/io/cache/metadata_store.hpp

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,34 @@
2020

2121
#include <cucascade/io/types.hpp>
2222

23+
#include <cstddef>
24+
#include <functional>
2325
#include <memory>
2426
#include <shared_mutex>
2527
#include <string>
28+
#include <string_view>
2629
#include <unordered_map>
2730

2831
namespace cucascade::io::cache {
2932

33+
namespace detail {
34+
35+
/// Transparent hasher so the store can be looked up by @c std::string_view (or
36+
/// @c const char*) without materialising a @c std::string. Paired with
37+
/// @c std::equal_to<> below, this enables C++20 heterogeneous lookup on the
38+
/// underlying @c unordered_map — without both, a string_view-taking getter
39+
/// would just construct a temporary key on every call and be strictly worse
40+
/// than taking @c std::string const&.
41+
struct string_hash {
42+
using is_transparent = void;
43+
[[nodiscard]] std::size_t operator()(std::string_view sv) const noexcept
44+
{
45+
return std::hash<std::string_view>{}(sv);
46+
}
47+
};
48+
49+
} // namespace detail
50+
3051
/**
3152
* @brief Thread-safe per-file metadata cache, keyed by an io_object's
3253
* raw_file_cache_id().
@@ -58,9 +79,19 @@ class metadata_store {
5879
/// miss.
5980
[[nodiscard]] std::shared_ptr<io_object_metadata> get_metadata(io_object const& obj) const;
6081

82+
/// As above but keyed directly by @c raw_file_cache_id() — for callers that
83+
/// know the path but have not built an io_object yet. Returns nullptr on miss.
84+
/// Looked up heterogeneously, so passing a @c string_view or a string literal
85+
/// allocates nothing.
86+
[[nodiscard]] std::shared_ptr<io_object_metadata> get_metadata(std::string_view cache_key) const;
87+
6188
private:
6289
mutable std::shared_mutex _mtx;
63-
std::unordered_map<std::string, std::shared_ptr<io_object_metadata>> _by_key;
90+
std::unordered_map<std::string,
91+
std::shared_ptr<io_object_metadata>,
92+
detail::string_hash,
93+
std::equal_to<>>
94+
_by_key;
6495
};
6596

6697
} // namespace cucascade::io::cache

include/cucascade/io/config.hpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#pragma once
1919

2020
#include <cucascade/io/cache/config.hpp>
21+
#include <cucascade/io/kvikio/config.hpp>
2122
#include <cucascade/io/object_store_config.hpp>
2223
#include <cucascade/io/rest/config.hpp>
2324
#include <cucascade/io/uring/config.hpp>
@@ -34,6 +35,7 @@ namespace cucascade::io {
3435
* Sub-configs:
3536
* - @c local — uring reactor tunables (local-disk IO path).
3637
* - @c rest — REST reactor tunables (S3/object-store IO path).
38+
* - @c kvikio — kvikIO fallback tunables (local-disk catch-all path).
3739
* - @c cache — prefetching cache tunables.
3840
* - @c object_store — object-store credentials and endpoint.
3941
*/
@@ -57,6 +59,12 @@ struct io_config {
5759
/// retry policy, etc.
5860
rest::config rest{};
5961

62+
/// kvikIO fallback configuration — thread-pool size, task/bounce sizing,
63+
/// O_DIRECT, compat mode. All fields default to "unset", leaving kvikIO's
64+
/// own env-var-seeded defaults in place. Note these are process-global once
65+
/// applied; see @ref kvikio_config.
66+
kvikio_config kvikio{};
67+
6068
/// Prefetching cache configuration — in-flight budget, pool sizing,
6169
/// dispose-after-use policy.
6270
cache::config cache{};

include/cucascade/io/datasource_factory.hpp

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,17 +38,13 @@ namespace cucascade::io {
3838
// ---------------------------------------------------------------------------
3939

4040
/**
41-
* @brief Thread-safe registry mapping URI schemes to @c ioctx instances.
41+
* @brief Thread-safe registry of @c ioctx backends, resolved by full path.
4242
*
43-
* The engine constructs a registry at startup and populates it with one
44-
* @c ioctx per backend (uring / gds / s3 / rdma_s3). The factory looks
45-
* up the correct backend by URI scheme at datasource-creation time.
46-
*
47-
* Scheme matching is case-insensitive: @c register_ioctx and @c lookup both
48-
* lowercase the scheme before storing / searching, matching the
49-
* normalization done by @c cucascade::io::parse (RFC 3986 §3.1). Callers may
50-
* register / look up with any casing — @c register_ioctx("S3", ...) and
51-
* @c lookup("s3") refer to the same entry.
43+
* The engine constructs a registry at startup and registers one entry per
44+
* backend (kvikio / uring / restful), each carrying a path-capability checker.
45+
* At datasource-creation time @c lookup_path runs the checkers against a full
46+
* path (the checkers parse the URI / stat the filesystem themselves) and picks
47+
* the backend, preferring an explicit backend over the kvikio catch-all.
5248
*
5349
* All operations are safe under concurrent reads; mutations take an exclusive
5450
* lock but are expected only at engine bootstrap / shutdown.
@@ -75,17 +71,21 @@ class io_context_registry {
7571
using factory_type = std::function<std::shared_ptr<io::ioctx>(const config_type&)>;
7672

7773
/**
78-
* @brief Register an ioctx for a scheme. Replaces any prior registration
79-
* for the same scheme.
74+
* @brief Register an ioctx backend. Replaces any prior registration for the
75+
* same type.
8076
*
81-
* The scheme is lowercased before storage; subsequent @c lookup calls
82-
* with any casing of the same scheme resolve to this entry.
83-
* @param type Opaque identifier for the ioctx type. Used by the engine to
84-
* identify the backend.
77+
* @param type Backend identifier (uring / restful / kvikio).
78+
* @param checker Decides whether this backend claims a given path.
79+
* @param factory Constructs the backend's ioctx; invoked by @c make_ioctx.
8580
*/
8681
void register_ioctx(io_context_type type, scheme_checker_type checker, factory_type factory);
8782

88-
std::optional<io_context_type> lookup(std::string_view scheme) const noexcept;
83+
/// Resolve the backend for a full @p path (not a bare scheme — the checkers
84+
/// parse the URI / stat the filesystem themselves). Explicit backends
85+
/// (uring / restful) take precedence over the kvikio catch-all, so `s3://`
86+
/// never resolves to kvikio and a local file routes to uring before the
87+
/// universal fallback. std::nullopt when nothing matches.
88+
std::optional<io_context_type> lookup_path(std::string_view path) const noexcept;
8989

9090
std::shared_ptr<ioctx> make_ioctx(io_context_type type) const noexcept;
9191

include/cucascade/io/io_context.hpp

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include <rmm/cuda_stream_view.hpp>
2828

2929
#include <cstddef>
30+
#include <cstdint>
3031
#include <memory>
3132
#include <optional>
3233
#include <span>
@@ -36,7 +37,15 @@
3637

3738
namespace cucascade::io {
3839

39-
enum class io_context_type { uring, restful };
40+
enum class io_context_type { uring, restful, kvikio };
41+
42+
/// Hint passed to @c open_io_object so a backend can tailor how it resolves an
43+
/// object's metadata. @c generic resolves the size however is cheapest for the
44+
/// scheme (a HEAD for object stores). @c parquet_footer_probe asks the backend
45+
/// to resolve the size *and* stash the object's trailing bytes in one
46+
/// round-trip (a suffix-range GET), so the parquet footer reads that follow are
47+
/// served locally instead of costing extra round-trips.
48+
enum class open_hint { generic, parquet_footer_probe };
4049

4150
namespace cache {
4251
class prefetching_cache;
@@ -102,6 +111,22 @@ class ioctx : public std::enable_shared_from_this<ioctx> {
102111
return create_io_object(std::move(path));
103112
}
104113

114+
/// As above, forwarding @p hint to the backend's io_object resolution so it
115+
/// can, e.g., prefetch a parquet footer in the same round-trip as the size.
116+
[[nodiscard]] std::shared_ptr<io_object> open_io_object(std::string path, open_hint hint)
117+
{
118+
return create_io_object(std::move(path), hint);
119+
}
120+
121+
/// As above, with the object's size already known (e.g. from an S3
122+
/// ListObjectsV2 response), so a backend that can act on it skips its size
123+
/// discovery entirely (no HEAD for object stores).
124+
[[nodiscard]] std::shared_ptr<io_object> open_io_object(std::string path,
125+
std::uint64_t known_size)
126+
{
127+
return create_io_object(std::move(path), known_size);
128+
}
129+
105130
/// Whether this backend can serve reads for @p path. Backends should
106131
/// validate scheme/protocol support and any backend-specific
107132
/// preconditions (e.g. file existence for local-disk backends).
@@ -263,6 +288,20 @@ class ioctx : public std::enable_shared_from_this<ioctx> {
263288
/// on unsupported / unreachable paths.
264289
virtual std::shared_ptr<io_object> create_io_object(std::string path) = 0;
265290

291+
/// Hinted variant. The base implementation ignores @p hint and delegates to
292+
/// the required @c create_io_object(path); a backend that can act on the hint
293+
/// (e.g. rest_ioctx's suffix-range footer probe) overrides this. Kept a
294+
/// distinct virtual — not a defaulted argument on the pure virtual above — so
295+
/// the hint dispatches on the dynamic type instead of binding statically.
296+
virtual std::shared_ptr<io_object> create_io_object(std::string path, open_hint hint);
297+
298+
/// Known-size variant. The base implementation ignores @p known_size and
299+
/// delegates to the required @c create_io_object(path); a backend whose size
300+
/// discovery would otherwise cost a round-trip overrides this to build the
301+
/// io_object without one. Same distinct-virtual rationale as the hint
302+
/// variant above.
303+
virtual std::shared_ptr<io_object> create_io_object(std::string path, std::uint64_t known_size);
304+
266305
/// Owned by this ioctx. Built by @ref initialize_cache, destroyed
267306
/// by @ref shutdown_cache (or the ioctx destructor as a safety net,
268307
/// though callers are expected to drive the lifecycle explicitly so

include/cucascade/io/io_request.hpp

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,9 +147,7 @@ struct device_cpy_request {
147147

148148
// Issue every copy on @p stream (a batch when there is more than one), then
149149
// record @p event once after the last so a single wait covers them all.
150-
cudaError_t copy_async(uint8_t* host_buffer,
151-
[[maybe_unused]] size_t bytes,
152-
cudaEvent_t event = nullptr) noexcept
150+
cudaError_t copy_async(uint8_t* host_buffer, size_t bytes, cudaEvent_t event = nullptr) noexcept
153151
{
154152
assert(host_buffer != nullptr && "Caller must provide a valid host buffer for the copy.");
155153
rmm::cuda_set_device_raii device_guard(rmm::cuda_device_id{device_id});
@@ -159,8 +157,24 @@ struct device_cpy_request {
159157
"Caller must provide a valid device destination buffer for the copy.");
160158
assert((c.src != nullptr || c.src_off + c.size <= bytes) &&
161159
"Caller must ensure the copy fits in the host buffer.");
162-
uint8_t* src_ptr = c.src != nullptr ? c.src : host_buffer + c.src_off;
163-
err = cudaMemcpyAsync(c.dst, src_ptr, c.size, cudaMemcpyHostToDevice, stream);
160+
// Resolve the host source. The asserts above are compiled out in release,
161+
// so validate *before* forming the pointer: for a bounce-staged copy
162+
// (c.src == nullptr) the source is host_buffer + c.src_off, but a null
163+
// host_buffer or an out-of-range [src_off, src_off + size) would otherwise
164+
// produce UB (nullptr + offset) or a wild in-range pointer that the
165+
// near-null check below cannot catch. A null-buffer segment must reach
166+
// here as c.src == nullptr, never as a non-null "nullptr + offset" pointer.
167+
uint8_t* src_ptr = nullptr;
168+
if (c.src != nullptr) {
169+
src_ptr = c.src;
170+
} else if (host_buffer != nullptr && c.src_off <= bytes && c.size <= bytes - c.src_off) {
171+
src_ptr = host_buffer + c.src_off;
172+
}
173+
if (c.dst == nullptr || src_ptr == nullptr ||
174+
reinterpret_cast<std::uintptr_t>(src_ptr) < 4096U) {
175+
return cudaErrorInvalidValue;
176+
}
177+
err = cudaMemcpyAsync(c.dst, src_ptr, c.size, cudaMemcpyHostToDevice, stream);
164178
if (err != cudaSuccess) { return err; }
165179
}
166180
if (event != nullptr) { err = cudaEventRecord(event, stream); }

0 commit comments

Comments
 (0)