src/pager.zig

@@ -557,6 +557,13 @@ pub const Pager = struct {
     pub fn reserve_pages(self: *Pager, hold: *Reservation, n: u32) !void {
         self.alloc_lock.lockUncancelable(self.io);
         defer self.alloc_lock.unlock(self.io);
+        return self.reserve_pages_locked(hold, n);
+    }
+
+    /// For callers already holding `alloc_lock`. The lock is not reentrant, so
+    /// the split is what lets `write_freelist` hold it across reading the lists
+    /// *and* allocating the pages it writes them into.
+    fn reserve_pages_locked(self: *Pager, hold: *Reservation, n: u32) !void {
         // Additive: room for every promise outstanding anywhere *plus* this one.
         // Two consumers reserving before the same log append must both be able to
         // rely on their promise.
@@ -646,9 +653,14 @@ pub const Pager = struct {
     }
 
     pub fn alloc_pages_assume_reserved(self: *Pager, hold: *Reservation, n: u32) u32 {
-        assert(n > 0);
         self.alloc_lock.lockUncancelable(self.io);
         defer self.alloc_lock.unlock(self.io);
+        return self.alloc_assume_reserved_locked(hold, n);
+    }
+
+    /// For callers already holding `alloc_lock`; see `reserve_pages_locked`.
+    fn alloc_assume_reserved_locked(self: *Pager, hold: *Reservation, n: u32) u32 {
+        assert(n > 0);
         assert_msg(
             n <= hold.pages,
             "page allocation overran reserve_pages' promise",
@@ -985,8 +997,18 @@ pub const Pager = struct {
 
     /// Give back a run of pages. They become reusable two generations later --
     /// see the field comment on `free_ready`.
+    ///
+    /// Under the allocation lock, like every other mutation of the free lists.
+    /// It was not, and the hot caller is `page_mut_cow`, which runs under a
+    /// *collection* lock: two collections doing copy-on-write concurrently
+    /// appended to the same list, and `publish` rotated all three lists
+    /// underneath them. No caller holds the lock already -- `page_mut_cow` takes
+    /// it inside `alloc_pages` and has released it by here -- so this cannot
+    /// recurse.
     pub fn free_pages(self: *Pager, first: u32, pages: u32) !void {
         if (pages == 0) return;
+        self.alloc_lock.lockUncancelable(self.io);
+        defer self.alloc_lock.unlock(self.io);
         try self.free_pending.append(self.gpa, .{ .first = first, .pages = pages });
     }
 
@@ -1010,9 +1032,22 @@ pub const Pager = struct {
         //
         // `take_free` never *adds* an entry, so the bound holds and one
         // allocation is enough.
+        //
+        // Under `alloc_lock` for the whole of it, allocation included. Reading
+        // the three lists is as much a use of them as appending is: a concurrent
+        // `free_pages` -- copy-on-write in some collection, which a checkpoint
+        // does not exclude -- grows `free_pending` while the loop below walks it,
+        // and a growth that reallocates leaves the loop on freed memory. A free
+        // that lands after this point simply waits for the next generation's
+        // list; the page stays allocated one generation longer, which is the
+        // safe direction.
+        self.alloc_lock.lockUncancelable(self.io);
+        defer self.alloc_lock.unlock(self.io);
         const bound = self.free_ready.items.len + self.free_hold.items.len + self.free_pending.items.len;
         const pages: u32 = @intCast((8 + bound * 8 + 8 + page_size - 1) / page_size);
-        const first = try self.alloc_pages(pages);
+        var hold: Reservation = .{};
+        try self.reserve_pages_locked(&hold, pages);
+        const first = self.alloc_assume_reserved_locked(&hold, pages);
         const buf = self.bytes_mut(@as(u64, first) << page_shift, @as(usize, pages) << page_shift);
         @memset(buf, 0);
         var at: usize = 8;
@@ -1809,6 +1844,79 @@ test "one-page requests do not carve up the runs the extents need" {
     try testing.expectEqual(tail_before, pg.alloc_tail);
 }
 
+test "concurrent frees lose no pages while a publish rotates the lists" {
+    // `free_pages` mutates the same three lists `publish` rotates, and its hot
+    // caller is `page_mut_cow` under a *collection* lock -- so two collections
+    // copying nodes concurrently were appending to one `ArrayList` unserialized
+    // while a checkpoint moved it out from under them.
+    //
+    // Pages are conserved across the rotation and across coalescing, so the sum
+    // over all three lists is the invariant to assert. Probabilistic by nature,
+    // as any test of a data race is: it says nothing when green and is only
+    // evidence when red. Mutation check: drop the lock from `free_pages` and
+    // this fails or crashes within a few runs.
+    const gpa = testing.allocator;
+    var threaded: std.Io.Threaded = .init(gpa, .{});
+    defer threaded.deinit();
+    const io = threaded.io();
+    var tp = try TmpPager.init(io, 64 << 20);
+    defer tp.deinit();
+
+    const freers = 4;
+    const per_freer = 200;
+    // One page each, allocated up front so no fiber is also growing the file.
+    var pages: [freers * per_freer]u32 = undefined;
+    for (&pages) |*p| p.* = try tp.pg().alloc_pages(1);
+    try tp.pg().publish(.{ .seq = 1 });
+
+    const Worker = struct {
+        fn freer(p: *Pager, run: []const u32) error{Canceled}!void {
+            for (run) |page| p.free_pages(page, 1) catch return error.Canceled;
+        }
+        fn publisher(p: *Pager, seq: *std.atomic.Value(u64)) error{Canceled}!void {
+            for (0..8) |_| {
+                p.publish(.{ .seq = seq.fetchAdd(1, .monotonic) }) catch return error.Canceled;
+            }
+        }
+    };
+
+    var seq = std.atomic.Value(u64).init(2);
+    var group: std.Io.Group = .init;
+    defer group.cancel(io);
+    for (0..freers) |i| {
+        group.async(io, Worker.freer, .{ tp.pg(), pages[i * per_freer ..][0..per_freer] });
+    }
+    group.async(io, Worker.publisher, .{ tp.pg(), &seq });
+    try group.await(io);
+
+    // Page identity, not a total: the publisher's own free-list streams are
+    // allocated *off this list*, so a plain count would be short by however many
+    // publishes found a fit. Every page still on the list must therefore be one
+    // the freers put there, exactly once -- a lost or half-written append shows
+    // up as a duplicate or as a page nobody freed, neither of which recycling
+    // can produce.
+    const lo = pages[0];
+    var seen = try std.DynamicBitSetUnmanaged.initEmpty(gpa, pages.len);
+    defer seen.deinit(gpa);
+    var on_list: usize = 0;
+    for ([_][]const Extent{
+        tp.pg().free_ready.items,
+        tp.pg().free_hold.items,
+        tp.pg().free_pending.items,
+    }) |list| for (list) |e| {
+        for (0..e.pages) |i| {
+            const p = e.first + @as(u32, @intCast(i));
+            try testing.expect(p >= lo and p - lo < pages.len);
+            try testing.expect(!seen.isSet(p - lo));
+            seen.set(p - lo);
+            on_list += 1;
+        }
+    };
+    // The only pages missing are the ones a publish recycled into its stream,
+    // and there were nine publishes at one page each.
+    try testing.expect(pages.len - on_list <= 9);
+}
+
 test "freed pages that touch merge back into a usable run" {
     // Mutation check: drop the `coalesce_free_ready()` call from `publish`. Red
     // -- the four one-page frees below stay four separate holes and the run of