From 5c5d8cb17c2e025be92214adeed5f7d12886abfd Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 17:37:48 +0300 Subject: [PATCH 1/8] Build on Windows: put every system call behind a platform floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libspill had no Windows implementation at all -- not a missing include, but a POSIX-only I/O and threading layer. Psi4 hit it first (issue #4): its clang-cl CI stops at open.c's while building libspill as a bundled dependency for the libpsio port. Everything the library asks of the operating system now goes through os.h, implemented once per platform in os_posix.c and os_win32.c. No other file names a system call, which is checkable with a grep and is the property worth keeping. Two things the Win32 side had to get right rather than merely compile: * The transfers stay POSITIONAL. §5a promises concurrent ls_read/ls_write on one store need no external locking, and pread/pwrite deliver that by carrying the offset. Win32 gets OVERLAPPED ReadFile/WriteFile, which is positional in the same sense even on a synchronous handle. _lseeki64 + _read would have compiled and quietly broken the promise. * Offsets are 64-bit throughout. off_t is 32 bits in the MSVC CRT, so the (off_t) casts this code used to do would have truncated silently past 2 GB -- inside the range this library exists for, not beyond it. Mapping is the one facility with no cheap equivalent: ls_map reserves the logical span PROT_NONE and maps each extent over its slice with MAP_FIXED, which on Windows needs a placeholder reservation (VirtualAlloc2 + MapViewOfFile3). Rather than carry an untested version of that, LS_MAPPED is refused at ls_open where LS_HAVE_MMAP is absent, with LS_ERR_MODE -- the answer callers already get for the other unsupported mode combinations. Every other mode works, and Psi4's shim never uses mapped mode. Also fixed, because it would have produced a DLL that exports nothing: LS_API was empty on _WIN32. Windows needs the mark to mean dllexport while the library is built and dllimport in a consumer, which is the same intent -fvisibility=hidden already carries elsewhere. Verification. The POSIX build is unchanged and `make check` and ctest pass in full. The no-mapping branches are not left for Windows to discover: LS_NO_MMAP compiles and runs exactly those branches on Linux, where the whole suite exercises them, and test_mapped skips (ctest SKIP_RETURN_CODE) rather than failing when mapping is absent. Both are new CI jobs, along with windows-latest under MSVC and clang-cl. I could not build the Windows path locally -- no MSVC and no mingw cross-compiler here -- so os_win32.c is reviewed, not run. The CI job added here is the first thing that will actually execute it. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 51 +++++++++ CMakeLists.txt | 33 +++++- Makefile | 3 +- include/libspill.h | 13 ++- src/alloc.c | 23 ++-- src/async.c | 68 +++++------ src/hdf5.c | 54 ++++----- src/internal.h | 18 +-- src/open.c | 100 ++++++++-------- src/os.h | 113 ++++++++++++++++++ src/os_posix.c | 100 ++++++++++++++++ src/os_win32.c | 242 +++++++++++++++++++++++++++++++++++++++ src/store.c | 166 +++++++++++++++------------ src/toc.c | 7 +- tests/test_mapped.c | 10 ++ tests/test_posix.c | 9 +- 16 files changed, 793 insertions(+), 217 deletions(-) create mode 100644 src/os.h create mode 100644 src/os_posix.c create mode 100644 src/os_win32.c diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2ff694..8597f45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,6 +62,57 @@ jobs: export LD_LIBRARY_PATH="$(dirname "$(find "$PWD/prefix" -name 'libspill.so.0.*')")" ./consumer/c_use && ./consumer/cxx_use && ./consumer/f_use + # Windows. Psi4 and OpenMolcas both ship Windows builds, so this is not a + # courtesy target: an adopter replacing its own I/O layer cannot carry a + # dependency that drops a platform it already supports. MSVC and clang-cl + # both, because clang-cl is what Psi4's Windows CI actually uses and the two + # disagree often enough to be worth separating. + windows: + name: windows (${{ matrix.toolset }}) + runs-on: windows-latest + strategy: + fail-fast: false + matrix: + include: + - toolset: msvc + flags: "" + - toolset: clang-cl + flags: "-T ClangCL" + steps: + - uses: actions/checkout@v4 + - name: Configure + run: > + cmake -S . -B build ${{ matrix.flags }} + -DLIBSPILL_BUILD_TESTS=ON + -DCMAKE_INSTALL_PREFIX=${{ github.workspace }}/prefix + - name: Build + run: cmake --build build --config Release -j + - name: Test + run: ctest --test-dir build -C Release --output-on-failure + - name: Install and consume + shell: bash + run: | + cmake --install build --config Release + cmake -S tests/consumer -B consumer \ + -Dlibspill_DIR="$(dirname "$(find "$PWD/prefix" -name libspillConfig.cmake)")" + cmake --build consumer --config Release -j + + # The Windows code path, run on Linux. Windows has no mmap, so LS_MAPPED is + # refused there and ls_map/ls_unmap take a different branch; LS_NO_MMAP + # compiles and runs exactly those branches here, where the whole suite can + # exercise them, instead of leaving them to be discovered on Windows. + no-mmap: + name: no mapping (the Windows path, on Linux) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - run: sudo apt-get update -qq && sudo apt-get install -y cmake + - name: Configure, build and test + run: | + cmake -S . -B build -DLIBSPILL_BUILD_TESTS=ON -DCMAKE_C_FLAGS=-DLS_NO_MMAP + cmake --build build -j + ctest --test-dir build --output-on-failure + # The suite must also pass with every optional piece absent -- that is what a # code taking only the POSIX core would build. minimal: diff --git a/CMakeLists.txt b/CMakeLists.txt index 8c6063a..fcd6c12 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,6 +40,14 @@ add_library(spill src/async.c) add_library(libspill::spill ALIAS spill) +# The platform floor: every system call the library makes goes through os.h, so +# that exactly one of these two files is built and nothing else is conditional. +if(WIN32) + target_sources(spill PRIVATE src/os_win32.c) +else() + target_sources(spill PRIVATE src/os_posix.c) +endif() + target_include_directories(spill PUBLIC $ $ @@ -47,6 +55,18 @@ target_include_directories(spill target_compile_features(spill PRIVATE c_std_99) +# Windows exports nothing from a DLL unless it is marked, so LS_API has to +# become __declspec(dllexport) while this library is being built and +# __declspec(dllimport) in a consumer -- see the LS_API block in libspill.h. +# LIBSPILL_STATIC is PUBLIC because a consumer of a static build must see it +# too, or it will look for imports that are not there. +if(WIN32) + target_compile_definitions(spill PRIVATE LIBSPILL_BUILD) + if(NOT BUILD_SHARED_LIBS) + target_compile_definitions(spill PUBLIC LIBSPILL_STATIC) + endif() +endif() + # The optional HDF5 backend. Off by default: §7b keeps POSIX the default and the # only backend that can deliver asynchrony, and HDF5 is here for codes that # already link it and want scratch files h5ls can read. A library built without @@ -64,8 +84,13 @@ endif() # because a consumer of the public header needs none of them. target_compile_definitions(spill PRIVATE _GNU_SOURCE) -find_package(Threads REQUIRED) -target_link_libraries(spill PUBLIC Threads::Threads) +# Windows threading is SRWLOCK and CONDITION_VARIABLE out of kernel32 -- there +# is nothing to find and nothing to link, and asking for Threads::Threads there +# would fail the configure over a dependency that does not exist. +if(NOT WIN32) + find_package(Threads REQUIRED) + target_link_libraries(spill PUBLIC Threads::Threads) +endif() set_target_properties(spill PROPERTIES VERSION ${PROJECT_VERSION} @@ -183,6 +208,10 @@ if(LIBSPILL_BUILD_TESTS) add_test(NAME ${t} COMMAND ${t}) endforeach() + # test_mapped is entirely about LS_MAPPED, so where the platform has no + # mapping it skips rather than fails. Nothing else in the suite is optional. + set_tests_properties(test_mapped PROPERTIES SKIP_RETURN_CODE 77) + # Built at both Fortran integer widths; a mismatch is invisible to the # toolchain, so it has to be caught here. foreach(w 0 1) diff --git a/Makefile b/Makefile index 36138e5..63c278b 100644 --- a/Makefile +++ b/Makefile @@ -22,7 +22,8 @@ FCFLAGS += -std=f2008 -Wall -Jfortran -Ifortran DEPFLAGS = -MMD -MP LDLIBS += -lpthread -SRC := src/error.c src/toc.c src/alloc.c src/store.c src/open.c src/async.c +SRC := src/error.c src/toc.c src/alloc.c src/store.c src/open.c src/async.c \ + src/os_posix.c # The optional HDF5 backend (§7b). Enabled when the headers are present; a # library built without them still accepts LS_HDF5 at compile time and refuses diff --git a/include/libspill.h b/include/libspill.h index 374ee75..8943287 100644 --- a/include/libspill.h +++ b/include/libspill.h @@ -23,8 +23,19 @@ extern "C" { * not the same as encapsulating them: without this, ls_rw, ls_toc_find, * ls_pool_start and thirty others would be linkable -- and therefore, in * practice, part of the ABI we promised to keep. */ +/* On Windows the same intent needs the opposite default: nothing is exported + * from a DLL unless it is marked, so LS_API has to be the mark. LIBSPILL_BUILD + * is defined by this library's own build (see CMakeLists.txt) and by nothing + * else, which is what makes the same declaration mean "export" here and + * "import" in a consumer. A static build defines neither and wants neither. */ #if defined(_WIN32) || defined(__CYGWIN__) -# define LS_API +# if defined(LIBSPILL_STATIC) +# define LS_API +# elif defined(LIBSPILL_BUILD) +# define LS_API __declspec(dllexport) +# else +# define LS_API __declspec(dllimport) +# endif #elif defined(__GNUC__) && (__GNUC__ >= 4) # define LS_API __attribute__((visibility("default"))) #else diff --git a/src/alloc.c b/src/alloc.c index fd85d38..45ae918 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -11,7 +11,6 @@ */ #include #include -#include #include #include "internal.h" @@ -71,7 +70,7 @@ static int fl_insert(ls_store *s, uint64_t foff, uint64_t len) if (s->nfl && s->fl[s->nfl - 1].foff + s->fl[s->nfl - 1].len == s->file_end) { s->file_end = s->fl[s->nfl - 1].foff; s->nfl--; - if (ftruncate(s->fd, (off_t)s->file_end) != 0) + if (ls_os_ftruncate(s->fd, s->file_end) != 0) ls_report(s, -errno, NULL, s->file_end, 0, "truncating freed tail"); } return LS_OK; @@ -90,7 +89,7 @@ int ls_alloc_best(ls_store *s, uint64_t need, ls_extent *out) need = round_up(need); if (need == 0) return 0; - pthread_mutex_lock(&s->alloc_lk); + ls_mutex_lock(&s->alloc_lk); for (i = 0; i < s->nfl; i++) if (s->fl[i].len >= need && (best == (size_t)-1 || s->fl[i].len < s->fl[best].len)) @@ -107,7 +106,7 @@ int ls_alloc_best(ls_store *s, uint64_t need, ls_extent *out) s->fl[best].len -= need; } } - pthread_mutex_unlock(&s->alloc_lk); + ls_mutex_unlock(&s->alloc_lk); return best != (size_t)-1; } @@ -116,7 +115,7 @@ int ls_alloc_largest(ls_store *s, uint64_t least, ls_extent *out) { size_t i, big = (size_t)-1; - pthread_mutex_lock(&s->alloc_lk); + ls_mutex_lock(&s->alloc_lk); for (i = 0; i < s->nfl; i++) if (big == (size_t)-1 || s->fl[i].len > s->fl[big].len) big = i; if (big != (size_t)-1 && s->fl[big].len >= round_up(least)) { @@ -128,7 +127,7 @@ int ls_alloc_largest(ls_store *s, uint64_t least, ls_extent *out) } else { big = (size_t)-1; } - pthread_mutex_unlock(&s->alloc_lk); + ls_mutex_unlock(&s->alloc_lk); return big != (size_t)-1; } @@ -136,11 +135,11 @@ int ls_alloc_largest(ls_store *s, uint64_t least, ls_extent *out) void ls_alloc_tail(ls_store *s, uint64_t need, ls_extent *out) { need = round_up(need); - pthread_mutex_lock(&s->alloc_lk); + ls_mutex_lock(&s->alloc_lk); out->foff = s->file_end; out->len = need; s->file_end += need; - pthread_mutex_unlock(&s->alloc_lk); + ls_mutex_unlock(&s->alloc_lk); } void ls_free_extents(ls_store *s, ls_extent *e, size_t n) @@ -151,18 +150,18 @@ void ls_free_extents(ls_store *s, ls_extent *e, size_t n) * already been written; recycling their extents would shrink the free list * back over the table of contents we just persisted. */ if (s->teardown) return; - pthread_mutex_lock(&s->alloc_lk); + ls_mutex_lock(&s->alloc_lk); for (i = 0; i < n; i++) (void)fl_insert(s, e[i].foff, e[i].len); - pthread_mutex_unlock(&s->alloc_lk); + ls_mutex_unlock(&s->alloc_lk); } uint64_t ls_alloc_live(ls_store *s) { uint64_t hole = 0, i; - pthread_mutex_lock(&s->alloc_lk); + ls_mutex_lock(&s->alloc_lk); for (i = 0; i < s->nfl; i++) hole += s->fl[i].len; i = s->file_end - LS_SUPER_SIZE - hole; - pthread_mutex_unlock(&s->alloc_lk); + ls_mutex_unlock(&s->alloc_lk); return i; } diff --git a/src/async.c b/src/async.c index 37bcdac..87744de 100644 --- a/src/async.c +++ b/src/async.c @@ -19,8 +19,8 @@ static void req_release(ls_store *s, ls_req *r) /* q_lk held */ { if (r->lprev) r->lprev->lnext = r->lnext; else s->live = r->lnext; if (r->lnext) r->lnext->lprev = r->lprev; - pthread_mutex_destroy(&r->lk); - pthread_cond_destroy(&r->cv); + ls_mutex_destroy(&r->lk); + ls_cond_destroy(&r->cv); free(r); } @@ -31,30 +31,30 @@ static void *worker(void *arg) for (;;) { ls_req *r; - pthread_mutex_lock(&s->q_lk); + ls_mutex_lock(&s->q_lk); while (!s->qh && !s->stop) - pthread_cond_wait(&s->q_cv, &s->q_lk); - if (!s->qh && s->stop) { pthread_mutex_unlock(&s->q_lk); break; } + ls_cond_wait(&s->q_cv, &s->q_lk); + if (!s->qh && s->stop) { ls_mutex_unlock(&s->q_lk); break; } r = s->qh; s->qh = r->next; if (!s->qh) s->qt = NULL; - pthread_mutex_unlock(&s->q_lk); + ls_mutex_unlock(&s->q_lk); { int rc = ls_rw(s, r->key, r->off, r->n, r->rbuf, r->wbuf, r->op); - pthread_mutex_lock(&r->lk); + ls_mutex_lock(&r->lk); r->status = rc; r->done = 1; - pthread_cond_broadcast(&r->cv); - pthread_mutex_unlock(&r->lk); + ls_cond_broadcast(&r->cv); + ls_mutex_unlock(&r->lk); - pthread_mutex_lock(&s->q_lk); + ls_mutex_lock(&s->q_lk); if (rc != LS_OK && s->drain_err == LS_OK) s->drain_err = rc; s->inflight--; - pthread_cond_broadcast(&s->q_cv); - pthread_mutex_unlock(&s->q_lk); + ls_cond_broadcast(&s->q_cv); + ls_mutex_unlock(&s->q_lk); } } return NULL; @@ -73,7 +73,7 @@ int ls_pool_start(ls_store *s) if (!s->thr) return -ENOMEM; for (i = 0; i < n; i++) - if (pthread_create(&s->thr[i], NULL, worker, s) != 0) break; + if (ls_thread_create(&s->thr[i], worker, s) != 0) break; s->nthr = i; if (s->nthr == 0) { free(s->thr); s->thr = NULL; return -EAGAIN; } return LS_OK; @@ -87,20 +87,20 @@ void ls_pool_stop(ls_store *s) { size_t i; - pthread_mutex_lock(&s->q_lk); + ls_mutex_lock(&s->q_lk); s->stop = 1; - pthread_cond_broadcast(&s->q_cv); - pthread_mutex_unlock(&s->q_lk); + ls_cond_broadcast(&s->q_cv); + ls_mutex_unlock(&s->q_lk); for (i = 0; i < s->nthr; i++) - pthread_join(s->thr[i], NULL); + ls_thread_join(s->thr[i]); free(s->thr); s->thr = NULL; s->nthr = 0; - pthread_mutex_lock(&s->q_lk); + ls_mutex_lock(&s->q_lk); while (s->live) req_release(s, s->live); - pthread_mutex_unlock(&s->q_lk); + ls_mutex_unlock(&s->q_lk); } static int submit(ls_store *s, const char *key, uint64_t off, size_t n, @@ -124,14 +124,14 @@ static int submit(ls_store *s, const char *key, uint64_t off, size_t n, r->n = n; r->rbuf = rbuf; r->wbuf = wbuf; - pthread_mutex_init(&r->lk, NULL); - pthread_cond_init(&r->cv, NULL); + ls_mutex_init(&r->lk); + ls_cond_init(&r->cv); - pthread_mutex_lock(&s->q_lk); + ls_mutex_lock(&s->q_lk); if (s->stop) { - pthread_mutex_unlock(&s->q_lk); - pthread_mutex_destroy(&r->lk); - pthread_cond_destroy(&r->cv); + ls_mutex_unlock(&s->q_lk); + ls_mutex_destroy(&r->lk); + ls_cond_destroy(&r->cv); free(r); return LS_ERR_BUSY; } @@ -142,8 +142,8 @@ static int submit(ls_store *s, const char *key, uint64_t off, size_t n, if (s->qt) s->qt->next = r; else s->qh = r; s->qt = r; s->inflight++; - pthread_cond_signal(&s->q_cv); - pthread_mutex_unlock(&s->q_lk); + ls_cond_signal(&s->q_cv); + ls_mutex_unlock(&s->q_lk); *out = r; return LS_OK; @@ -165,23 +165,23 @@ int ls_wait(ls_req *req) if (!req) return LS_ERR_INVAL; s = req->s; - pthread_mutex_lock(&req->lk); + ls_mutex_lock(&req->lk); while (!req->done) - pthread_cond_wait(&req->cv, &req->lk); + ls_cond_wait(&req->cv, &req->lk); rc = req->status; - pthread_mutex_unlock(&req->lk); + ls_mutex_unlock(&req->lk); - pthread_mutex_lock(&s->q_lk); + ls_mutex_lock(&s->q_lk); req_release(s, req); - pthread_mutex_unlock(&s->q_lk); + ls_mutex_unlock(&s->q_lk); return rc; } int ls_test(ls_req *req, int *done) { if (!req || !done) return LS_ERR_INVAL; - pthread_mutex_lock(&req->lk); + ls_mutex_lock(&req->lk); *done = req->done; - pthread_mutex_unlock(&req->lk); + ls_mutex_unlock(&req->lk); return LS_OK; } diff --git a/src/hdf5.c b/src/hdf5.c index 40c8eac..6269e51 100644 --- a/src/hdf5.c +++ b/src/hdf5.c @@ -193,7 +193,7 @@ static int h5_transfer(ls_store *s, const char *key, uint64_t off, size_t n, int ls_h5_open(ls_store *s, const char *path, int existing) { h5_quiet(); - pthread_mutex_init(&s->h5_lk, NULL); + ls_mutex_init(&s->h5_lk); s->h5_file = existing ? H5Fopen(path, H5F_ACC_RDWR, H5P_DEFAULT) : H5Fcreate(path, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT); @@ -210,7 +210,7 @@ int ls_h5_close(ls_store *s) int rc = LS_OK; if (s->h5_file >= 0 && H5Fclose(s->h5_file) < 0) rc = LS_ERR_BACKEND; s->h5_file = -1; - pthread_mutex_destroy(&s->h5_lk); + ls_mutex_destroy(&s->h5_lk); return rc; } @@ -219,9 +219,9 @@ int ls_h5_rw(ls_store *s, const char *key, uint64_t off, size_t n, { int rc; if (n == 0) return LS_OK; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); rc = h5_transfer(s, key, off, n, rbuf, wbuf, op); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); if (rc != LS_OK) ls_report(s, rc, key, off, n, op == LS_OP_WRITE ? "write" : "read"); return rc; @@ -230,27 +230,27 @@ int ls_h5_rw(ls_store *s, const char *key, uint64_t off, size_t n, int ls_h5_size(ls_store *s, const char *key, uint64_t *n) { int rc; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); rc = h5_extent(s, key, n); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); return rc; } int ls_h5_exists(ls_store *s, const char *key, int *found) { - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); *found = h5_dataset_exists(s, key); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); return LS_OK; } int ls_h5_erase(ls_store *s, const char *key) { int rc = LS_OK; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); if (!h5_dataset_exists(s, key)) rc = LS_ERR_NOKEY; else if (H5Ldelete(s->h5_file, key, H5P_DEFAULT) < 0) rc = LS_ERR_BACKEND; - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); return rc; } @@ -258,11 +258,11 @@ int ls_h5_reserve(ls_store *s, const char *key, uint64_t nbytes) { hid_t ds; int rc = LS_OK; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); ds = h5_open_or_create(s, key, nbytes, 1); if (ds < 0) rc = LS_ERR_BACKEND; else H5Dclose(ds); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); if (rc != LS_OK) ls_report(s, rc, key, 0, 0, "reserve"); return rc; } @@ -273,13 +273,13 @@ int ls_h5_append(ls_store *s, const char *key, size_t n, const void *buf, uint64_t at = 0; int rc; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); if (h5_dataset_exists(s, key)) { rc = h5_extent(s, key, &at); - if (rc != LS_OK) { pthread_mutex_unlock(&s->h5_lk); return rc; } + if (rc != LS_OK) { ls_mutex_unlock(&s->h5_lk); return rc; } } rc = h5_transfer(s, key, at, n, NULL, buf, LS_OP_WRITE); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); if (rc == LS_OK && off_out) *off_out = at; return rc; @@ -294,7 +294,7 @@ int ls_h5_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, tmp = malloc(n); if (!tmp) return -ENOMEM; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); rc = h5_transfer(s, key, off, n, tmp, NULL, LS_OP_READ); if (rc == LS_ERR_NOKEY || rc == LS_ERR_RANGE) { memset(tmp, 0, n); /* accumulate into fresh space */ @@ -304,7 +304,7 @@ int ls_h5_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, op(tmp, buf, n, ctx); rc = h5_transfer(s, key, off, n, NULL, tmp, LS_OP_WRITE); } - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); free(tmp); if (rc != LS_OK) ls_report(s, rc, key, off, n, "accumulate"); @@ -319,9 +319,9 @@ int ls_h5_set_attr(ls_store *s, const char *key, const void *blob, size_t n) hsize_t dim = (hsize_t)n; int rc = LS_OK; - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); ds = h5_open_or_create(s, key, 0, 1); - if (ds < 0) { pthread_mutex_unlock(&s->h5_lk); return LS_ERR_BACKEND; } + if (ds < 0) { ls_mutex_unlock(&s->h5_lk); return LS_ERR_BACKEND; } if (H5Aexists(ds, H5_ATTR_NAME) > 0) H5Adelete(ds, H5_ATTR_NAME); sp = H5Screate_simple(1, &dim, NULL); at = H5Acreate2(ds, H5_ATTR_NAME, H5T_NATIVE_UCHAR, sp, H5P_DEFAULT, H5P_DEFAULT); @@ -329,7 +329,7 @@ int ls_h5_set_attr(ls_store *s, const char *key, const void *blob, size_t n) if (at >= 0) H5Aclose(at); H5Sclose(sp); H5Dclose(ds); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); return rc; } @@ -339,14 +339,14 @@ int ls_h5_get_attr(ls_store *s, const char *key, void *blob, size_t *n) hsize_t dim = 0; int rc = LS_OK; - pthread_mutex_lock(&s->h5_lk); - if (!h5_dataset_exists(s, key)) { pthread_mutex_unlock(&s->h5_lk); return LS_ERR_NOKEY; } + ls_mutex_lock(&s->h5_lk); + if (!h5_dataset_exists(s, key)) { ls_mutex_unlock(&s->h5_lk); return LS_ERR_NOKEY; } ds = H5Dopen2(s->h5_file, key, H5P_DEFAULT); - if (ds < 0) { pthread_mutex_unlock(&s->h5_lk); return LS_ERR_BACKEND; } + if (ds < 0) { ls_mutex_unlock(&s->h5_lk); return LS_ERR_BACKEND; } if (H5Aexists(ds, H5_ATTR_NAME) <= 0) { *n = 0; H5Dclose(ds); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); return LS_OK; } at = H5Aopen(ds, H5_ATTR_NAME, H5P_DEFAULT); @@ -365,7 +365,7 @@ int ls_h5_get_attr(ls_store *s, const char *key, void *blob, size_t *n) } H5Aclose(at); H5Dclose(ds); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); return rc; } @@ -394,9 +394,9 @@ int ls_h5_keys(ls_store *s, char ***keys, size_t *n) hsize_t idx = 0; memset(&kl, 0, sizeof kl); - pthread_mutex_lock(&s->h5_lk); + ls_mutex_lock(&s->h5_lk); LS_H5_ITERATE(s->h5_file, H5_INDEX_NAME, H5_ITER_NATIVE, &idx, h5_collect, &kl); - pthread_mutex_unlock(&s->h5_lk); + ls_mutex_unlock(&s->h5_lk); if (kl.bad) { while (kl.n) free(kl.v[--kl.n]); diff --git a/src/internal.h b/src/internal.h index cb85174..c1c4a7c 100644 --- a/src/internal.h +++ b/src/internal.h @@ -3,7 +3,7 @@ #ifndef LS_INTERNAL_H #define LS_INTERNAL_H -#include +#include "os.h" #include #include @@ -55,19 +55,19 @@ struct ls_store { * data itself, which is both the point (§7b: inspectable with h5ls) and * the defect (x1.4-1.8 under varying-size churn). */ long h5_file; /* hid_t, kept opaque to this header */ - pthread_mutex_t h5_lk; + ls_mutex h5_lk; #endif char *path; ls_opts o; int direct; /* O_DIRECT actually in force */ /* table of contents. Locked -- DESIGN.md §4b. Never held across I/O. */ - pthread_mutex_t toc_lk; + ls_mutex toc_lk; ls_rec **tab; size_t nbuckets, nrec; /* extent allocator: free list sorted by offset, coalescing on release */ - pthread_mutex_t alloc_lk; + ls_mutex alloc_lk; int teardown; /* in ls_close: stop recycling extents */ ls_extent **retired; /* extent arrays a reader may still hold */ size_t nretired, retcap; @@ -80,10 +80,10 @@ struct ls_store { ls_rec *lru_head, *lru_tail; /* head = most recently used */ /* async worker pool */ - pthread_t *thr; + ls_thread *thr; size_t nthr; - pthread_mutex_t q_lk; - pthread_cond_t q_cv; + ls_mutex q_lk; + ls_cond q_cv; struct ls_req *qh, *qt; /* pending queue */ struct ls_req *live; /* every request not yet reaped */ int stop; @@ -101,8 +101,8 @@ struct ls_req { const void *wbuf; int status; int done; - pthread_mutex_t lk; - pthread_cond_t cv; + ls_mutex lk; + ls_cond cv; struct ls_req *next; /* pending queue */ struct ls_req *lnext, *lprev; /* live list, for release at ls_close */ }; diff --git a/src/open.c b/src/open.c index e8beb48..176d3bb 100644 --- a/src/open.c +++ b/src/open.c @@ -4,14 +4,10 @@ * exists so a scratch file can be inspected and reopened while debugging a * port, not so a job can resume from one. */ #include -#include #include #include #include #include -#include -#include -#include #include "internal.h" @@ -89,7 +85,7 @@ static int resolve_rank(int rank) if (r >= 0) return (int)r; } } - return (int)getpid(); /* never collides on a node; §4b */ + return ls_os_getpid(); /* never collides on a node; §4b */ } static char *build_path(const char *name, const ls_opts *o) @@ -120,7 +116,7 @@ int ls_store_exists(const char *name, const ls_opts *opts, int *found) { ls_opts o; char *path; - struct stat st; + uint64_t sz; if (!name || !*name || !found) return LS_ERR_INVAL; ls_opts_init(&o, LS_OPTS_VERSION); @@ -132,7 +128,7 @@ int ls_store_exists(const char *name, const ls_opts *opts, int *found) } path = build_path(name, &o); if (!path) return -ENOMEM; - *found = (stat(path, &st) == 0 && st.st_size > 0); + *found = (ls_os_path_size(path, &sz) == 0 && sz > 0); free(path); return LS_OK; } @@ -269,7 +265,7 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) { ls_opts o; ls_store *s; - struct stat st; + uint64_t fsize = 0; int rc = LS_OK; if (err) *err = LS_OK; @@ -299,6 +295,15 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) if (err) *err = LS_ERR_INVAL; return NULL; } +#ifndef LS_HAVE_MMAP + /* No mapping on this platform (see os.h). Refused here, once, so that + * ls_map and ls_unmap can never be reached with a mapped store and every + * other mode is unaffected. */ + if (o.mode == LS_MAPPED) { + if (err) *err = LS_ERR_MODE; + return NULL; + } +#endif /* A per-process memory tier would keep writes where the other processes * cannot see them. In a shared store that is a correctness failure, not a * tuning choice, so it is refused rather than ignored. */ @@ -341,44 +346,35 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) #ifdef LIBSPILL_HAVE_HDF5 if (o.backend == LS_HDF5) { - struct stat hst; - int existing = (stat(s->path, &hst) == 0 && hst.st_size > 0); - pthread_mutex_init(&s->toc_lk, NULL); - pthread_mutex_init(&s->alloc_lk, NULL); - pthread_mutex_init(&s->q_lk, NULL); - pthread_cond_init(&s->q_cv, NULL); + uint64_t hsize; + int existing = (ls_os_path_size(s->path, &hsize) == 0 && hsize > 0); + ls_mutex_init(&s->toc_lk); + ls_mutex_init(&s->alloc_lk); + ls_mutex_init(&s->q_lk); + ls_cond_init(&s->q_cv); rc = ls_h5_open(s, s->path, existing); if (rc != LS_OK) goto fail; return s; } #endif - pthread_mutex_init(&s->toc_lk, NULL); - pthread_mutex_init(&s->alloc_lk, NULL); - pthread_mutex_init(&s->q_lk, NULL); - pthread_cond_init(&s->q_cv, NULL); + ls_mutex_init(&s->toc_lk); + ls_mutex_init(&s->alloc_lk); + ls_mutex_init(&s->q_lk); + ls_cond_init(&s->q_cv); { - int flags = O_RDWR | O_CREAT; -#ifdef O_DIRECT - if (o.direct_io) flags |= O_DIRECT; -#endif - s->fd = open(s->path, flags, 0600); -#ifdef O_DIRECT - if (s->fd < 0 && o.direct_io) { /* filesystem may refuse it */ - s->fd = open(s->path, O_RDWR | O_CREAT, 0600); - if (s->fd >= 0) - ls_report(s, LS_OK, NULL, 0, 0, "O_DIRECT unavailable; using buffered I/O"); - } else if (s->fd >= 0 && o.direct_io) { - s->direct = 1; - } -#endif + int got_direct = 0; + s->fd = ls_os_open_rw(s->path, o.direct_io, &got_direct); if (s->fd < 0) { rc = -errno; goto fail; } + if (o.direct_io && !got_direct) + ls_report(s, LS_OK, NULL, 0, 0, "uncached I/O unavailable; using buffered I/O"); + s->direct = got_direct; } - if (fstat(s->fd, &st) != 0) { rc = -errno; goto fail; } + if (ls_os_file_size(s->fd, &fsize) != 0) { rc = -errno; goto fail; } s->file_end = LS_SUPER_SIZE; - if (st.st_size >= (off_t)LS_SUPER_SIZE) { + if (fsize >= (uint64_t)LS_SUPER_SIZE) { unsigned char sb[LS_SUPER_SIZE]; rc = ls_pread_all(s, sb, LS_SUPER_SIZE, 0); if (rc != LS_OK) goto fail; @@ -390,10 +386,10 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) uint64_t toc_off = get64(sb + 16), toc_n = get64(sb + 24); uint64_t fend = get64(sb + 32); uint32_t crc = get32(sb + 40); - uint64_t tlen = (uint64_t)st.st_size - toc_off; + uint64_t tlen = fsize - toc_off; unsigned char *tb; - if (toc_off < LS_SUPER_SIZE || toc_off > (uint64_t)st.st_size) { + if (toc_off < LS_SUPER_SIZE || toc_off > fsize) { rc = LS_ERR_CORRUPT; goto fail; } s->file_end = fend; @@ -407,7 +403,7 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) rc = rebuild_free_list(s); if (rc != LS_OK) goto fail; } - } else if (st.st_size > 0) { + } else if (fsize > 0) { rc = LS_ERR_CORRUPT; goto fail; } else if (o.parallel == LS_SHARED) { @@ -416,7 +412,7 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) * saying so is better than opening an empty store that fails later. */ rc = -ENOENT; goto fail; - } else if (ftruncate(s->fd, (off_t)LS_SUPER_SIZE) != 0) { + } else if (ls_os_ftruncate(s->fd, LS_SUPER_SIZE) != 0) { rc = -errno; goto fail; } @@ -429,7 +425,7 @@ ls_store *ls_open(const char *name, const ls_opts *opts, int *err) fail: if (err) *err = rc; if (s) { - if (s->fd >= 0) close(s->fd); + if (s->fd >= 0) ls_os_close(s->fd); free(s->tab); free(s->path); free(s); @@ -441,7 +437,7 @@ int ls_unlink_now(ls_store *s) { if (!s) return LS_ERR_INVAL; if (!s->path) return LS_OK; /* already unlinked */ - if (unlink(s->path) != 0 && errno != ENOENT) { + if (ls_os_unlink(s->path) != 0 && errno != ENOENT) { int rc = -errno; ls_report(s, rc, NULL, 0, 0, "unlink_now"); return rc; @@ -464,11 +460,11 @@ int ls_close(ls_store *s, int keep) #ifdef LIBSPILL_HAVE_HDF5 if (s->o.backend == LS_HDF5) { int hrc = ls_h5_close(s); - if (!keep && s->path) unlink(s->path); - pthread_mutex_destroy(&s->toc_lk); - pthread_mutex_destroy(&s->alloc_lk); - pthread_mutex_destroy(&s->q_lk); - pthread_cond_destroy(&s->q_cv); + if (!keep && s->path) ls_os_unlink(s->path); + ls_mutex_destroy(&s->toc_lk); + ls_mutex_destroy(&s->alloc_lk); + ls_mutex_destroy(&s->q_lk); + ls_cond_destroy(&s->q_cv); free(s->tab); free(s->path); free(s); @@ -501,7 +497,7 @@ int ls_close(ls_store *s, int keep) put32(sb + 40, crc32_of(tb, tn)); rc = ls_pwrite_all(s, tb, tn, s->file_end); if (rc == LS_OK) rc = ls_pwrite_all(s, sb, LS_SUPER_SIZE, 0); - if (rc == LS_OK && fsync(s->fd) != 0) rc = -errno; + if (rc == LS_OK && ls_os_fsync(s->fd) != 0) rc = -errno; } free(tb); if (first == LS_OK) first = rc; @@ -512,13 +508,13 @@ int ls_close(ls_store *s, int keep) ls_rec *r = s->tab[i]; while (r) { ls_rec *nx = r->hnext; ls_rec_free(s, r); r = nx; } } - if (s->fd >= 0) close(s->fd); - if (!keep && s->path) unlink(s->path); + if (s->fd >= 0) ls_os_close(s->fd); + if (!keep && s->path) ls_os_unlink(s->path); - pthread_mutex_destroy(&s->toc_lk); - pthread_mutex_destroy(&s->alloc_lk); - pthread_mutex_destroy(&s->q_lk); - pthread_cond_destroy(&s->q_cv); + ls_mutex_destroy(&s->toc_lk); + ls_mutex_destroy(&s->alloc_lk); + ls_mutex_destroy(&s->q_lk); + ls_cond_destroy(&s->q_cv); for (i = 0; i < s->nretired; i++) free(s->retired[i]); free(s->retired); free(s->fl); diff --git a/src/os.h b/src/os.h new file mode 100644 index 0000000..25dc472 --- /dev/null +++ b/src/os.h @@ -0,0 +1,113 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* libspill -- the platform floor. Internal; not installed. + * + * Everything the library asks of the operating system, in one place: the file + * primitives, threading, and whether mapping exists at all. POSIX and Win32 + * implement it in os_posix.c and os_win32.c respectively, and nothing else in + * the library names a system call. + * + * Two rules the Win32 side has to keep, because breaking either would compile + * silently and be wrong at run time: + * + * 1. The reads and writes are POSITIONAL. §5a promises that concurrent + * ls_read/ls_write on one store need no external locking, and the POSIX + * backend delivers that by using pread/pwrite, which carry the offset and + * touch no shared file position. _lseeki64 followed by _read would compile + * and would quietly break that promise. Win32 gets OVERLAPPED ReadFile / + * WriteFile, which is positional in the same sense. + * + * 2. Offsets are 64-bit. off_t is 32 bits in the MSVC CRT, so the casts this + * library used to do would have truncated silently somewhere past 2 GB -- + * inside, not beyond, the range libspill exists for. Everything below + * takes uint64_t and no caller mentions off_t. + * + * Errors come back the way the rest of the library expects: -1 (or NULL) with + * errno set, so the `rc = -errno` idiom at the call sites is unchanged. The + * Win32 side translates GetLastError() into the nearest errno itself. + */ +#ifndef LS_OS_H +#define LS_OS_H + +#include +#include + +/* Mapping is the one facility with no cheap Win32 equivalent: ls_map reserves + * the logical span PROT_NONE and maps each extent over its slice with + * MAP_FIXED, which on Windows needs a placeholder reservation + * (VirtualAlloc2 + MapViewOfFile3, Windows 10 1803 and later). Rather than + * carry an untested version of that, LS_MAPPED is refused at ls_open where + * this is not defined, with LS_ERR_MODE -- the same answer a caller already + * gets for the other unsupported mode combinations. Every other mode works. */ +/* LS_NO_MMAP forces the same path on a platform that does have mapping. It + * exists so the no-mapping branches are compiled and run by the ordinary + * Linux CI rather than only ever on Windows: everything below os.h behaves + * identically either way. */ +#if !defined(_WIN32) && !defined(LS_NO_MMAP) +# define LS_HAVE_MMAP 1 +#endif + +/* ------------------------------------------------------------------- files */ + +/* Open (creating if needed) for read and write. want_direct asks for + * uncached I/O; *got_direct says whether it was granted, since a filesystem + * may refuse it and buffered I/O is the correct fallback. Returns a descriptor + * or -1/errno. */ +int ls_os_open_rw(const char *path, int want_direct, int *got_direct); +int ls_os_close(int fd); + +/* Positional, and short transfers are the caller's to loop over -- exactly + * pread/pwrite. Return bytes moved, or -1/errno. */ +int64_t ls_os_pread (int fd, void *buf, size_t n, uint64_t off); +int64_t ls_os_pwrite(int fd, const void *buf, size_t n, uint64_t off); + +int ls_os_ftruncate(int fd, uint64_t len); +int ls_os_fsync(int fd); +int ls_os_file_size(int fd, uint64_t *size); /* 0, or -1/errno */ +int ls_os_path_size(const char *path, uint64_t *size); /* same, by name */ +int ls_os_unlink(const char *path); +int ls_os_getpid(void); + +/* Punch a zeroed range without writing it, where the platform can. Returns 0 on + * success, -1 if unsupported or refused -- the caller then writes zeros, which + * is always correct and merely slower. */ +int ls_os_zero_range(int fd, uint64_t off, uint64_t len); + +/* ----------------------------------------------------------------- threads + * + * A mutex, a condition variable and a joinable thread: the whole of what this + * library uses. Named ls_* rather than shimmed under the pthread_ names on + * purpose -- a header that defines pthread_mutex_lock is a trap for anyone who + * later links a real pthreads on Windows. */ + +#ifdef _WIN32 +/* is not included here: it would reach every translation unit and + * bring min/max macros and a great deal else with it. These mirror SRWLOCK, + * CONDITION_VARIABLE and HANDLE, which are a pointer each; os_win32.c asserts + * the sizes agree. */ +typedef struct { void *srw; } ls_mutex; +typedef struct { void *cond; } ls_cond; +typedef struct { void *h; } ls_thread; +#else +#include +typedef pthread_mutex_t ls_mutex; +typedef pthread_cond_t ls_cond; +typedef pthread_t ls_thread; +#endif + +void ls_mutex_init(ls_mutex *m); +void ls_mutex_destroy(ls_mutex *m); +void ls_mutex_lock(ls_mutex *m); +void ls_mutex_unlock(ls_mutex *m); + +void ls_cond_init(ls_cond *c); +void ls_cond_destroy(ls_cond *c); +void ls_cond_wait(ls_cond *c, ls_mutex *m); +void ls_cond_signal(ls_cond *c); +void ls_cond_broadcast(ls_cond *c); + +/* Returns 0, or -1/errno. The worker signature is pthread's so that the POSIX + * implementation is a direct call and the Win32 one trampolines. */ +int ls_thread_create(ls_thread *t, void *(*fn)(void *), void *arg); +void ls_thread_join(ls_thread t); + +#endif /* LS_OS_H */ diff --git a/src/os_posix.c b/src/os_posix.c new file mode 100644 index 0000000..f2e308d --- /dev/null +++ b/src/os_posix.c @@ -0,0 +1,100 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* The platform floor on POSIX: every one of these is the system call it is + * named after. The file exists so that os_win32.c has something to be the + * counterpart of; see os.h for what the two must agree on. */ +#ifndef _GNU_SOURCE +#define _GNU_SOURCE /* O_DIRECT and fallocate; see CMakeLists */ +#endif +#include +#include +#include +#include +#include +#include +#include + +#include "os.h" + +int ls_os_open_rw(const char *path, int want_direct, int *got_direct) +{ + int flags = O_RDWR | O_CREAT; + int fd; + + if (got_direct) *got_direct = 0; +#ifdef O_DIRECT + if (want_direct) flags |= O_DIRECT; +#else + (void)want_direct; +#endif + fd = open(path, flags, 0600); +#ifdef O_DIRECT + if (fd < 0 && want_direct) /* the filesystem may refuse it */ + return open(path, O_RDWR | O_CREAT, 0600); + if (fd >= 0 && want_direct && got_direct) *got_direct = 1; +#endif + return fd; +} + +int ls_os_close(int fd) { return close(fd); } + +int64_t ls_os_pread(int fd, void *buf, size_t n, uint64_t off) +{ return (int64_t)pread(fd, buf, n, (off_t)off); } + +int64_t ls_os_pwrite(int fd, const void *buf, size_t n, uint64_t off) +{ return (int64_t)pwrite(fd, buf, n, (off_t)off); } + +int ls_os_ftruncate(int fd, uint64_t len) { return ftruncate(fd, (off_t)len); } + +int ls_os_fsync(int fd) { return fsync(fd); } + +int ls_os_file_size(int fd, uint64_t *size) +{ + struct stat st; + if (fstat(fd, &st) != 0) return -1; + if (size) *size = (uint64_t)st.st_size; + return 0; +} + +int ls_os_path_size(const char *path, uint64_t *size) +{ + struct stat st; + if (stat(path, &st) != 0) return -1; + if (size) *size = (uint64_t)st.st_size; + return 0; +} + +int ls_os_unlink(const char *path) { return unlink(path); } + +int ls_os_getpid(void) { return (int)getpid(); } + +int ls_os_zero_range(int fd, uint64_t off, uint64_t len) +{ +#ifdef FALLOC_FL_ZERO_RANGE + return fallocate(fd, FALLOC_FL_ZERO_RANGE, (off_t)off, (off_t)len); +#else + (void)fd; (void)off; (void)len; + errno = ENOSYS; + return -1; +#endif +} + +/* ----------------------------------------------------------------- threads */ + +void ls_mutex_init(ls_mutex *m) { pthread_mutex_init(m, NULL); } +void ls_mutex_destroy(ls_mutex *m) { pthread_mutex_destroy(m); } +void ls_mutex_lock(ls_mutex *m) { pthread_mutex_lock(m); } +void ls_mutex_unlock(ls_mutex *m) { pthread_mutex_unlock(m); } + +void ls_cond_init(ls_cond *c) { pthread_cond_init(c, NULL); } +void ls_cond_destroy(ls_cond *c) { pthread_cond_destroy(c); } +void ls_cond_wait(ls_cond *c, ls_mutex *m) { pthread_cond_wait(c, m); } +void ls_cond_signal(ls_cond *c) { pthread_cond_signal(c); } +void ls_cond_broadcast(ls_cond *c) { pthread_cond_broadcast(c); } + +int ls_thread_create(ls_thread *t, void *(*fn)(void *), void *arg) +{ + int rc = pthread_create(t, NULL, fn, arg); + if (rc != 0) { errno = rc; return -1; } + return 0; +} +void ls_thread_join(ls_thread t) { pthread_join(t, NULL); } diff --git a/src/os_win32.c b/src/os_win32.c new file mode 100644 index 0000000..d6f17c7 --- /dev/null +++ b/src/os_win32.c @@ -0,0 +1,242 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* The platform floor on Windows (MSVC, clang-cl and MinGW alike). + * + * The two things worth reading os.h for before changing anything here: the + * reads and writes must stay POSITIONAL, because §5a promises concurrent + * ls_read/ls_write need no external locking, and offsets must stay 64-bit, + * because the CRT's off_t is 32 bits. + * + * Descriptors: the library passes `int fd` around, so this keeps the CRT layer + * (_open/_close) for the lifecycle and reaches the underlying HANDLE with + * _get_osfhandle for the positional transfers. That is cheaper than threading a + * HANDLE through every structure, and it keeps _unlink and the delete-on-close + * semantics working the way the rest of the library expects. + */ +#include +#include +#include +#include +#include +#include +#include +#include + +#define WIN32_LEAN_AND_MEAN +#include +#include + +#include "os.h" + +/* os.h mirrors these as one pointer each rather than including + * everywhere; if that ever stops being true it must fail here, loudly. */ +typedef char ls_srw_size_check[sizeof(SRWLOCK) <= sizeof(void *) ? 1 : -1]; +typedef char ls_cv_size_check [sizeof(CONDITION_VARIABLE) <= sizeof(void *) ? 1 : -1]; +typedef char ls_h_size_check [sizeof(HANDLE) <= sizeof(void *) ? 1 : -1]; + +/* The rest of the library reports failures as -errno, so Win32 status has to + * become errno here rather than at the call sites. Only the codes these + * operations can actually produce are listed; anything else becomes EIO, which + * is what the callers treat as "the I/O failed" anyway. */ +static void set_errno_from_win32(DWORD e) +{ + switch (e) { + case ERROR_SUCCESS: errno = 0; break; + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: errno = ENOENT; break; + case ERROR_ACCESS_DENIED: + case ERROR_SHARING_VIOLATION: errno = EACCES; break; + case ERROR_FILE_EXISTS: + case ERROR_ALREADY_EXISTS: errno = EEXIST; break; + case ERROR_NOT_ENOUGH_MEMORY: + case ERROR_OUTOFMEMORY: errno = ENOMEM; break; + case ERROR_DISK_FULL: errno = ENOSPC; break; + case ERROR_INVALID_HANDLE: errno = EBADF; break; + case ERROR_INVALID_PARAMETER: errno = EINVAL; break; + case ERROR_HANDLE_EOF: errno = 0; break; + default: errno = EIO; break; + } +} + +static HANDLE handle_of(int fd) +{ + intptr_t h = _get_osfhandle(fd); + return (h == -1) ? INVALID_HANDLE_VALUE : (HANDLE)h; +} + +/* ------------------------------------------------------------------- files */ + +int ls_os_open_rw(const char *path, int want_direct, int *got_direct) +{ + int fd = -1; + + /* FILE_FLAG_NO_BUFFERING is the O_DIRECT equivalent, but it cannot be + * requested through _open, and it constrains offset, length and buffer + * alignment more tightly than O_DIRECT does. Buffered I/O is a correct + * answer to direct_io -- the caller is told it did not get it -- so this + * reports "not granted" rather than carrying an untested aligned path. */ + (void)want_direct; + if (got_direct) *got_direct = 0; + + if (_sopen_s(&fd, path, _O_RDWR | _O_CREAT | _O_BINARY, _SH_DENYNO, + _S_IREAD | _S_IWRITE) != 0) + return -1; /* _sopen_s has set errno */ + return fd; +} + +int ls_os_close(int fd) { return _close(fd); } + +/* Positional, and short transfers are the caller's to loop over, exactly as + * pread/pwrite. + * + * The subtlety, since a reviewer will reach for the documentation here: the + * handle is NOT opened FILE_FLAG_OVERLAPPED, and passing an OVERLAPPED to a + * synchronous handle is explicitly allowed -- the transfer starts at the offset + * in the OVERLAPPED and the call does not return until it completes. It also + * leaves the shared file pointer wherever it finished, which is why this is + * still safe under §5a: every transfer states its own offset and nothing in the + * library ever reads that pointer. Two concurrent calls each get their own + * bytes. What would NOT be safe is _lseeki64 + _read, where the seek and the + * read are separate steps over that same shared pointer. */ +int64_t ls_os_pread(int fd, void *buf, size_t n, uint64_t off) +{ + HANDLE h = handle_of(fd); + OVERLAPPED ov; + DWORD got = 0, want = (n > 0xFFFFFFFFu) ? 0xFFFFFFFFu : (DWORD)n; + + if (h == INVALID_HANDLE_VALUE) { errno = EBADF; return -1; } + memset(&ov, 0, sizeof ov); + ov.Offset = (DWORD)(off & 0xFFFFFFFFu); + ov.OffsetHigh = (DWORD)(off >> 32); + + if (!ReadFile(h, buf, want, &got, &ov)) { + DWORD e = GetLastError(); + if (e == ERROR_HANDLE_EOF) return 0; /* read past end: 0, like pread */ + set_errno_from_win32(e); + return -1; + } + return (int64_t)got; +} + +int64_t ls_os_pwrite(int fd, const void *buf, size_t n, uint64_t off) +{ + HANDLE h = handle_of(fd); + OVERLAPPED ov; + DWORD put = 0, want = (n > 0xFFFFFFFFu) ? 0xFFFFFFFFu : (DWORD)n; + + if (h == INVALID_HANDLE_VALUE) { errno = EBADF; return -1; } + memset(&ov, 0, sizeof ov); + ov.Offset = (DWORD)(off & 0xFFFFFFFFu); + ov.OffsetHigh = (DWORD)(off >> 32); + + if (!WriteFile(h, buf, want, &put, &ov)) { + set_errno_from_win32(GetLastError()); + return -1; + } + return (int64_t)put; +} + +int ls_os_ftruncate(int fd, uint64_t len) +{ + /* _chsize_s takes a 64-bit length, which _chsize does not. Growing a file + * this way zero-fills, as ftruncate does. */ + int e = _chsize_s(fd, (__int64)len); + if (e != 0) { errno = e; return -1; } + return 0; +} + +int ls_os_fsync(int fd) +{ + HANDLE h = handle_of(fd); + if (h == INVALID_HANDLE_VALUE) { errno = EBADF; return -1; } + if (!FlushFileBuffers(h)) { set_errno_from_win32(GetLastError()); return -1; } + return 0; +} + +int ls_os_file_size(int fd, uint64_t *size) +{ + HANDLE h = handle_of(fd); + LARGE_INTEGER li; + if (h == INVALID_HANDLE_VALUE) { errno = EBADF; return -1; } + if (!GetFileSizeEx(h, &li)) { set_errno_from_win32(GetLastError()); return -1; } + if (size) *size = (uint64_t)li.QuadPart; + return 0; +} + +int ls_os_path_size(const char *path, uint64_t *size) +{ + struct __stat64 st; + if (_stat64(path, &st) != 0) return -1; /* _stat64 has set errno */ + if (size) *size = (uint64_t)st.st_size; + return 0; +} + +int ls_os_unlink(const char *path) { return _unlink(path); } + +int ls_os_getpid(void) { return (int)_getpid(); } + +int ls_os_zero_range(int fd, uint64_t off, uint64_t len) +{ + /* FSCTL_SET_ZERO_DATA is the equivalent, but only on a sparse file, and + * making the store sparse is a policy decision this port is not entitled to + * take. The caller writes zeros instead, which is always correct. */ + (void)fd; (void)off; (void)len; + errno = ENOSYS; + return -1; +} + +/* ----------------------------------------------------------------- threads + * + * SRWLOCK and CONDITION_VARIABLE need no destruction and cannot fail to + * initialise, which is why the destroy entry points below are empty rather than + * missing: the POSIX side does need them. Neither is recursive -- nor is the + * pthread mutex this replaces, and the library never relocks. */ + +void ls_mutex_init(ls_mutex *m) { InitializeSRWLock((PSRWLOCK)&m->srw); } +void ls_mutex_destroy(ls_mutex *m) { (void)m; } +void ls_mutex_lock(ls_mutex *m) { AcquireSRWLockExclusive((PSRWLOCK)&m->srw); } +void ls_mutex_unlock(ls_mutex *m) { ReleaseSRWLockExclusive((PSRWLOCK)&m->srw); } + +void ls_cond_init(ls_cond *c) { InitializeConditionVariable((PCONDITION_VARIABLE)&c->cond); } +void ls_cond_destroy(ls_cond *c) { (void)c; } + +void ls_cond_wait(ls_cond *c, ls_mutex *m) +{ + SleepConditionVariableSRW((PCONDITION_VARIABLE)&c->cond, + (PSRWLOCK)&m->srw, INFINITE, 0); +} +void ls_cond_signal(ls_cond *c) { WakeConditionVariable((PCONDITION_VARIABLE)&c->cond); } +void ls_cond_broadcast(ls_cond *c) { WakeAllConditionVariable((PCONDITION_VARIABLE)&c->cond); } + +/* The worker signature is pthread's, so Win32 needs a trampoline. _beginthreadex + * rather than CreateThread because the workers call into the CRT. */ +typedef struct { void *(*fn)(void *); void *arg; } ls_trampoline; + +static unsigned __stdcall ls_thread_main(void *p) +{ + ls_trampoline t = *(ls_trampoline *)p; + free(p); + t.fn(t.arg); + return 0; +} + +int ls_thread_create(ls_thread *t, void *(*fn)(void *), void *arg) +{ + ls_trampoline *tr = (ls_trampoline *)malloc(sizeof *tr); + uintptr_t h; + + if (!tr) { errno = ENOMEM; return -1; } + tr->fn = fn; + tr->arg = arg; + + h = _beginthreadex(NULL, 0, ls_thread_main, tr, 0, NULL); + if (h == 0) { free(tr); return -1; } /* _beginthreadex has set errno */ + t->h = (void *)h; + return 0; +} + +void ls_thread_join(ls_thread t) +{ + if (!t.h) return; + WaitForSingleObject((HANDLE)t.h, INFINITE); + CloseHandle((HANDLE)t.h); +} diff --git a/src/store.c b/src/store.c index 0b10b30..8e30eb2 100644 --- a/src/store.c +++ b/src/store.c @@ -4,18 +4,17 @@ * what makes the free-list reuse ours to control, and it sidesteps the * many-small-datasets problem §7b flags for HDF5. */ #include -#include #include #include #include #include -#include -#include -#include -#include #include "internal.h" +#ifdef LS_HAVE_MMAP /* LS_HAVE_MMAP comes from os.h, via internal.h */ +#include +#endif + /* ------------------------------------------------------------- raw file I/O */ static void *aligned_alloc_or_null(size_t n) @@ -49,9 +48,9 @@ static int rw_all(ls_store *s, void *buf, const void *cbuf, size_t n, (off & (uint64_t)(LS_ALIGN - 1)) == 0 && (n & (size_t)(LS_ALIGN - 1)) == 0)) { while (done < n) { - ssize_t r = is_write - ? pwrite(s->fd, cp + done, n - done, (off_t)(off + done)) - : pread (s->fd, p + done, n - done, (off_t)(off + done)); + int64_t r = is_write + ? ls_os_pwrite(s->fd, cp + done, n - done, off + done) + : ls_os_pread (s->fd, p + done, n - done, off + done); if (r < 0) { if (errno == EINTR) continue; return -errno; } if (r == 0) return is_write ? -ENOSPC : -EIO; done += (size_t)r; @@ -70,21 +69,21 @@ static int rw_all(ls_store *s, void *buf, const void *cbuf, size_t n, size_t skew = (size_t)(abs - base); size_t chunk = n - done; size_t span; - ssize_t r; + int64_t r; if (chunk > LS_BOUNCE - skew) chunk = LS_BOUNCE - skew; span = (skew + chunk + LS_ALIGN - 1) & ~(size_t)(LS_ALIGN - 1); if (is_write) { if (skew || span != skew + chunk) { /* read-modify-write */ - r = pread(s->fd, bb, span, (off_t)base); + r = ls_os_pread(s->fd, bb, span, base); if (r < 0) { rc = -errno; break; } if ((size_t)r < span) memset(bb + r, 0, span - (size_t)r); } memcpy(bb + skew, cp + done, chunk); - r = pwrite(s->fd, bb, span, (off_t)base); + r = ls_os_pwrite(s->fd, bb, span, base); } else { - r = pread(s->fd, bb, span, (off_t)base); + r = ls_os_pread(s->fd, bb, span, base); if (r >= 0 && (size_t)r < span) memset(bb + r, 0, span - (size_t)r); if (r >= 0) memcpy(p + done, bb + skew, chunk); } @@ -111,11 +110,8 @@ static int zero_range(ls_store *s, uint64_t off, uint64_t len) unsigned char *z; int rc = LS_OK; -#ifdef FALLOC_FL_ZERO_RANGE - if (!s->direct && - fallocate(s->fd, FALLOC_FL_ZERO_RANGE, (off_t)off, (off_t)len) == 0) + if (!s->direct && ls_os_zero_range(s->fd, off, len) == 0) return LS_OK; -#endif z = aligned_alloc_or_null(CH); if (!z) return -ENOMEM; memset(z, 0, CH); @@ -203,9 +199,9 @@ int ls_evict_to(ls_store *s, size_t budget, ls_rec *keep) int ls_spill_all(ls_store *s) { int rc; - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); rc = ls_evict_to(s, 0, NULL); - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return rc; } @@ -363,31 +359,31 @@ int ls_rw(ls_store *s, const char *key, uint64_t off, size_t n, return LS_ERR_INVAL; if (off + n < off) return LS_ERR_RANGE; - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (op == LS_OP_READ) { - if (!r) { pthread_mutex_unlock(&s->toc_lk); + if (!r) { ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_NOKEY, key, off, n, "read"); return LS_ERR_NOKEY; } - if (off + n > r->size) { pthread_mutex_unlock(&s->toc_lk); + if (off + n > r->size) { ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_RANGE, key, off, n, "read"); return LS_ERR_RANGE; } } else { if (s->o.parallel == LS_SHARED && (!r || off + n > r->size)) { /* would change the layout */ - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_MODE, key, off, n, "a shared store's layout is frozen; reserve it before sharing"); return LS_ERR_MODE; } if (!r) r = ls_toc_insert(s, key); - if (!r) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!r) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } /* A write past the end leaves the gap zero-filled (§4b); fresh extents * arrive zeroed from ensure_extents and the resident path memsets. */ rc = ensure_cap(s, r, off + n); if (rc != LS_OK) { - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, rc, key, off, n, "reserving space for write"); return rc; } @@ -396,13 +392,13 @@ int ls_rw(ls_store *s, const char *key, uint64_t off, size_t n, if (r->mem) ls_lru_touch(s, r); r->busy++; ls_place_of(r, &pl); - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); rc = ls_scatter(s, &pl, off, n, rbuf, wbuf, op); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r->busy--; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (rc != LS_OK) ls_report(s, rc, key, off, n, op == LS_OP_WRITE ? "write" : "read"); @@ -437,9 +433,9 @@ int ls_exists(ls_store *s, const char *key, int *found) { if (!s || !found || !ls_key_ok(key)) return LS_ERR_INVAL; LS_IF_HDF5(s, ls_h5_exists(s, key, found)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); *found = ls_toc_find(s, key) != NULL; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return LS_OK; } @@ -448,10 +444,10 @@ int ls_size(ls_store *s, const char *key, uint64_t *nbytes) ls_rec *r; if (!s || !nbytes || !ls_key_ok(key)) return LS_ERR_INVAL; LS_IF_HDF5(s, ls_h5_size(s, key, nbytes)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (r) *nbytes = r->size; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return r ? LS_OK : LS_ERR_NOKEY; } @@ -461,10 +457,10 @@ int ls_erase(ls_store *s, const char *key) if (!s || !ls_key_ok(key)) return LS_ERR_INVAL; if (s->o.parallel == LS_SHARED) return LS_ERR_MODE; LS_IF_HDF5(s, ls_h5_erase(s, key)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (r) { ls_toc_unlink(s, r); ls_lru_drop(s, r); ls_rec_free(s, r); } - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return r ? LS_OK : LS_ERR_NOKEY; } @@ -476,13 +472,13 @@ int ls_reserve(ls_store *s, const char *key, uint64_t nbytes) if (s->o.parallel == LS_SHARED) return LS_ERR_MODE; LS_IF_HDF5(s, ls_h5_reserve(s, key, nbytes)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (!r) r = ls_toc_insert(s, key); - if (!r) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!r) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } rc = ensure_cap(s, r, nbytes); if (rc == LS_OK && nbytes > r->size) r->size = nbytes; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (rc != LS_OK) ls_report(s, rc, key, 0, 0, "reserve"); @@ -497,9 +493,9 @@ int ls_keys(ls_store *s, char ***keys, size_t *n) if (!s || !keys || !n) return LS_ERR_INVAL; LS_IF_HDF5(s, ls_h5_keys(s, keys, n)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); out = s->nrec ? calloc(s->nrec, sizeof *out) : calloc(1, sizeof *out); - if (!out) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!out) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } for (i = 0; i < s->nbuckets; i++) { ls_rec *r; for (r = s->tab[i]; r; r = r->hnext) { @@ -507,13 +503,13 @@ int ls_keys(ls_store *s, char ***keys, size_t *n) if (!out[k]) { while (k) free(out[--k]); free(out); - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } k++; } } - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); *keys = out; *n = k; @@ -545,17 +541,17 @@ int ls_append(ls_store *s, const char *key, size_t n, const void *buf, /* The offset is taken and the record extended under one lock. That is the * whole point: ls_size followed by ls_write is not the same thing, because * another thread can append in between. */ - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (!r) r = ls_toc_insert(s, key); - if (!r) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!r) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } off = r->size; - if (off + n < off) { pthread_mutex_unlock(&s->toc_lk); return LS_ERR_RANGE; } + if (off + n < off) { ls_mutex_unlock(&s->toc_lk); return LS_ERR_RANGE; } rc = ensure_cap(s, r, off + n); if (rc != LS_OK) { - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, rc, key, off, n, "reserving space for append"); return rc; } @@ -563,13 +559,13 @@ int ls_append(ls_store *s, const char *key, size_t n, const void *buf, if (r->mem) ls_lru_touch(s, r); r->busy++; ls_place_of(r, &pl); - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); rc = ls_scatter(s, &pl, off, n, NULL, buf, LS_OP_WRITE); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r->busy--; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (rc != LS_OK) ls_report(s, rc, key, off, n, "append"); else if (off_out) *off_out = off; @@ -610,27 +606,27 @@ static int segv(ls_store *s, const char *key, const ls_seg *segs, size_t nseg, i #endif /* One lookup for the whole list. Against Conquest's "thousands of individual * scalar operations per file", that alone is most of the gain. */ - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (op == LS_OP_READ) { - if (!r) { pthread_mutex_unlock(&s->toc_lk); + if (!r) { ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_NOKEY, key, 0, 0, "readv"); return LS_ERR_NOKEY; } - if (hi > r->size) { pthread_mutex_unlock(&s->toc_lk); + if (hi > r->size) { ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_RANGE, key, 0, 0, "readv"); return LS_ERR_RANGE; } } else { if (s->o.parallel == LS_SHARED && (!r || hi > r->size)) { - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_MODE, key, 0, hi, "a shared store's layout is frozen; reserve it before sharing"); return LS_ERR_MODE; } if (!r) r = ls_toc_insert(s, key); - if (!r) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!r) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } rc = ensure_cap(s, r, hi); - if (rc != LS_OK) { pthread_mutex_unlock(&s->toc_lk); + if (rc != LS_OK) { ls_mutex_unlock(&s->toc_lk); ls_report(s, rc, key, 0, 0, "reserving space for writev"); return rc; } if (hi > r->size) r->size = hi; @@ -638,16 +634,16 @@ static int segv(ls_store *s, const char *key, const ls_seg *segs, size_t nseg, i if (r->mem) ls_lru_touch(s, r); r->busy++; ls_place_of(r, &pl); - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); for (i = 0; i < nseg && rc == LS_OK; i++) rc = ls_scatter(s, &pl, segs[i].off, segs[i].len, op == LS_OP_READ ? segs[i].buf : NULL, op == LS_OP_READ ? NULL : segs[i].buf, op); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r->busy--; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (rc != LS_OK) ls_report(s, rc, key, 0, 0, op == LS_OP_READ ? "readv" : "writev"); @@ -671,13 +667,13 @@ int ls_set_attr(ls_store *s, const char *key, const void *blob, size_t n) if (s->o.parallel == LS_SHARED) return LS_ERR_MODE; LS_IF_HDF5(s, ls_h5_set_attr(s, key, blob, n)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (!r) r = ls_toc_insert(s, key); - if (!r) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!r) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } if (n) memcpy(r->attr, blob, n); r->attrlen = (uint32_t)n; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return LS_OK; } @@ -689,7 +685,7 @@ int ls_get_attr(ls_store *s, const char *key, void *blob, size_t *n) if (!s || !ls_key_ok(key) || !n) return LS_ERR_INVAL; LS_IF_HDF5(s, ls_h5_get_attr(s, key, blob, n)); - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (!r) { rc = LS_ERR_NOKEY; @@ -702,7 +698,7 @@ int ls_get_attr(ls_store *s, const char *key, void *blob, size_t *n) if (r->attrlen) memcpy(blob, r->attr, r->attrlen); *n = r->attrlen; } - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return rc; } @@ -741,19 +737,19 @@ int ls_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, if (off + n < off) return LS_ERR_RANGE; if (n == 0) return LS_OK; - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (s->o.parallel == LS_SHARED && (!r || off + n > r->size)) { - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, LS_ERR_MODE, key, off, n, "a shared store's layout is frozen; reserve it before sharing"); return LS_ERR_MODE; } if (!r) r = ls_toc_insert(s, key); - if (!r) { pthread_mutex_unlock(&s->toc_lk); return -ENOMEM; } + if (!r) { ls_mutex_unlock(&s->toc_lk); return -ENOMEM; } rc = ensure_cap(s, r, off + n); if (rc != LS_OK) { - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, rc, key, off, n, "reserving space for accumulate"); return rc; } @@ -761,7 +757,7 @@ int ls_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, if (r->mem) ls_lru_touch(s, r); r->busy++; ls_place_of(r, &pl); - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (pl.mem) { /* The case §4 exists for: resident, so the reduction runs in place and @@ -787,9 +783,9 @@ int ls_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, } } - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r->busy--; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (rc != LS_OK) ls_report(s, rc, key, off, n, "accumulate"); return rc; @@ -812,6 +808,7 @@ int ls_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, * and does not attempt to hide. It cannot: a fault happens in the caller's * instruction stream, not inside a libspill call. */ +#ifdef LS_HAVE_MMAP int ls_map(ls_store *s, const char *key, void **addr, size_t *len) { ls_rec *r; @@ -824,24 +821,24 @@ int ls_map(ls_store *s, const char *key, void **addr, size_t *len) if (!s || !ls_key_ok(key) || !addr) return LS_ERR_INVAL; if (s->o.mode != LS_MAPPED) return LS_ERR_MODE; - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (!r || r->size == 0) { - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); ls_report(s, r ? LS_ERR_RANGE : LS_ERR_NOKEY, key, 0, 0, "map"); return r ? LS_ERR_RANGE : LS_ERR_NOKEY; } if (r->map_addr) { /* already mapped: hand it back */ *addr = r->map_addr; if (len) *len = (size_t)r->size; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); return LS_OK; } ls_place_of(r, &pl); for (i = 0; i < pl.next; i++) total += pl.ext[i].len; logical = r->size; r->busy++; - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); base = mmap(NULL, (size_t)total, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); @@ -859,13 +856,13 @@ int ls_map(ls_store *s, const char *key, void **addr, size_t *len) if (rc != LS_OK) { munmap(base, (size_t)total); base = MAP_FAILED; } } - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r->busy--; if (rc == LS_OK) { r->map_addr = base; r->map_len = (size_t)total; } - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (rc != LS_OK) { ls_report(s, rc, key, 0, 0, "map"); return rc; } *addr = base; @@ -882,7 +879,7 @@ int ls_unmap(ls_store *s, const char *key) if (!s || !ls_key_ok(key)) return LS_ERR_INVAL; if (s->o.mode != LS_MAPPED) return LS_ERR_MODE; - pthread_mutex_lock(&s->toc_lk); + ls_mutex_lock(&s->toc_lk); r = ls_toc_find(s, key); if (r && r->map_addr) { a = r->map_addr; @@ -890,9 +887,26 @@ int ls_unmap(ls_store *s, const char *key) r->map_addr = NULL; r->map_len = 0; } - pthread_mutex_unlock(&s->toc_lk); + ls_mutex_unlock(&s->toc_lk); if (!r) return LS_ERR_NOKEY; if (!a) return LS_OK; /* not mapped: nothing to undo */ return munmap(a, n) == 0 ? LS_OK : -errno; } +#else +/* No mapping on this platform -- see os.h. ls_open refuses LS_MAPPED, so a + * store can never be in the mode these require; they answer the way they + * already would for a store opened in any other mode. */ +int ls_map(ls_store *s, const char *key, void **addr, size_t *len) +{ + (void)addr; (void)len; + if (!s || !ls_key_ok(key)) return LS_ERR_INVAL; + return LS_ERR_MODE; +} + +int ls_unmap(ls_store *s, const char *key) +{ + if (!s || !ls_key_ok(key)) return LS_ERR_INVAL; + return LS_ERR_MODE; +} +#endif diff --git a/src/toc.c b/src/toc.c index db47a6f..8859da7 100644 --- a/src/toc.c +++ b/src/toc.c @@ -1,11 +1,14 @@ /* SPDX-License-Identifier: BSD-3-Clause */ #include -#include #include #include #include "internal.h" +#ifdef LS_HAVE_MMAP +#include +#endif + int ls_key_ok(const char *key) { size_t n; @@ -86,7 +89,9 @@ void ls_toc_unlink(ls_store *s, ls_rec *r) void ls_rec_free(ls_store *s, ls_rec *r) { if (!r) return; +#ifdef LS_HAVE_MMAP if (r->map_addr) munmap(r->map_addr, r->map_len); /* close unmaps */ +#endif if (r->mem) { if (s->resident >= r->mem_cap) s->resident -= (size_t)r->mem_cap; free(r->mem); diff --git a/tests/test_mapped.c b/tests/test_mapped.c index fa63d8c..792d748 100644 --- a/tests/test_mapped.c +++ b/tests/test_mapped.c @@ -75,6 +75,16 @@ int main(void) ok(ls_open("m_bad", &o, &err) == NULL && err == LS_ERR_INVAL, "O_DIRECT and mmap together are refused"); + /* Everything above is option validation, which holds everywhere. The rest + * needs mapping to exist. Where it does not -- Windows today -- ls_open + * says so with LS_ERR_MODE, and there is nothing here left to test. */ + s = open_mapped("m_probe", &err); + if (!s && err == LS_ERR_MODE) { + puts(" [SKIP] mapping is not supported on this platform"); + return 77; + } + if (s) ls_close(s, 0); + /* A single-extent record. */ s = open_mapped("m_one", &err); ok(s != NULL, "open a mapped store"); diff --git a/tests/test_posix.c b/tests/test_posix.c index 4bc2a8d..e7c86e5 100644 --- a/tests/test_posix.c +++ b/tests/test_posix.c @@ -132,8 +132,13 @@ static void t_options(void) ls_opts_default(&o); o.mode = LS_MAPPED; s = ls_open("t_opt_m", &o, &err); - ok(s != NULL && err == LS_OK, "LS_MAPPED store opens"); - if (s) ok_rc(ls_close(s, 0), LS_OK, " ... and closes"); + if (!s && err == LS_ERR_MODE) { + ok(1, "LS_MAPPED is refused with LS_ERR_MODE (no mapping on this platform)"); + ok(1, " ... which is the documented answer where mapping is absent"); + } else { + ok(s != NULL && err == LS_OK, "LS_MAPPED store opens"); + if (s) ok_rc(ls_close(s, 0), LS_OK, " ... and closes"); + } /* the combination that is wrong in principle outranks the one that is * merely not built yet, so this stays stable when LS_MAPPED lands */ From 65e71d8a936cb5717f6df6468b41e82cb1ef09d0 Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 17:43:30 +0300 Subject: [PATCH 2/8] Fix the three things the new Windows job found The job earned its keep on its first run: the port compiled almost entirely under both MSVC and clang-cl, and then failed at the link. * strerror_r and posix_memalign were still being called directly. Both are POSIX-only and both belong in the floor: ls_os_strerror hides the fact that glibc's strerror_r returns char*, XSI's returns int and the MSVC CRT spells it strerror_s with the arguments the other way round, and ls_os_aligned_alloc pairs with ls_os_aligned_free -- which is not decoration, since memory from _aligned_malloc must never reach free(). * warning C4141: 'dllexport' used more than once. A stray duplicated LS_API on ls_opts_init, harmless while LS_API expanded to nothing on Windows and to a repeated visibility attribute on GCC. Making LS_API mean dllexport is what turned it into a diagnostic. Found by a sweep for the rest of the POSIX-only surface at the same time, so this should be the last link round rather than the first of three: strdup is the only other non-C89 name left and the MSVC CRT provides it (the failing build reached the link, so it had already resolved). POSIX is still unaffected: ctest 9/9, the LS_NO_MMAP configuration 8/9 with test_mapped skipped, and make check green end to end. Co-Authored-By: Claude Opus 5 (1M context) --- include/libspill.h | 2 +- src/error.c | 15 +++------------ src/os.h | 14 ++++++++++++++ src/os_posix.c | 26 ++++++++++++++++++++++++++ src/os_win32.c | 15 +++++++++++++++ src/store.c | 15 ++++----------- 6 files changed, 63 insertions(+), 24 deletions(-) diff --git a/include/libspill.h b/include/libspill.h index 8943287..125a913 100644 --- a/include/libspill.h +++ b/include/libspill.h @@ -193,7 +193,7 @@ typedef struct { * header keeps working against a newer library. Always obtain an ls_opts from * ls_opts_default rather than declaring one and filling it in. */ -LS_API /* Fills o with defaults. Call it through the ls_opts_default macro below, which +/* Fills o with defaults. Call it through the ls_opts_default macro below, which * passes the version YOUR header declares -- the library then writes only the * fields that version defines, so linking a newer libspill against a struct * compiled from an older header cannot write past its end. */ diff --git a/src/error.c b/src/error.c index 55f0fc4..d20d34d 100644 --- a/src/error.c +++ b/src/error.c @@ -33,18 +33,9 @@ const char *ls_strerror(int err, char *buf, size_t buflen) if (m) { snprintf(buf, buflen, "%s", m); } else if (err < 0 && err > -1000) { - /* The negated-errno range. strerror_r has two incompatible flavours -- - * GNU returns char* and may not touch the buffer, XSI returns int -- - * and getting this wrong prints uninitialised stack. */ - char sys[96]; -#if defined(__GLIBC__) && defined(_GNU_SOURCE) - const char *p = strerror_r(-err, sys, sizeof sys); - snprintf(buf, buflen, "%s", p ? p : "unknown error"); -#else - if (strerror_r(-err, sys, sizeof sys) != 0) - snprintf(sys, sizeof sys, "errno %d", -err); - snprintf(buf, buflen, "%s", sys); -#endif + /* The negated-errno range. Which strerror this is, and how it reports, + * is the platform floor's problem -- see ls_os_strerror. */ + ls_os_strerror(-err, buf, buflen); } else { snprintf(buf, buflen, "unknown error %d", err); } diff --git a/src/os.h b/src/os.h index 25dc472..3eb3c78 100644 --- a/src/os.h +++ b/src/os.h @@ -72,6 +72,20 @@ int ls_os_getpid(void); * is always correct and merely slower. */ int ls_os_zero_range(int fd, uint64_t off, uint64_t len); +/* Fill buf with the system's text for errnum, always NUL-terminated and always + * something. This is in the floor because the three implementations disagree + * about almost everything: glibc's strerror_r returns char* and may not touch + * the buffer at all, XSI's returns int, and the MSVC CRT spells it + * strerror_s with the arguments in a different order. Getting it wrong prints + * uninitialised stack. */ +void ls_os_strerror(int errnum, char *buf, size_t buflen); + +/* LS_ALIGN-aligned allocation, for the uncached-I/O bounce buffers. The free + * has to match the alloc: memory from Windows' _aligned_malloc must never + * reach free(), and it does not fault until much later if it does. */ +void *ls_os_aligned_alloc(size_t align, size_t n); +void ls_os_aligned_free(void *p); + /* ----------------------------------------------------------------- threads * * A mutex, a condition variable and a joinable thread: the whole of what this diff --git a/src/os_posix.c b/src/os_posix.c index f2e308d..6394828 100644 --- a/src/os_posix.c +++ b/src/os_posix.c @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include #include @@ -78,6 +80,30 @@ int ls_os_zero_range(int fd, uint64_t off, uint64_t len) #endif } +void ls_os_strerror(int errnum, char *buf, size_t buflen) +{ + if (buflen == 0) return; +#if defined(__GLIBC__) && defined(_GNU_SOURCE) + { /* GNU: returns the text, which may or may not be in buf */ + const char *p = strerror_r(errnum, buf, buflen); + if (p != buf) snprintf(buf, buflen, "%s", p ? p : "unknown error"); + } +#else + if (strerror_r(errnum, buf, buflen) != 0) + snprintf(buf, buflen, "errno %d", errnum); +#endif + buf[buflen - 1] = '\0'; +} + +void *ls_os_aligned_alloc(size_t align, size_t n) +{ + void *p = NULL; + if (posix_memalign(&p, align, n) != 0) return NULL; + return p; +} + +void ls_os_aligned_free(void *p) { free(p); } + /* ----------------------------------------------------------------- threads */ void ls_mutex_init(ls_mutex *m) { pthread_mutex_init(m, NULL); } diff --git a/src/os_win32.c b/src/os_win32.c index d6f17c7..c4ef149 100644 --- a/src/os_win32.c +++ b/src/os_win32.c @@ -15,8 +15,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -184,6 +186,19 @@ int ls_os_zero_range(int fd, uint64_t off, uint64_t len) return -1; } +void ls_os_strerror(int errnum, char *buf, size_t buflen) +{ + if (buflen == 0) return; + if (strerror_s(buf, buflen, errnum) != 0) + snprintf(buf, buflen, "errno %d", errnum); + buf[buflen - 1] = '\0'; +} + +/* _aligned_malloc, not malloc: the CRT tracks the adjustment it made, which is + * why the matching _aligned_free is not optional. */ +void *ls_os_aligned_alloc(size_t align, size_t n) { return _aligned_malloc(n, align); } +void ls_os_aligned_free(void *p) { _aligned_free(p); } + /* ----------------------------------------------------------------- threads * * SRWLOCK and CONDITION_VARIABLE need no destruction and cannot fail to diff --git a/src/store.c b/src/store.c index 8e30eb2..b203cb6 100644 --- a/src/store.c +++ b/src/store.c @@ -17,13 +17,6 @@ /* ------------------------------------------------------------- raw file I/O */ -static void *aligned_alloc_or_null(size_t n) -{ - void *p = NULL; - if (posix_memalign(&p, LS_ALIGN, n) != 0) return NULL; - return p; -} - /* Retries short transfers, so the data calls are all-or-nothing to the caller * as §4b promises. Under O_DIRECT everything is staged through an aligned * bounce buffer: correctness first, and the benchmark's reason for wanting @@ -59,7 +52,7 @@ static int rw_all(ls_store *s, void *buf, const void *cbuf, size_t n, } { /* O_DIRECT: offset, length and buffer must all be LS_ALIGN-aligned */ - unsigned char *bb = aligned_alloc_or_null(LS_BOUNCE); + unsigned char *bb = ls_os_aligned_alloc(LS_ALIGN, LS_BOUNCE); int rc = LS_OK; if (!bb) return -ENOMEM; @@ -90,7 +83,7 @@ static int rw_all(ls_store *s, void *buf, const void *cbuf, size_t n, if (r < 0) { rc = (errno == EINTR) ? LS_OK : -errno; if (rc) break; continue; } done += chunk; } - free(bb); + ls_os_aligned_free(bb); return rc; } } @@ -112,7 +105,7 @@ static int zero_range(ls_store *s, uint64_t off, uint64_t len) if (!s->direct && ls_os_zero_range(s->fd, off, len) == 0) return LS_OK; - z = aligned_alloc_or_null(CH); + z = ls_os_aligned_alloc(LS_ALIGN, CH); if (!z) return -ENOMEM; memset(z, 0, CH); while (len && rc == LS_OK) { @@ -121,7 +114,7 @@ static int zero_range(ls_store *s, uint64_t off, uint64_t len) off += k; len -= k; } - free(z); + ls_os_aligned_free(z); return rc; } From 1eb300ef6c9f3a3dbbc33b1708923ab818a77e70 Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 17:48:17 +0300 Subject: [PATCH 3/8] Port the test programs to Windows too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The library builds and links on Windows now; this round was entirely the tests, which the first sweep did not cover -- it looked at src/ and stopped there. * pthreads in test_posix and test_surveyed. src/os.h cannot help: it is internal and its implementation is not exported, so a test linking only the public library cannot call it. tests/thread_compat.h gives the three primitives the tests actually use, with the pthread signature on both sides so the thread bodies are unchanged. * fork in test_surveyed. §3a(1) is a genuinely multi-process property and fork is the barrier; a CreateProcess version would be this binary re-executing itself with an argument, which is a different test rather than the same one ported. That half is split into its own function and skipped where there is no fork, saying so. The option refusals that followed it in the same function are portable and now always run -- previously they would have been lost with it. * EDQUOT in abi_header_test: POSIX, not C, and absent from the MSVC CRT. The property is about the errnos that exist, so it is checked for those. * A real bug, not a portability shim: churn_libspill and test_hdf5 declare a 64-bit LCG as unsigned long, which is 32 bits on Windows -- the constants truncate and `rs >> 33` is undefined. MSVC said so (C4293). uint64_t is what it always meant; on Linux this changes nothing. unlink -> remove in test_posix, which is C89 and does the same thing here. Linux unaffected: ctest 9/9 and make check green end to end. Co-Authored-By: Claude Opus 5 (1M context) --- tests/abi_header_test.c | 7 +++++ tests/churn_libspill.c | 9 ++++-- tests/test_hdf5.c | 9 ++++-- tests/test_posix.c | 11 ++++--- tests/test_surveyed.c | 40 ++++++++++++++++++++++---- tests/thread_compat.h | 63 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 121 insertions(+), 18 deletions(-) create mode 100644 tests/thread_compat.h diff --git a/tests/abi_header_test.c b/tests/abi_header_test.c index 6e3d237..78cc3b3 100644 --- a/tests/abi_header_test.c +++ b/tests/abi_header_test.c @@ -50,9 +50,16 @@ int main(void) /* 3. The errnos §4b promises to pass through unchanged must be nameable and * must not collide with anything of ours. */ +#ifdef EDQUOT sprintf(detail, "ENOSPC=%d EIO=%d EDQUOT=%d", ENOSPC, EIO, EDQUOT); check(ENOSPC < 1000 && EIO < 1000 && EDQUOT < 1000, "passed-through errnos are in range", detail); +#else + /* EDQUOT is POSIX, not C: the MSVC CRT has no such errno. The property + * being checked is about the ones that exist. */ + sprintf(detail, "ENOSPC=%d EIO=%d (no EDQUOT on this platform)", ENOSPC, EIO); + check(ENOSPC < 1000 && EIO < 1000, "passed-through errnos are in range", detail); +#endif /* 4. version is first in ls_opts, which is what lets the struct grow * without breaking a caller built against an older header. */ diff --git a/tests/churn_libspill.c b/tests/churn_libspill.c index b0ccc3d..afeccd9 100644 --- a/tests/churn_libspill.c +++ b/tests/churn_libspill.c @@ -8,6 +8,7 @@ * disqualifying for a scratch heap. Best-fit with coalescing is the answer; * this is the number that says whether it worked. */ +#include #include #include #include @@ -20,9 +21,11 @@ #define LO (1 << 14) #define HI (1 << 18) -static unsigned long rs = 1; -static unsigned long nextr(void) { rs = rs * 6364136223846793005UL + 1442695040888963407UL; - return (rs >> 33); } +/* A 64-bit LCG, so it must say so: unsigned long is 32 bits on Windows, where + * the constants would truncate and rs >> 33 is undefined. */ +static uint64_t rs = 1; +static uint64_t nextr(void) { rs = rs * 6364136223846793005ULL + 1442695040888963407ULL; + return (rs >> 33); } static size_t pick(void) { return (size_t)(LO + nextr() % (HI - LO)); } int main(void) diff --git a/tests/test_hdf5.c b/tests/test_hdf5.c index 5c57e8e..ad7b4a6 100644 --- a/tests/test_hdf5.c +++ b/tests/test_hdf5.c @@ -6,6 +6,7 @@ * checked -- and that §7b's reason for keeping HDF5 optional still holds, by * running the same churn protocol through both and comparing. */ +#include #include #include #include @@ -35,9 +36,11 @@ static void ok_rc(int rc, int want, const char *what) } } -static unsigned long rs = 1; -static unsigned long nextr(void) { rs = rs * 6364136223846793005UL + 1442695040888963407UL; - return (rs >> 33); } +/* A 64-bit LCG, so it must say so: unsigned long is 32 bits on Windows, where + * the constants would truncate and rs >> 33 is undefined. */ +static uint64_t rs = 1; +static uint64_t nextr(void) { rs = rs * 6364136223846793005ULL + 1442695040888963407ULL; + return (rs >> 33); } /* The protocol of tests/churn_libspill.c and tests/hdf5_churn_varsize.py: * 8 records, 60 cycles, each recreated at a different random size. */ diff --git a/tests/test_posix.c b/tests/test_posix.c index e7c86e5..3fc51f5 100644 --- a/tests/test_posix.c +++ b/tests/test_posix.c @@ -2,12 +2,11 @@ /* Correctness suite for the POSIX backend: the §4b contract, the memory tier, * the async layer, and the concurrency §5a promises. */ #include -#include +#include "thread_compat.h" #include #include #include #include -#include #include "libspill.h" @@ -327,7 +326,7 @@ static void *hammer(void *p) static void t_concurrent(void) { ls_store *s = fresh("t_conc", 0); - pthread_t th[NTHREAD]; + ls_test_thread th[NTHREAD]; struct arg ar[NTHREAD]; int i, bad = 0; char **k = NULL; @@ -335,10 +334,10 @@ static void t_concurrent(void) for (i = 0; i < NTHREAD; i++) { ar[i].s = s; ar[i].id = i; ar[i].bad = 0; - pthread_create(&th[i], NULL, hammer, &ar[i]); + ls_test_thread_create(&th[i], hammer, &ar[i]); } for (i = 0; i < NTHREAD; i++) { - pthread_join(th[i], NULL); + ls_test_thread_join(th[i]); if (ar[i].bad) bad = ar[i].bad; } ok(!bad, "8 threads x 64 distinct keys: no error, no corruption"); @@ -390,7 +389,7 @@ static void t_persist(void) ls_opts_default(&o); s = ls_open("t_junk", &o, &err); ok(s == NULL && err == LS_ERR_CORRUPT, "a foreign file is refused, not clobbered"); - unlink(path); + remove(path); /* C89; unlink is POSIX-only */ } } diff --git a/tests/test_surveyed.c b/tests/test_surveyed.c index 6a8e0b8..dad6c5f 100644 --- a/tests/test_surveyed.c +++ b/tests/test_surveyed.c @@ -8,12 +8,22 @@ * §3a(5) tuple keys 3 codes -- already answered by string keys; no code */ #include -#include #include #include #include + +#include "thread_compat.h" + +/* §3a(1) is a MULTI-PROCESS property, so t_shared below uses a real fork as + * the caller's barrier. Windows has no fork, and an equivalent written with + * CreateProcess would be re-executing this binary with an argument -- a + * different test, not the same one ported. That section is skipped there and + * says so; everything else in this file runs everywhere. */ +#ifndef _WIN32 +#define LS_TEST_HAVE_FORK 1 #include #include +#endif #include "libspill.h" @@ -93,17 +103,17 @@ static void t_append(void) /* The reason this is a primitive rather than ls_size plus ls_write: with * six threads appending, every offset must still be distinct. */ { - pthread_t th[NAPP_THREAD]; + ls_test_thread th[NAPP_THREAD]; struct app ar[NAPP_THREAD]; uint64_t all[NAPP_THREAD * NAPP_EACH]; int i, j, k = 0, bad = 0, dup = 0; for (i = 0; i < NAPP_THREAD; i++) { ar[i].s = s; ar[i].id = i; ar[i].bad = 0; - pthread_create(&th[i], NULL, appender, &ar[i]); + ls_test_thread_create(&th[i], appender, &ar[i]); } for (i = 0; i < NAPP_THREAD; i++) { - pthread_join(th[i], NULL); + ls_test_thread_join(th[i]); if (ar[i].bad) bad = 1; for (j = 0; j < NAPP_EACH; j++) all[k++] = ar[i].off[j]; } @@ -216,7 +226,10 @@ static void t_attrs(void) #define NPROC 4 #define NPER 4096 -static void t_shared(void) +/* The multi-process half of §3a(1): several processes writing disjoint ranges + * of one shared record. Needs a real barrier, which here is fork. */ +#ifdef LS_TEST_HAVE_FORK +static void t_shared_procs(void) { ls_opts o; ls_store *s; @@ -273,8 +286,23 @@ static void t_shared(void) free(chk); ls_close(s, 0); } +} +#else +static void t_shared_procs(void) +{ + puts(" [SKIP] the multi-process section needs fork(); not available here"); +} +#endif + +/* The rest of §3a(1): the refusals that keep the layout frozen. Portable. */ +static void t_shared(void) +{ + ls_opts o; + ls_store *s; + int err = 0; + + t_shared_procs(); - /* The refusals that keep the layout frozen. */ ls_opts_default(&o); o.parallel = LS_SHARED; o.memory_budget = 1 << 20; diff --git a/tests/thread_compat.h b/tests/thread_compat.h new file mode 100644 index 0000000..7f99990 --- /dev/null +++ b/tests/thread_compat.h @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ +/* Threads for the test programs. + * + * src/os.h does this for the library, but it is internal and its + * implementation is not exported, so a test linking only the public library + * cannot reach it. The tests want exactly three things -- start a thread, join + * it, and nothing else -- so here they are, with the pthread signature on both + * sides to keep the thread bodies unchanged. + */ +#ifndef LS_TEST_THREAD_COMPAT_H +#define LS_TEST_THREAD_COMPAT_H + +#ifdef _WIN32 + +#define WIN32_LEAN_AND_MEAN +#include +#include +#include + +typedef HANDLE ls_test_thread; + +typedef struct { void *(*fn)(void *); void *arg; } ls_test_tramp; + +static unsigned __stdcall ls_test_thread_main(void *p) +{ + ls_test_tramp t = *(ls_test_tramp *)p; + free(p); + t.fn(t.arg); + return 0; +} + +static int ls_test_thread_create(ls_test_thread *t, void *(*fn)(void *), void *arg) +{ + ls_test_tramp *tr = (ls_test_tramp *)malloc(sizeof *tr); + uintptr_t h; + if (!tr) return -1; + tr->fn = fn; + tr->arg = arg; + h = _beginthreadex(NULL, 0, ls_test_thread_main, tr, 0, NULL); + if (h == 0) { free(tr); return -1; } + *t = (HANDLE)h; + return 0; +} + +static void ls_test_thread_join(ls_test_thread t) +{ + WaitForSingleObject(t, INFINITE); + CloseHandle(t); +} + +#else + +#include + +typedef pthread_t ls_test_thread; + +static int ls_test_thread_create(ls_test_thread *t, void *(*fn)(void *), void *arg) +{ return pthread_create(t, NULL, fn, arg); } + +static void ls_test_thread_join(ls_test_thread t) { pthread_join(t, NULL); } + +#endif +#endif /* LS_TEST_THREAD_COMPAT_H */ From 894ef5e82c629a66ca449cd75d1e0880c09c3a12 Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 17:52:22 +0300 Subject: [PATCH 4/8] Resolve the default scratch directory per platform Everything compiles and links on Windows now; this was the first round where the tests ran, and they ran straight into ENOENT from ls_open. build_path resolved its default directory as TMPDIR, then /tmp. Windows has neither: the environment variables are TMP and TEMP, and there is no /tmp to fall back to, so every store opened without an explicit dir tried to create a file in a directory that does not exist. Nothing was wrong with the I/O -- it never got that far. ls_os_tmpdir joins the rest of the floor. POSIX keeps TMPDIR then /tmp exactly as before; Windows asks GetTempPath, which is the documented resolution there (TMP, then TEMP, then the user profile, then the Windows directory) and always ends with a separator, which build_path does not want. A caller that sets ls_opts.dir -- Psi4's shim does, from PSIOManager -- never reached this path in the first place. Co-Authored-By: Claude Opus 5 (1M context) --- src/open.c | 4 ++-- src/os.h | 6 ++++++ src/os_posix.c | 8 ++++++++ src/os_win32.c | 13 +++++++++++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/open.c b/src/open.c index 176d3bb..25da99a 100644 --- a/src/open.c +++ b/src/open.c @@ -91,11 +91,11 @@ static int resolve_rank(int rank) static char *build_path(const char *name, const ls_opts *o) { const char *dir = o->dir; + char tmp[512]; char *p; size_t n; - if (!dir) dir = getenv("TMPDIR"); - if (!dir || !*dir) dir = "/tmp"; + if (!dir || !*dir) { ls_os_tmpdir(tmp, sizeof tmp); dir = tmp; } n = strlen(dir) + strlen(name) + 64; p = malloc(n); diff --git a/src/os.h b/src/os.h index 3eb3c78..7d3cab5 100644 --- a/src/os.h +++ b/src/os.h @@ -72,6 +72,12 @@ int ls_os_getpid(void); * is always correct and merely slower. */ int ls_os_zero_range(int fd, uint64_t off, uint64_t len); +/* The directory a store goes in when the caller names none. POSIX reads TMPDIR + * and falls back to /tmp; Windows has neither, and its own answer comes from + * GetTempPath (TMP, then TEMP, then the user profile). Never returns an empty + * string, and never a trailing separator. */ +void ls_os_tmpdir(char *buf, size_t buflen); + /* Fill buf with the system's text for errnum, always NUL-terminated and always * something. This is in the floor because the three implementations disagree * about almost everything: glibc's strerror_r returns char* and may not touch diff --git a/src/os_posix.c b/src/os_posix.c index 6394828..f7ff814 100644 --- a/src/os_posix.c +++ b/src/os_posix.c @@ -80,6 +80,14 @@ int ls_os_zero_range(int fd, uint64_t off, uint64_t len) #endif } +void ls_os_tmpdir(char *buf, size_t buflen) +{ + const char *d = getenv("TMPDIR"); + if (!d || !*d) d = "/tmp"; + snprintf(buf, buflen, "%s", d); + if (buflen) buf[buflen - 1] = '\0'; +} + void ls_os_strerror(int errnum, char *buf, size_t buflen) { if (buflen == 0) return; diff --git a/src/os_win32.c b/src/os_win32.c index c4ef149..a4a0200 100644 --- a/src/os_win32.c +++ b/src/os_win32.c @@ -186,6 +186,19 @@ int ls_os_zero_range(int fd, uint64_t off, uint64_t len) return -1; } +void ls_os_tmpdir(char *buf, size_t buflen) +{ + /* GetTempPath is the documented resolution -- TMP, then TEMP, then the + * user profile, then the Windows directory -- and it always ends with a + * separator, which the caller does not want. */ + DWORD n; + if (buflen == 0) return; + n = GetTempPathA((DWORD)buflen, buf); + if (n == 0 || n >= buflen) { snprintf(buf, buflen, "."); return; } + while (n > 0 && (buf[n - 1] == '\\' || buf[n - 1] == '/')) buf[--n] = '\0'; + if (n == 0) snprintf(buf, buflen, "."); +} + void ls_os_strerror(int errnum, char *buf, size_t buflen) { if (buflen == 0) return; From 66e12c787121f7dbee20aa06031998c4455236e8 Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 17:59:38 +0300 Subject: [PATCH 5/8] Let the tests state the scratch directory instead of guessing it Four checks in test_posix and test_cxx stat the file the library created, so the test and the library have to agree on its path. Each side worked it out separately -- the tests as TMPDIR then /tmp, the library as whatever the platform says -- which agreed on Linux by coincidence and stopped agreeing on Windows, where the library now asks GetTempPath. The fix is to say the directory rather than derive it twice: the tests pass ls_opts.dir explicitly and build their paths from the same constant. That removes the duplicated resolution rather than teaching it a third platform, and it is what a consumer inspecting its own scratch files would do anyway -- Psi4's shim passes the directory for exactly this reason. Everything else in that round already passed on Windows, including the one that mattered: "8 threads x 64 distinct keys: no error, no corruption", which is the OVERLAPPED positional-I/O claim actually being exercised. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_cxx.cc | 8 +++++--- tests/test_posix.c | 18 +++++++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/tests/test_cxx.cc b/tests/test_cxx.cc index 7a32361..3ebdb7f 100644 --- a/tests/test_cxx.cc +++ b/tests/test_cxx.cc @@ -123,7 +123,7 @@ int main() { // ---- the memory tier is where accumulate never touches disk ---- { - ls::store s{"cxx_mem", {.memory_budget = 8ull << 20}}; + ls::store s{"cxx_mem", {.memory_budget = 8ull << 20, .dir = "."}}; std::vector v(1024, 1.0), one(1024, 1.0); s.write("a", 0, v); for (int i = 0; i < 100; i++) s.accumulate("a", 0, one); @@ -135,8 +135,10 @@ int main() { // §4's actual claim: resident, the reduction runs in place and disk is // never touched. Only the superblock should ever have been written. - const char *td = std::getenv("TMPDIR"); - std::filesystem::path p = std::filesystem::path(td ? td : "/tmp") / "cxx_mem.libspill"; + // The path is pinned above rather than re-derived here: the library's + // default directory is the platform's, and guessing it is what broke + // this check on Windows. + std::filesystem::path p = std::filesystem::path(".") / "cxx_mem.libspill"; ok(std::filesystem::exists(p) && std::filesystem::file_size(p) == 4096, " ... and never reached disk"); } diff --git a/tests/test_posix.c b/tests/test_posix.c index 3fc51f5..2244f86 100644 --- a/tests/test_posix.c +++ b/tests/test_posix.c @@ -32,12 +32,21 @@ static void ok_rc(int rc, int want, const char *what) } } +/* Where the stores go. Several checks below stat the file the library created, + * so the test and the library must agree on its path -- and the way to agree is + * to say it, not for each side to work out the platform's default separately. + * That is what broke on Windows: the test resolved TMPDIR then /tmp while the + * library asked GetTempPath, and they had only ever agreed by coincidence. + * "." is ctest's working directory. */ +#define TEST_DIR "." + static ls_store *fresh(const char *name, size_t budget) { ls_opts o; int err = 0; ls_store *s; ls_opts_default(&o); + o.dir = TEST_DIR; o.memory_budget = budget; s = ls_open(name, &o, &err); if (!s) { printf(" [FAIL] open %s: %d\n", name, err); exit(1); } @@ -219,7 +228,6 @@ static void t_memory_tier(void) double *buf = malloc(N * sizeof *buf), *back = malloc(N * sizeof *back); ls_store *s; struct stat st; - const char *dir = getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp"; char path[512]; size_t i; @@ -231,7 +239,7 @@ static void t_memory_tier(void) memset(back, 0, N * sizeof *back); ok_rc(ls_read(s, "a", 0, N * sizeof *back, back), LS_OK, "read from the memory tier"); ok(memcmp(buf, back, N * sizeof *buf) == 0, "memory tier round-trips exactly"); - snprintf(path, sizeof path, "%s/t_mem.libspill", dir); + snprintf(path, sizeof path, "%s/t_mem.libspill", TEST_DIR); ok(stat(path, &st) == 0 && st.st_size == 4096, "nothing was written to disk while under budget"); ls_close(s, 0); @@ -243,7 +251,7 @@ static void t_memory_tier(void) memset(back, 0, N * sizeof *back); ok_rc(ls_read(s, "a", 0, N * sizeof *back, back), LS_OK, "read a spilled record"); ok(memcmp(buf, back, N * sizeof *buf) == 0, "spilled record round-trips exactly"); - snprintf(path, sizeof path, "%s/t_mem2.libspill", dir); + snprintf(path, sizeof path, "%s/t_mem2.libspill", TEST_DIR); ok(stat(path, &st) == 0 && st.st_size > 4096, "the spill reached disk"); ls_close(s, 0); @@ -379,14 +387,14 @@ static void t_persist(void) /* a file that is not one of ours must not be silently truncated */ { - const char *dir = getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp"; char path[512]; FILE *f; - snprintf(path, sizeof path, "%s/t_junk.libspill", dir); + snprintf(path, sizeof path, "%s/t_junk.libspill", TEST_DIR); f = fopen(path, "wb"); if (f) { char junk[8192]; memset(junk, 'Z', sizeof junk); fwrite(junk, 1, sizeof junk, f); fclose(f); } ls_opts_default(&o); + o.dir = TEST_DIR; s = ls_open("t_junk", &o, &err); ok(s == NULL && err == LS_ERR_CORRUPT, "a foreign file is refused, not clobbered"); remove(path); /* C89; unlink is POSIX-only */ From 9f29f0c15336e2b63800dc2be6374f0ab11f58c4 Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 18:06:14 +0300 Subject: [PATCH 6/8] Do not require a Fortran compiler to consume libspill The whole suite passes on Windows now; the only thing left failing was the packaging check, and not for a Windows reason: CMake Error at CMakeLists.txt:10 (project): No CMAKE_Fortran_COMPILER could be found. tests/consumer declared `project(consumer LANGUAGES C CXX Fortran)`, so a consumer with no Fortran compiler could not configure at all -- and it then linked libspill::spill_f unconditionally, which is not installed unless LIBSPILL_BUILD_FORTRAN was on. The library's own position is the opposite, and says so in CMakeLists.txt: the Fortran binding is optional "so that a C or C++ consumer does not need a Fortran compiler to use the library". The packaging check was contradicting the thing it exists to verify. f_use is now built when there is a Fortran compiler AND a Fortran binding to link, and skipped with a message otherwise; make check runs it only if it was built. On Linux nothing changes -- all three consumers still build and run. Verified both ways here: the ordinary build still reports "[PASS] Fortran consumer via find_package", and an install without the Fortran binding (which is what the Windows job does) configures, skips f_use, and builds the C and C++ consumers. Co-Authored-By: Claude Opus 5 (1M context) --- Makefile | 2 +- tests/consumer/CMakeLists.txt | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 63c278b..b64001f 100644 --- a/Makefile +++ b/Makefile @@ -137,7 +137,7 @@ check-install: LD_LIBRARY_PATH=$$(dirname $$(find $(INSTALL_TEST_DIR)/prefix -name 'libspill.so.0.*')) \ sh -c '$(INSTALL_TEST_DIR)/consumer/c_use && \ $(INSTALL_TEST_DIR)/consumer/cxx_use && \ - $(INSTALL_TEST_DIR)/consumer/f_use' && \ + { [ ! -x $(INSTALL_TEST_DIR)/consumer/f_use ] || $(INSTALL_TEST_DIR)/consumer/f_use; }' && \ rm -rf $(INSTALL_TEST_DIR) check-clean: diff --git a/tests/consumer/CMakeLists.txt b/tests/consumer/CMakeLists.txt index 3434a3a..91894ee 100644 --- a/tests/consumer/CMakeLists.txt +++ b/tests/consumer/CMakeLists.txt @@ -7,7 +7,7 @@ # OpenMolcas would, in all three compiled languages, and nothing here refers to # the libspill source tree. cmake_minimum_required(VERSION 3.16) -project(consumer LANGUAGES C CXX Fortran) +project(consumer LANGUAGES C CXX) find_package(libspill 0.1 REQUIRED) add_executable(c_use c_use.c) @@ -17,5 +17,18 @@ add_executable(cxx_use cxx_use.cc) target_link_libraries(cxx_use PRIVATE libspill::spill) target_compile_features(cxx_use PRIVATE cxx_std_20) -add_executable(f_use f_use.F90) -target_link_libraries(f_use PRIVATE libspill::spill_f) +# The Fortran consumer only where there is something for it to consume. The +# library itself takes this position already -- LIBSPILL_BUILD_FORTRAN is off by +# default "so that a C or C++ consumer does not need a Fortran compiler" -- and +# this file was contradicting it by demanding one from every consumer, which is +# how a Windows runner with no gfortran fails a packaging check that has nothing +# to do with Fortran. +include(CheckLanguage) +check_language(Fortran) +if(CMAKE_Fortran_COMPILER AND TARGET libspill::spill_f) + enable_language(Fortran) + add_executable(f_use f_use.F90) + target_link_libraries(f_use PRIVATE libspill::spill_f) +else() + message(STATUS "No Fortran compiler or no libspill Fortran binding: skipping f_use") +endif() From 5fb7085dcee3dde62416ce6076ab18114cbdb1e0 Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Sun, 6 Sep 2026 18:18:41 +0300 Subject: [PATCH 7/8] Put the mapping family behind the floor too, and the path separator Review is right that the invariant was stated more strongly than it held: "nothing else in the library names a system call" was not true of mmap/munmap, which stayed in store.c and toc.c behind #ifdef LS_HAVE_MMAP. Of the two ways out offered -- narrow the claim, or make it true -- this takes the second. The claim was the useful thing. ls_os_map_extents and ls_os_unmap now live in the floor. They are declared in internal.h rather than os.h because they speak in ls_extent, and only os_posix.c defines them, under LS_HAVE_MMAP. store.c keeps the policy (which record, the busy count, what goes in the table of contents) and hands the reserve-then-MAP_FIXED sequence to the platform, which is where a Windows implementation would slot in if VirtualAlloc2 + MapViewOfFile3 is ever worth carrying. The reviewer's grep now returns nothing. Also LS_PATH_SEP, for the same reason the review gives: Windows accepts '/' so this fixes no bug, but a floor that abstracts the temp directory and then hardcodes the separator is only half a floor. Not changed, deliberately: LS_ERR_MODE rather than LS_ERR_BACKEND for a build without mapping. The review makes the better argument -- LS_ERR_BACKEND is what LS_HDF5 gets for "not compiled in" -- and also says it is not worth churning without a caller asking. Agreed on both counts; noting it here so the next person finds the reasoning rather than the omission. Verified: ctest 9/9 with test_mapped passing (so the moved mapping code is exercised, not merely compiled), LS_NO_MMAP 9/9 with test_mapped skipped, and make check green end to end. Co-Authored-By: Claude Opus 5 (1M context) --- src/internal.h | 10 ++++++++++ src/open.c | 6 +++--- src/os.h | 18 +++++++++++++++--- src/os_posix.c | 38 ++++++++++++++++++++++++++++++++++++++ src/store.c | 27 +++++++-------------------- src/toc.c | 6 +----- 6 files changed, 74 insertions(+), 31 deletions(-) diff --git a/src/internal.h b/src/internal.h index c1c4a7c..e8bfaf4 100644 --- a/src/internal.h +++ b/src/internal.h @@ -142,6 +142,16 @@ typedef struct { void ls_place_of(const ls_rec *r, ls_place *p); /* toc_lk held */ int ls_scatter(ls_store *s, const ls_place *p, uint64_t off, size_t n, void *rbuf, const void *wbuf, int op); +/* Mapping, only where the platform has it. ls_os_map_extents reserves `total` + * bytes and maps each extent over its own slice, so a record that is a list of + * extents can still be handed to the caller as one pointer; it returns NULL + * with *err set on failure. Declared here rather than in os.h because it speaks + * in ls_extent. */ +#ifdef LS_HAVE_MMAP +void *ls_os_map_extents(int fd, const ls_extent *ext, size_t n, uint64_t total, int *err); +int ls_os_unmap(void *addr, size_t len); +#endif + int ls_pread_all (ls_store *s, void *buf, size_t n, uint64_t off); int ls_pwrite_all (ls_store *s, const void *buf, size_t n, uint64_t off); diff --git a/src/open.c b/src/open.c index 25da99a..c66d263 100644 --- a/src/open.c +++ b/src/open.c @@ -102,13 +102,13 @@ static char *build_path(const char *name, const ls_opts *o) if (!p) return NULL; if (o->exact_name) - snprintf(p, n, "%s/%s", dir, name); + snprintf(p, n, "%s" LS_PATH_SEP "%s", dir, name); /* LS_SHARED deliberately does NOT fold in the rank: every process must * name the same file. */ else if (o->parallel == LS_PER_RANK) - snprintf(p, n, "%s/%s.r%d.libspill", dir, name, resolve_rank(o->rank)); + snprintf(p, n, "%s" LS_PATH_SEP "%s.r%d.libspill", dir, name, resolve_rank(o->rank)); else - snprintf(p, n, "%s/%s.libspill", dir, name); + snprintf(p, n, "%s" LS_PATH_SEP "%s.libspill", dir, name); return p; } diff --git a/src/os.h b/src/os.h index 7d3cab5..dad2f87 100644 --- a/src/os.h +++ b/src/os.h @@ -2,9 +2,12 @@ /* libspill -- the platform floor. Internal; not installed. * * Everything the library asks of the operating system, in one place: the file - * primitives, threading, and whether mapping exists at all. POSIX and Win32 - * implement it in os_posix.c and os_win32.c respectively, and nothing else in - * the library names a system call. + * primitives, threading, and mapping. POSIX and Win32 implement it in + * os_posix.c and os_win32.c respectively, and nothing else in the library names + * a system call -- mapping included, since ls_map's reserve-then-MAP_FIXED + * sequence is exactly the kind of thing that would otherwise sit in store.c + * with a platform guard around it. The mapping entry points are declared in + * internal.h rather than here, because they speak in ls_extent. * * Two rules the Win32 side has to keep, because breaking either would compile * silently and be wrong at run time: @@ -46,6 +49,15 @@ # define LS_HAVE_MMAP 1 #endif +/* The separator to build a path with. Windows accepts '/' in its file APIs, so + * this is tidiness rather than a fix -- but a platform floor that abstracts the + * temp directory and then hardcodes the separator is only half a floor. */ +#ifdef _WIN32 +# define LS_PATH_SEP "\\" +#else +# define LS_PATH_SEP "/" +#endif + /* ------------------------------------------------------------------- files */ /* Open (creating if needed) for read and write. want_direct asks for diff --git a/src/os_posix.c b/src/os_posix.c index f7ff814..a759e51 100644 --- a/src/os_posix.c +++ b/src/os_posix.c @@ -13,8 +13,10 @@ #include #include #include +#include #include +#include "internal.h" #include "os.h" int ls_os_open_rw(const char *path, int want_direct, int *got_direct) @@ -112,6 +114,42 @@ void *ls_os_aligned_alloc(size_t align, size_t n) void ls_os_aligned_free(void *p) { free(p); } +/* A record is a list of extents, so it is not contiguous in the file and cannot + * be handed to one mmap call. It can still be handed to the caller as one + * pointer: reserve the whole logical span with an anonymous PROT_NONE mapping, + * then map each extent over its own slice with MAP_FIXED. Extents are + * LS_ALIGN-aligned and LS_ALIGN-sized, which is what makes this legal, and is a + * reason that alignment is worth keeping beyond O_DIRECT. */ +void *ls_os_map_extents(int fd, const ls_extent *ext, size_t n, uint64_t total, int *err) +{ + unsigned char *base; + uint64_t at = 0; + size_t i; + int rc = 0; + + base = mmap(NULL, (size_t)total, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (base == MAP_FAILED) { if (err) *err = -errno; return NULL; } + + for (i = 0; i < n && rc == 0; i++) { + void *want = base + at; + void *got = mmap(want, (size_t)ext[i].len, PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_FIXED, fd, (off_t)ext[i].foff); + if (got == MAP_FAILED) rc = -errno; + at += ext[i].len; + } + if (rc != 0) { + munmap(base, (size_t)total); + if (err) *err = rc; + return NULL; + } + return base; +} + +int ls_os_unmap(void *addr, size_t len) +{ + return munmap(addr, len) == 0 ? 0 : -errno; +} + /* ----------------------------------------------------------------- threads */ void ls_mutex_init(ls_mutex *m) { pthread_mutex_init(m, NULL); } diff --git a/src/store.c b/src/store.c index b203cb6..0cbbfdc 100644 --- a/src/store.c +++ b/src/store.c @@ -792,10 +792,8 @@ int ls_accumulate(ls_store *s, const char *key, uint64_t off, size_t n, * * A record is a list of extents, so it is not contiguous in the file and cannot * be handed to one mmap call. It can still be handed to the caller as one - * pointer: reserve the whole logical span with an anonymous PROT_NONE mapping, - * then map each extent over its own slice with MAP_FIXED. Extents are - * LS_ALIGN-aligned and LS_ALIGN-sized, which is what makes this legal, and is - * a reason that alignment is worth keeping beyond O_DIRECT. + * pointer, and how that is done is the platform's business rather than this + * file's: ls_os_map_extents. * * Errors under a mapping arrive as SIGBUS, which §3 says the library documents * and does not attempt to hide. It cannot: a fault happens in the caller's @@ -833,21 +831,7 @@ int ls_map(ls_store *s, const char *key, void **addr, size_t *len) r->busy++; ls_mutex_unlock(&s->toc_lk); - base = mmap(NULL, (size_t)total, PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - if (base == MAP_FAILED) { - rc = -errno; - } else { - uint64_t at = 0; - for (i = 0; i < pl.next && rc == LS_OK; i++) { - void *want = base + at; - void *got = mmap(want, (size_t)pl.ext[i].len, PROT_READ | PROT_WRITE, - MAP_SHARED | MAP_FIXED, s->fd, (off_t)pl.ext[i].foff); - if (got == MAP_FAILED) rc = -errno; - at += pl.ext[i].len; - } - if (rc != LS_OK) { munmap(base, (size_t)total); base = MAP_FAILED; } - } + base = ls_os_map_extents(s->fd, pl.ext, pl.next, total, &rc); ls_mutex_lock(&s->toc_lk); r->busy--; @@ -884,7 +868,10 @@ int ls_unmap(ls_store *s, const char *key) if (!r) return LS_ERR_NOKEY; if (!a) return LS_OK; /* not mapped: nothing to undo */ - return munmap(a, n) == 0 ? LS_OK : -errno; + { + int urc = ls_os_unmap(a, n); + return urc == 0 ? LS_OK : urc; + } } #else /* No mapping on this platform -- see os.h. ls_open refuses LS_MAPPED, so a diff --git a/src/toc.c b/src/toc.c index 8859da7..4a15653 100644 --- a/src/toc.c +++ b/src/toc.c @@ -5,10 +5,6 @@ #include "internal.h" -#ifdef LS_HAVE_MMAP -#include -#endif - int ls_key_ok(const char *key) { size_t n; @@ -90,7 +86,7 @@ void ls_rec_free(ls_store *s, ls_rec *r) { if (!r) return; #ifdef LS_HAVE_MMAP - if (r->map_addr) munmap(r->map_addr, r->map_len); /* close unmaps */ + if (r->map_addr) ls_os_unmap(r->map_addr, r->map_len); /* close unmaps */ #endif if (r->mem) { if (s->resident >= r->mem_cap) s->resident -= (size_t)r->mem_cap; From 4da3591c7203b80d4ce38cc97cbd7b5dc2d6476d Mon Sep 17 00:00:00 2001 From: Susi Lehtola Date: Mon, 7 Sep 2026 19:42:34 +0300 Subject: [PATCH 8/8] Extend the file with a write, not a truncate The merge resolution had zero_range call ls_os_ftruncate to reach the end of a new extent. That is not safe here: reads and writes run outside toc_lk on purpose, so a truncate is not serialised against a thread that has already grown the file past the length we computed, and it cuts that thread's data off. On Windows it is worse, because _chsize_s zero-fills as it grows, so it clobbers rather than truncates. The Windows CI caught it as a failure in the 8-thread test; the same race exists on Linux and merely did not show. Extend with one zero byte at the last offset instead. That byte is inside the extent being zeroed, so no other thread can own it, and a positional write cannot shrink the file. Skipping the erase entirely when the extent starts past the end of the file also spares the zero-write that platforms without a zeroing primitive were doing over every new tail extent. Co-Authored-By: Claude Opus 5 (1M context) --- src/store.c | 56 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/src/store.c b/src/store.c index bb824d6..36e0556 100644 --- a/src/store.c +++ b/src/store.c @@ -98,39 +98,47 @@ int ls_pwrite_all(ls_store *s, const void *buf, size_t n, uint64_t off) * zero, but a reused hole holds whatever the previous record left there, and * §4b promises reserved space and write gaps read as zero. * - * Only the part of the range that lies inside the file can hold stale bytes; - * past the end there is nothing to erase, and a gap left behind by a later - * write reads as zero by POSIX. Clamping to the current size is not just an - * optimisation. A single FALLOC_FL_ZERO_RANGE call asked to both zero and - * extend was observed on ext4 to extend the file, report success, and leave - * the bytes below the old end of file untouched -- which is precisely where - * the previous ls_close wrote its table of contents, so a tail extent handed - * out over that ToC served its bytes back to the caller as data. Never let - * one call do both jobs. */ + * Only bytes already inside the file can be stale; past the end there is + * nothing to erase. Splitting the range on the end of file is not an + * optimisation. A single FALLOC_FL_ZERO_RANGE call asked to zero and extend at + * once was observed on ext4 to extend the file, report success, and leave the + * bytes below the old end of file untouched -- which is exactly where the + * previous ls_close wrote its table of contents, so a tail extent handed out + * over that ToC served its bytes back to the caller as data (issue #7). Never + * let one call do both jobs. + * + * The file still has to reach the end of the range, because ls_reserve lets a + * caller read space it never wrote. Extending with one zero byte at the last + * offset rather than a truncate is deliberate: the byte lies inside the extent + * being zeroed, so no other thread can own it, and unlike ftruncate -- or + * _chsize_s, which zero-fills as it goes -- it cannot cut back a file another + * thread has already grown. Reads and writes run outside toc_lk on purpose, so + * a truncate here is not serialised against them. */ static int zero_range(ls_store *s, uint64_t off, uint64_t len) { static const size_t CH = 1u << 20; unsigned char *z; int rc = LS_OK; - uint64_t old_end; + uint64_t end, inside; - if (ls_os_file_size(s->fd, &old_end) != 0) return -errno; + if (len == 0) return LS_OK; - /* Extending is the first job: the new tail is a hole, so it reads as zero - * without anything being written, and ls_reserve depends on the file - * actually reaching that far. */ - if (off + len > old_end && ls_os_ftruncate(s->fd, off + len) != 0) - return -errno; + /* O_DIRECT has always written its zeros: the zeroing primitive is a page + * cache operation and mixing the two is not worth the reasoning. */ + if (!s->direct) { + if (ls_os_file_size(s->fd, &end) != 0) return -errno; + inside = (off >= end) ? 0 : (len > end - off ? end - off : len); - /* Erasing is the second job, and only what was already inside the file can - * need it. ls_os_zero_range is documented to be handed a range that is - * wholly inside the file, which is what makes it safe to use here. */ - if (off >= old_end) return LS_OK; - if (len > old_end - off) len = old_end - off; - if (len == 0) return LS_OK; + if (inside == 0 || ls_os_zero_range(s->fd, off, inside) == 0) { + unsigned char zb = 0; + if (off + len <= end) return LS_OK; + return ls_pwrite_all(s, &zb, 1, off + len - 1); + } + } - if (!s->direct && ls_os_zero_range(s->fd, off, len) == 0) - return LS_OK; + /* No zeroing primitive, or it refused: write the zeros over the whole + * range, which extends the file as a side effect. This is what every + * platform without one has always done, so it is the well-trodden path. */ z = ls_os_aligned_alloc(LS_ALIGN, CH); if (!z) return -ENOMEM; memset(z, 0, CH);