VSR: Generalize trailer #1351

sentientwaffle · 2023-12-11T17:38:30Z

In preparation for moving client sessions data from the superblock trailer into the grid, generalize FreeSetEncoded into CheckpointTrailer.

Note that this commit doesn't touch the schema.FreeSetNode block type; that will be part of the next PR.

sentientwaffle · 2023-12-11T17:46:38Z

src/vsr/checkpoint_trailer.zig

This was a git mv from free_set_encoded.zig; not sure why it is showing it as a new file. 😞

As I've recently learned, git mv is literally mv && git add, it doesn't actually record the fact that this was a move anywhere, move vs insert+delete is still based on diff heuristic. I think the only reliable way to record moves in git is to commit just moves separately, and to fixup in a different commit on-top.

🤦 Ah, that explains it, thanks.

I'll split the commit up, but will hold off on pushing that until I implement other review comments, to avoid messing with any PR comments that you've already written.

not much written yet, please push as it is indeed a bit annoying to review without finer-grained diff :)

Done! 281360c

In preparation for moving client sessions data from the superblock trailer into the grid, generalize `FreeSetEncoded` into `CheckpointTrailer`. Note that this commit doesn't touch the `schema.FreeSetNode` block type; that will be part of the next PR.

sentientwaffle · 2023-12-11T18:08:27Z

src/vsr/checkpoint_trailer.zig

-        /// Checkpoint process is delicate:
-        ///   1. Encode free set.
-        ///   2. Derive the number of blocks required to store the encoding.
-        ///   3. Allocate blocks for the encoding (in the old checkpoint).
-        ///   4. Write the blocks to disk.
-        ///   --------------------------------------------------------------
-        ///   5. Mark currently released blocks as free.
-        ///   6. Release the freshly acquired blocks in the new checkpoint.
-        ///
-        /// This function handles steps 1-4. The caller is responsible for calling
-        /// FreeSet.checkpoint which handles 5 and 6.


This description has moved to grid.zig.

matklad · 2023-12-11T18:14:49Z

src/vsr/checkpoint_trailer.zig

-        // the superblock, which doesn't have access to grid. It is set to null by reset, to verify
-        // that the free set is not used before it is opened during sync.
+        // the grid, which doesn't have access to a stable grid pointer. It is set to null by
+        // `reset`, to verify that the free set is not used before it is opened during sync.


some residual free sets in the comment here

I think in this particular case the comment is correct -- the reason for lazy-initializing the grid pointer is specifically due to the free-set/grid relationship, not client sessions.

src/vsr/checkpoint_trailer.zig

matklad · 2023-12-11T18:24:38Z

src/vsr/checkpoint_trailer.zig

-                assert(set.grid.?.free_set.count_reservations() == 0);
-                const reservation = set.grid.?.free_set.reserve(set.block_count()).?;
-                defer set.grid.?.free_set.forfeit(reservation);
+                assert(trailer.grid.?.free_set.count_reservations() == 0);


I wonder whether this is necessary? I think it would be correct to checkpoint a trailer concurrently with something else? Though, it's fine to leave assert as is, just to solidify the current implementation.

However, let's add the same assert to:

that place in grid.zig where we do "include_staging()" trick. There it is crucialt that there are no outstanding reservations

inside include_staging itself

matklad · 2023-12-11T18:29:40Z

src/vsr/grid.zig

+            );
+        }
+
+        fn free_set_open_callback(free_set_checkpoint: *CheckpointTrailer) void {


maybe open_free_set_callback, so that is lexicographically nested with open? I think we have both patterns, and I somewhat consciously chose open_xxx_callback naming for free set

And we should totally be consitent with checkpoint_free_set_callback in the same file

matklad · 2023-12-11T18:31:06Z

src/vsr/grid.zig

+                    .encoded = free_set_checkpoint.buffer[0..free_set_checkpoint.size],
+                    .block_addresses = free_set_checkpoint.block_addresses[0..free_set_checkpoint.block_count()],
+                });
+                assert((free_set_checkpoint.size > 0) == (grid.free_set.count_acquired() > 0));


I wonder if we should add free_set.pristine()? I worry a little that count_acquired is O(N), while its enough to just find a single set bit (which probably going to be the first one even)

