VSR: Move client sessions from superblock to grid #1356

sentientwaffle · 2023-12-13T22:07:17Z

(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.

(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 · 2023-12-13T22:32:53Z

src/vsr/superblock.zig

+/// The size of an individual superblock including padding.
+/// TODO Add some padding between copies and include that here.
+pub const superblock_copy_size = @sizeOf(SuperBlockHeader);


A few reasons to include padding in this number:

we probably want to reserve a bit of extra space as a just-in-case, since reformatting a whole data file isn't really an option

can add padding here without requiring an additional zone (grid_padding)

separate physical block addresses (in SSD terms, not grid blocks) of the superblock headers (probably doesn't matter with SSDs 🤷)

sentientwaffle · 2023-12-13T22:34:00Z

src/testing/cluster/storage_checker.zig

-//! Areas verified at compaction (half-measure):
-//! - Acquired Grid blocks (ignores skipped recovery compactions)
-//!  TODO Because ManifestLog acquires blocks potentially several beats prior to actually writing
-//!  the block, this check will need to be removed or use a different strategy.


Outdated comment. Originally the storage checker was going to verify storage determinism at half-bar boundaries, but that turns out not to work (due to the manifest log). Checkpoint is more comprehensive anyway.

sentientwaffle · 2023-12-13T22:34:20Z

src/vsr/replica_test.zig

@@ -872,17 +872,17 @@ test "Cluster: sync: sync, bump target, sync" {
    t.replica(.R2).drop_all(.R_, .bidirectional);
    try c.request(checkpoint_2_trigger, checkpoint_2_trigger);

-    // Allow R2 to complete SyncStage.requesting_trailers, but get stuck
-    // during SyncStage.requesting_trailers.
+    // Allow R2 to complete SyncStage.requesting_target, but get stuck


This was a typo, I think.

sentientwaffle · 2023-12-13T22:35:43Z

src/vsr/replica.zig

-            // Faulty bits will be set in state_machine_open_callback().
+            // Faulty bits will be set in sync_content().


This comment was out of date. I think state_machine_open_callback did originally handle this logic, but it was moved to sync_content as soon as client-replies + table sync was added.

sentientwaffle · 2023-12-13T22:37:31Z

src/testing/cluster/storage_checker.zig

        var free_set_buffer = try allocator.alignedAlloc(
            u8,
            @alignOf(u64),
            vsr.FreeSet.encode_size_max(Storage.grid_blocks_max),
        );
-        errdefer allocator.free(free_set);
+        errdefer allocator.free(free_set_buffer);


Typo. Not found by the compiler (prior to adding client_sessions_buffer) since the errdefer was not previously reachable (nothing could possibly error before here and the end of the function).

matklad · 2023-12-14T18:50:22Z

src/lsm/schema.zig

-        previous_free_set_block_checksum_padding: u128 = 0,
-        previous_free_set_block_address: u64,
+        previous_trailer_block_checksum: u128,
+        previous_trailer_block_checksum_padding: u128 = 0,


Wait, do we need this padding? I think we switched all pure checksumms to be unpadded?

I don't think this actually is a pure checksum -- it is referring to a block, which would be encrypted, so the "checksum" would be the AEAD tag.

src/testing/cluster/storage_checker.zig

src/vsr/checkpoint_trailer.zig

src/vsr/client_replies.zig

src/vsr/replica.zig

matklad · 2023-12-14T18:50:23Z

src/vsr/replica.zig

@@ -3177,6 +3158,11 @@
                .checkpoint_client_replies => {
                    self.client_replies.checkpoint(commit_op_checkpoint_client_replies_callback);
                },
+                .checkpoint_client_sessions => {
+                    self.client_sessions_checkpoint.size =
+                        self.client_sessions.encode(self.client_sessions_checkpoint.buffer);


I am wondering if perhaps there's some way to assert that both size and buffer are correctly set
at the same time? But I don't see any.

Yeah, the API is a little awkward. 🤔

src/vsr/superblock.zig

matklad · 2023-12-15T12:16:49Z

src/vsr/replica.zig

@@ -7977,6 +7963,7 @@ pub fn ReplicaType(

            self.grid.free_set.reset();
            self.grid.free_set_checkpoint.reset();
+            self.client_sessions_checkpoint.reset();


Feels odd that we are not reseting client_sessions here, but rather do it in client_sessions_open_callback.

I think resetting right before decoding makes sense to me from the perspective of defensive programming the decode code doesn't have to worry whether the object was properly reset. But maybe it makes sense to go on an offensive here? Reset the sessions here, so that, between this moment, and the moment we actually decode them, the sessions are in a well-defined reset state? In decoding, we can then assert that the sessions are reset.

sentientwaffle force-pushed the dj-vsr-no-superblock-trailers branch from 506f574 to 2ffb729 Compare December 13, 2023 22:20

sentientwaffle force-pushed the dj-vsr-no-superblock-trailers branch from 2ffb729 to 5e60d4b Compare December 13, 2023 22:22

sentientwaffle commented Dec 13, 2023

View reviewed changes

sentientwaffle marked this pull request as ready for review December 13, 2023 22:37

sentientwaffle assigned matklad Dec 13, 2023

matklad reviewed Dec 14, 2023

View reviewed changes

VSR: Move client sessions from superblock to grid (code review)

097c2c6

matklad reviewed Dec 15, 2023

View reviewed changes

VSR: Reset ClientSessions before superblock.sync()

0bf91e8

matklad approved these changes Dec 15, 2023

View reviewed changes

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

Merged via the queue into main with commit 928b36f Dec 15, 2023
27 checks passed

sentientwaffle deleted the dj-vsr-no-superblock-trailers branch December 15, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VSR: Move client sessions from superblock to grid #1356

VSR: Move client sessions from superblock to grid #1356

sentientwaffle commented Dec 13, 2023

sentientwaffle Dec 13, 2023

sentientwaffle Dec 13, 2023

sentientwaffle Dec 13, 2023

sentientwaffle Dec 13, 2023

sentientwaffle Dec 13, 2023

matklad Dec 14, 2023

sentientwaffle Dec 14, 2023

matklad Dec 14, 2023

sentientwaffle Dec 14, 2023

matklad Dec 15, 2023

		// Faulty bits will be set in state_machine_open_callback().
		// Faulty bits will be set in sync_content().

VSR: Move client sessions from superblock to grid #1356

VSR: Move client sessions from superblock to grid #1356

Conversation

sentientwaffle commented Dec 13, 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