From dbbbadf025123d783db7d87b0369097c92253e68 Mon Sep 17 00:00:00 2001 From: retoor Date: Mon, 14 Sep 2026 07:03:11 +0000 Subject: [PATCH] Document overlay.c's merge policy and journal wire format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A closer re-audit of "is everything documented well" found real gaps beyond the first pass: overlay.c's dispatch functions (open/mkdir/ unlink/rename) implement genuinely non-obvious merge logic — checking both the upper layer and the lower pack, in a specific order, for existence/emptiness/whiteout status — with no comment explaining why, only individually-readable lines. Added a doc comment to each covering the actual decision policy, not a restatement of the code. The journal's binary wire format was referenced by name throughout (length-prefixed, checksummed, self-checking per Section 4.3) but never actually specified anywhere, and its record-type discriminator was three bare magic numbers (0/1/2). Documented the full record layout and replaced the magic numbers with named constants (JOP_PUT/ JOP_DELETE/JOP_MKDIR). Verified under -fsanitize=undefined (30 runs across the three sequential tests, 20 runs of the concurrency test, zero failures) and -fsanitize=address (20 runs; the known ASan-in-this-sandbox startup artifact diagnosed earlier accounted for 5, zero real findings in the rest), per CLAUDE.md's sanitizer rule for any overlay.c change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB --- src/overlay.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 74 insertions(+), 8 deletions(-) diff --git a/src/overlay.c b/src/overlay.c index 281b51e..32ca63c 100644 --- a/src/overlay.c +++ b/src/overlay.c @@ -37,6 +37,28 @@ static const char *dirrel(const char *p) { return p[0] == '/' ? p + 1 : p; } /* ---- journal (Section 4.4) ---- */ +/* Journal record wire format (append-only; a checksummed record with a + * length prefix, per Section 4.3's self-checking-record principle): + * + * u32 payload_len + * u32 checksum FNV-1a64 of payload, truncated to 32 bits + * u8 op JOP_PUT / JOP_DELETE / JOP_MKDIR + * -- payload (payload_len bytes) -- + * u32 name_len + * u8[name_len] name + * -- JOP_PUT only: -- + * i64 mtime + * u64 size + * u8[size] data + * + * journal_replay reads this sequentially and stops at the first record + * whose length doesn't fit the remaining file or whose checksum doesn't + * match — everything after that point is treated as never committed. + */ +#define JOP_PUT 0 +#define JOP_DELETE 1 +#define JOP_MKDIR 2 + static void journal_append_record(Overlay *ov, uint8_t op, const char *name, int64_t mtime, uint64_t size, const void *data) { if (!ov->pack_path) return; @@ -48,12 +70,12 @@ static void journal_append_record(Overlay *ov, uint8_t op, const char *name, } if (ov->journal_fp) { uint32_t name_len = (uint32_t)strlen(name); - uint32_t payload_len = 4 + name_len + (op == 0 ? (uint32_t)(8 + 8 + size) : 0); + uint32_t payload_len = 4 + name_len + (op == JOP_PUT ? (uint32_t)(8 + 8 + size) : 0); unsigned char *payload = (unsigned char *)malloc(payload_len); size_t off = 0; memcpy(payload + off, &name_len, 4); off += 4; memcpy(payload + off, name, name_len); off += name_len; - if (op == 0) { + if (op == JOP_PUT) { memcpy(payload + off, &mtime, 8); off += 8; memcpy(payload + off, &size, 8); off += 8; if (size) memcpy(payload + off, data, size); @@ -71,13 +93,13 @@ static void journal_append_record(Overlay *ov, uint8_t op, const char *name, } static void journal_append_put(Overlay *ov, const char *name, const void *data, uint64_t size, int64_t mtime) { - journal_append_record(ov, 0, name, mtime, size, data); + journal_append_record(ov, JOP_PUT, name, mtime, size, data); } static void journal_append_delete(Overlay *ov, const char *name) { - journal_append_record(ov, 1, name, 0, 0, NULL); + journal_append_record(ov, JOP_DELETE, name, 0, 0, NULL); } static void journal_append_mkdir(Overlay *ov, const char *name) { - journal_append_record(ov, 2, name, 0, 0, NULL); + journal_append_record(ov, JOP_MKDIR, name, 0, 0, NULL); } /* journals the *current* full content of `path` after a write/rename, @@ -132,17 +154,17 @@ static void journal_replay(Overlay *ov) { memcpy(name, payload + off, name_len); name[name_len] = '\0'; off += name_len; int err = 0; - if (op == 0) { + if (op == JOP_PUT) { int64_t mtime; uint64_t size; memcpy(&mtime, payload + off, 8); off += 8; memcpy(&size, payload + off, 8); off += 8; MutCell *cell; upper_copy_up(us, name, payload + off, size, mtime, &cell, &err); - } else if (op == 1) { + } else if (op == JOP_DELETE) { const PackIndexEntry *pe; int had = ov->pack && pack_find(ov->pack, name, &pe); upper_remove(us, name, had, &err); - } else if (op == 2) { + } else if (op == JOP_MKDIR) { upper_mkdir(us, name, &err); } free(name); @@ -172,6 +194,20 @@ static VfsFile *make_upper_file(Backend *b, Overlay *ov, const char *path, MutCe return f; } +/* + * Merge policy (Section 4): checked in this order, upper always wins. + * 1. An ENTRY_FILE in upper -> serve it directly (already copied up). + * 2. An ENTRY_WHITEOUT in upper -> the lower entry is hidden; behaves + * like "not found" except VFS_O_CREAT installs a fresh upper file, + * superseding the whiteout. + * 3. Not in upper, not in the lower pack -> VFS_O_CREAT installs a new + * upper file; otherwise VFS_ERR_NOENT. + * 4. In the lower pack only, opened read-only -> served straight from + * the pack's mmap, no copy-up (Section 4.1 step 3 ties copy-up to a + * write-capable open specifically, not every open). + * 5. In the lower pack only, opened for writing -> copy-up first + * (Section 4.1 step 3), then served from the new upper entry. + */ static int overlay_open(Backend *b, const char *path, int flags, VfsFile **out) { Overlay *ov = (Overlay *)b->state; UpperStore *us = upper_of(ov); @@ -379,6 +415,11 @@ static int overlay_readdir(Backend *b, const char *path, VfsDir *out) { return VFS_OK; } +/* "Does anything already occupy `path`?" has to be answered by checking + * both layers: an explicit or implicit (has-children) entry in upper, or + * an explicit entry or a live prefix range in the lower pack (a + * whiteout'd or since-superseded pack entry doesn't count, which is why + * this checks `!exists` before consulting the pack at all). */ static int overlay_mkdir(Backend *b, const char *path) { Overlay *ov = (Overlay *)b->state; UpperStore *us = upper_of(ov); @@ -408,6 +449,15 @@ static int overlay_mkdir(Backend *b, const char *path) { return VFS_OK; } +/* + * Mirrors overlay_mkdir's "check both layers" requirement in the other + * direction: a directory is non-empty if either layer has children + * beneath it, and a path exists (for the sake of returning NOTEMPTY + * instead of NOENT on an implicit directory) if either layer has + * anything there at all. `had_lower` — whether the *lower pack itself* + * has an entry at `path`, not merely a descendant — is what + * upper_remove uses to decide whiteout-vs-plain-removal (Section 4.2). + */ static int overlay_unlink(Backend *b, const char *path) { Overlay *ov = (Overlay *)b->state; UpperStore *us = upper_of(ov); @@ -451,6 +501,15 @@ static int overlay_unlink(Backend *b, const char *path) { return VFS_OK; } +/* + * If `from` is already in upper, this is a pure structural move + * (upper_rename, no data copy). If `from` exists only in the lower + * pack, there is nothing to "move" — it's a copy-up under the new name + * (or an mkdir, for a directory) followed by a whiteout of the old name, + * which is exactly what Section 4.1 step 4 specifies for rename. + * Journaling happens after the fact by reading back whatever now lives + * at `to`, rather than trying to journal the move itself. + */ static int overlay_rename(Backend *b, const char *from, const char *to) { Overlay *ov = (Overlay *)b->state; UpperStore *us = upper_of(ov); @@ -500,6 +559,13 @@ typedef struct BuildCtx { size_t upper_start; /* index at which upper-sourced (malloc'd) entries begin */ } BuildCtx; +/* Appends one entry to the compaction build list. `owned` distinguishes + * two different lifetimes of `data`: entries carried over from the old + * pack (owned=0) point directly into its mmap, valid only until + * pack_write finishes reading them; entries from the live upper store + * (owned=1, via build_visit) are copied here because upper_walk_live's + * callback only lends its buffer for the duration of one visit call. + * free_build_entries frees only the owned=1 tail (see BuildCtx.upper_start). */ static void build_push(BuildCtx *bc, const char *name, const void *data, pfs_usize size, uint32_t mode, int64_t mtime, int owned) { if (bc->count == bc->cap) { bc->cap = bc->cap ? bc->cap * 2 : 16; bc->entries = (PackBuildEntry *)realloc(bc->entries, bc->cap * sizeof(PackBuildEntry)); } PackBuildEntry *e = &bc->entries[bc->count++];