Maybe .empty(), since we have open_empty()?
(And to check that I'm on the same page: pristine/empty would check count_acquired() == 0, but is implemented with an early-exit when the first acquired address is found.)

I was a bit hesitant about empty, as I think we might want to check all of blocks, staging, and reservations. But I am probably overthinking this. And yes, the idea is to do an early return. Though I guess if we are checking staging, we’d still be O(N)?

🤔 I think I'd rather assert each of those separately rather than bundling them up into a single function, to make it obvious what is being checked.
That information is already exposed via free_set.count_*.

matklad · 2023-12-11T18:37:42Z

src/vsr/grid.zig

        pub fn checkpoint(grid: *Grid, callback: *const fn (*Grid) void) void {
-            assert(grid.checkpointing == null);
-            assert(grid.canceling == null);
+            assert(grid.callback == .none);


I was thinking about a pattern like

assert(grid.callback == .none); defere assert(grid.callback == .checkpoint);

to underscore that this is a state transition function. But that's probably more noise than helpful.

I do like that pattern when there are multiple branches / return points, though!

matklad · 2023-12-11T18:39:16Z

src/vsr/grid.zig

-            assert(grid.canceling == null);
-            assert(grid.checkpointing == null);
+            // grid.open() is cancellable the same way that read_block()/write_block() are.
+            assert(grid.callback == .none or grid.callback == .open);


maybe switch for exhaustiveness here? feels useful to decect missing new cancelation cases at compiletime.

matklad · 2023-12-11T18:42:36Z

src/vsr/superblock.zig

    checksum: u128,
    last_block_address: u64,
    last_block_checksum: u128,
-    free_set_size: u32,
-    storage_size: u64,
+    trailer_size: u32,


For generic trailer, let's future-proof this to u64?

Should free_set_size/client_sessions_size in the CheckpointState be u64 too?

I’d say yes!

matklad · 2023-12-12T12:26:18Z

src/vsr/grid.zig

+                .open => {},
+                .checkpoint => unreachable,
+                .cancel => unreachable,
+            }


Any specific reason to avoid

.none, .open => {} .checkpoint, .cancel => unreachable,

grouping here?

🤔 I think we have one extra invariant here that we aren't actually checking perhaps? For states which are not cancelable (so, checkpoint and cancel), we must also ensure that we don't read any blocks, as that could deadlock.

In particular, we shouldn't be reading blocks during checkpoint. So, assert(grid.callback != .cancel); in read_block should also check callback != chcekpoint, and, really, it should be the same as current assert.

so maybe add gird.callback.can_cancel() helper for assertions here and on the read path?

In grid.read_block we already have:

switch (callback) { .from_local_storage => { maybe(grid.callback == .checkpoint); ... }, .from_local_or_global_storage => { assert(grid.callback != .checkpoint); ... }, }

Which I think is correct -- reads to satisfy grid repair requests are safe.

Any specific reason to avoid

Just personal preference -- I like all of the variants aligned, so if I was grouping them I would do

.none, .open, => {}, .checkpoint, .cancel, => unreachable,

But in this case that is longer than just repeating the rhs.

(Follow-up for #1351). No more trailers in the superblock! Client sessions is encoded with `CheckpointTrailer`, like the free set. Unlike the free set, client sessions encoding/decoding is handled directly by `replica.zig`, not `grid.zig`. This simplifies the superblock checkpoint/sync control flow, which now just needs to care about the superblock headers. This simplifies state sync, since the superblock sync is now just a single `sync_checkpoint` message.

sentientwaffle marked this pull request as ready for review December 11, 2023 17:43

sentientwaffle assigned matklad Dec 11, 2023

VSR: Spelling

692dc61

sentientwaffle force-pushed the dj-vsr-generalize-trailer branch from 82d0b72 to 6bac783 Compare December 11, 2023 17:44

sentientwaffle commented Dec 11, 2023

View reviewed changes

sentientwaffle added 2 commits December 11, 2023 10:00

VSR: Rename src/free_set_encoded.zig to src/vsr/checkpoint_trailer.zig

06c5313

sentientwaffle force-pushed the dj-vsr-generalize-trailer branch from 6bac783 to 281360c Compare December 11, 2023 18:06

sentientwaffle commented Dec 11, 2023

View reviewed changes

matklad reviewed Dec 11, 2023

View reviewed changes

VSR: CheckpointTrailer code review

51fc1d7

matklad reviewed Dec 12, 2023

View reviewed changes

matklad approved these changes Dec 13, 2023

View reviewed changes

sentientwaffle added this pull request to the merge queue Dec 13, 2023

Merged via the queue into main with commit 106b117 Dec 13, 2023
27 checks passed

sentientwaffle deleted the dj-vsr-generalize-trailer branch December 13, 2023 14:15

sentientwaffle mentioned this pull request Dec 13, 2023

VSR: Move client sessions from superblock to grid #1356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VSR: Generalize trailer #1351

VSR: Generalize trailer #1351

sentientwaffle commented Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

matklad Dec 11, 2023

matklad Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

matklad Dec 11, 2023

sentientwaffle Dec 11, 2023

matklad Dec 11, 2023

matklad Dec 12, 2023

matklad Dec 12, 2023

sentientwaffle Dec 12, 2023

sentientwaffle Dec 12, 2023

VSR: Generalize trailer #1351

VSR: Generalize trailer #1351

Conversation

sentientwaffle commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment