WIP Only save MVCC copy when overwriting an object

src/blockstore/blockstore_heap.cpp

@@ -853,26 +853,40 @@ void blockstore_heap_t::reshard(pool_id_t pool, uint32_t pg_count, uint32_t pg_s
     };
 }
 
-heap_object_t *blockstore_heap_t::lock_and_read_entry(object_id oid, uint64_t & lsn)
+heap_object_t *blockstore_heap_t::lock_and_read_entry(object_id oid, uint64_t & copy_id)
 {
     auto obj = read_entry(oid, NULL);
     if (!obj)
     {
         return NULL;
     }
-    lsn = obj->get_writes()->lsn;
+    auto mvcc_it = object_mvcc.lower_bound({ .oid = { .inode = oid.inode, .stripe = oid.stripe+1 }, .lsn = 0 });
-    auto & mvcc = object_mvcc[(heap_object_lsn_t){ .oid = oid, .lsn = lsn }];
+    copy_id = 1;
-    mvcc.readers++;
+    if (mvcc_it != object_mvcc.begin())
-    if (mvcc.entry_copy)
     {
-        return mvcc.entry_copy;
+        mvcc_it--;
+        if (mvcc_it->first.oid == oid)
+        {
+            if (mvcc_it->second.entry_copy)
+            {
+                // Already modified, need to create another copy
+                copy_id = mvcc_it->first.lsn+1;
+            }
+            else
+            {
+                copy_id = mvcc_it->first.lsn;
+                mvcc_it->second.readers++;
+                return obj;
+            }
+        }
     }
+    object_mvcc[(heap_object_lsn_t){ .oid = oid, .lsn = copy_id }] = (heap_object_mvcc_t){ .readers = 1 };
     return obj;
 }
 
-heap_object_t *blockstore_heap_t::read_locked_entry(object_id oid, uint64_t lsn)
+heap_object_t *blockstore_heap_t::read_locked_entry(object_id oid, uint64_t copy_id)
 {
-    auto mvcc_it = object_mvcc.find((heap_object_lsn_t){ .oid = oid, .lsn = lsn });
+    auto mvcc_it = object_mvcc.find((heap_object_lsn_t){ .oid = oid, .lsn = copy_id });
     if (mvcc_it == object_mvcc.end())
     {
         return NULL;
@@ -884,9 +898,9 @@ heap_object_t *blockstore_heap_t::read_locked_entry(object_id oid, uint64_t lsn)
     return read_entry(oid, NULL);
 }
 
-bool blockstore_heap_t::unlock_entry(object_id oid, uint64_t lsn)
+bool blockstore_heap_t::unlock_entry(object_id oid, uint64_t copy_id)
 {
-    auto mvcc_it = object_mvcc.find((heap_object_lsn_t){ .oid = oid, .lsn = lsn });
+    auto mvcc_it = object_mvcc.find((heap_object_lsn_t){ .oid = oid, .lsn = copy_id });
     if (mvcc_it == object_mvcc.end())
     {
         return false;
@@ -1266,13 +1280,20 @@ int blockstore_heap_t::add_object(object_id oid, heap_write_t *wr, uint32_t *mod
 heap_object_t *blockstore_heap_t::mvcc_save_copy(heap_object_t *obj)
 {
     auto oid = (object_id){ .inode = obj->inode, .stripe = obj->stripe };
-    auto lsn = obj->get_writes()->lsn;
+    auto mvcc_it = object_mvcc.lower_bound({ .oid = { .inode = oid.inode, .stripe = oid.stripe+1 }, .lsn = 0 });
-    auto mvcc_it = object_mvcc.find((heap_object_lsn_t){ .oid = oid, .lsn = lsn });
+    if (mvcc_it == object_mvcc.begin())
-    if (mvcc_it == object_mvcc.end())
     {
         return NULL;
     }
-    assert(!mvcc_it->second.entry_copy);
+    mvcc_it--;
+    if (mvcc_it->first.oid != oid)
+    {
+        return NULL;
+    }
+    if (mvcc_it->second.entry_copy)
+    {
+        return mvcc_it->second.entry_copy;
+    }
     assert(obj->size == sizeof(heap_object_t));
     uint32_t total_size = obj->size;
     for (auto wr = obj->get_writes(); wr; wr = wr->next())
@@ -1361,8 +1382,8 @@ int blockstore_heap_t::update_object(uint32_t block_num, heap_object_t *obj, hea
     {
         *modified_block = block_num;
     }
-    // Save a copy of the object
+    // Save a copy of the object - only when overwriting
-    heap_object_t *obj_copy = mvcc_save_copy(obj);
+    heap_object_t *obj_copy = is_overwrite ? mvcc_save_copy(obj) : NULL;
     bool tracking_active = !!obj_copy;
     if (!tracking_active)
     {
@@ -1486,10 +1507,10 @@ int blockstore_heap_t::post_stabilize(object_id oid, uint64_t version, uint32_t
         *before_compact_lsn = pre_wr->lsn;
     }
     // Save a copy of the object
-    mvcc_save_copy(obj);
     if (unstable_big_wr && unstable_big_wr->next())
     {
         // Remove previous stable entry series
+        mvcc_save_copy(obj);
         free_object_space(obj->inode, unstable_big_wr->next(), NULL);
         add_used_space(block_num, -free_writes(unstable_big_wr->next(), NULL));
         unstable_big_wr->next_pos = 0;

src/blockstore/blockstore_heap.h

@@ -203,13 +203,13 @@ public:
     void reshard(pool_id_t pool, uint32_t pg_count, uint32_t pg_stripe_size);
     // read an object entry and lock it against removal
     // in the future, may become asynchronous
-    heap_object_t *lock_and_read_entry(object_id oid, uint64_t & lsn);
+    heap_object_t *lock_and_read_entry(object_id oid, uint64_t & copy_id);
     // re-read a locked object entry with the given lsn (pointer may be invalidated)
-    heap_object_t *read_locked_entry(object_id oid, uint64_t lsn);
+    heap_object_t *read_locked_entry(object_id oid, uint64_t copy_id);
     // read an object entry without locking it
     heap_object_t *read_entry(object_id oid, uint32_t *block_num_ptr, bool for_update = false);
     // unlock an entry
-    bool unlock_entry(object_id oid, uint64_t lsn);
+    bool unlock_entry(object_id oid, uint64_t copy_id);
     // set or verify checksums in a write request
     bool calc_checksums(heap_write_t *wr, uint8_t *data, bool set);
     // set or verify raw block checksums

src/test/test_heap.cpp

@@ -234,13 +234,13 @@ void test_mvcc(bool csum)
         assert(heap.find_free_data() == 0x20000);
 
         object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-        uint64_t lsn = 0;
+        uint64_t copy_id = 0;
-        heap_object_t *obj = heap.lock_and_read_entry(oid, lsn);
+        heap_object_t *obj = heap.lock_and_read_entry(oid, copy_id);
         assert(obj);
-        assert(lsn >= 1);
+        assert(copy_id == 1);
         assert(count_writes(obj) == 1);
         heap_write_t *wr = obj->get_writes();
-        assert(wr->lsn == lsn);
+        assert(wr->lsn == 1);
         assert(wr->version == 1);
         assert(wr->offset == 0);
         assert(wr->len == dsk.data_block_size);
@@ -248,7 +248,7 @@ void test_mvcc(bool csum)
         assert(wr->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE);
         uint64_t old_size = obj->size + wr->size;
 
-        assert(heap.read_locked_entry(oid, lsn) == obj);
+        assert(heap.read_locked_entry(oid, copy_id) == obj);
 
         assert(_test_do_small_write(heap, dsk, 1, 0, 1, 0, 4096, 0) == EINVAL);
 
@@ -258,38 +258,24 @@ void test_mvcc(bool csum)
         assert(heap.get_meta_block_used_space(0) == old_size + obj->get_writes()->get_size(&heap));
 
         assert(!heap.read_locked_entry(oid, UINT64_MAX));
-        obj = heap.read_locked_entry(oid, lsn);
+        assert(heap.read_locked_entry(oid, copy_id) == obj); // small_write isn't MVCCed
-        assert(obj);
+
+        _test_big_write(heap, dsk, 1, 0, 3, 0x20000);
+        obj = heap.read_entry(oid, NULL);
         assert(count_writes(obj) == 1);
         wr = obj->get_writes();
-        assert(wr->lsn == lsn);
+        assert(wr->lsn == 3);
-        assert(wr->version == 1);
+        assert(wr->version == 3);
-        assert(wr->offset == 0);
-        assert(wr->len == dsk.data_block_size);
-        assert(wr->location == 0);
         assert(wr->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE);
 
-        obj = heap.read_entry(oid, NULL);
+        assert(heap.read_locked_entry(oid, copy_id) != obj); // big_write is MVCCed
-        assert(obj);
+        obj = heap.read_locked_entry(oid, copy_id);
         assert(count_writes(obj) == 2);
         wr = obj->get_writes();
-        assert(wr->lsn > lsn);
+        assert(wr->lsn == 2);
-        assert(wr->version == 2);
-        assert(wr->offset == 8192);
-        assert(wr->len == 4096);
-        assert(wr->location == 16384);
-        assert(wr->flags == BS_HEAP_SMALL_WRITE|BS_HEAP_STABLE);
-        assert(!wr->get_int_bitmap(&heap));
-        wr = wr->next();
-        assert(wr->lsn == lsn);
-        assert(wr->version == 1);
-        assert(wr->offset == 0);
-        assert(wr->len == dsk.data_block_size);
-        assert(wr->location == 0);
-        assert(wr->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE);
 
         assert(!heap.unlock_entry(oid, UINT64_MAX));
-        assert(heap.unlock_entry(oid, lsn));
+        assert(heap.unlock_entry(oid, copy_id));
     }
 
     printf("OK test_mvcc %s\n", csum ? "csum" : "no_csum");
@@ -403,8 +389,8 @@ void test_compact(bool csum, bool stable)
 
     // write unstable - stabilize - compact
     object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-    uint64_t lsn = 0;
+    uint64_t copy_id = 0;
-    heap_object_t *obj = heap.lock_and_read_entry(oid, lsn);
+    heap_object_t *obj = heap.lock_and_read_entry(oid, copy_id);
     assert(obj);
     assert(count_writes(obj) == 1);
     assert(obj->get_writes()->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE);
@@ -422,10 +408,9 @@ void test_compact(bool csum, bool stable)
 
     _test_big_write(heap, dsk, 2, 0, 1, 0x40000, true, 0, 4096);
 
-    obj = heap.read_locked_entry(oid, lsn);
+    obj = heap.read_locked_entry(oid, copy_id);
     assert(obj);
-    assert(count_writes(obj) == 1);
+    assert(count_writes(obj) == 2);
-    assert(obj->get_writes()->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE);
 
     uint32_t mblock;
     object_id compact_oid = {};
@@ -486,7 +471,7 @@ void test_compact(bool csum, bool stable)
     assert(count_writes(obj) == 1);
     assert(obj->get_writes()->version == 1);
 
-    int unlock_res = heap.unlock_entry(oid, lsn);
+    int unlock_res = heap.unlock_entry(oid, copy_id);
     assert(unlock_res);
 
     printf("OK test_compact %s %s\n", stable ? "stable" : "unstable", csum ? "csum" : "no_csum");
@@ -503,9 +488,9 @@ void test_modify_bitmap()
 
     _test_big_write(heap, dsk, 1, 0, 1, 0x20000);
 
-    uint64_t lsn = 0;
+    uint64_t copy_id = 0;
     object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-    heap_object_t *obj = heap.lock_and_read_entry(oid, lsn);
+    heap_object_t *obj = heap.lock_and_read_entry(oid, copy_id);
     assert(obj);
 
     uint32_t modified_block = 1;
@@ -519,7 +504,7 @@ void test_modify_bitmap()
     uint8_t ref_int_bitmap[dsk.clean_entry_bitmap_size];
     memset(ref_int_bitmap, 0xFF, dsk.clean_entry_bitmap_size);
 
-    obj = heap.read_locked_entry(oid, lsn);
+    obj = heap.read_locked_entry(oid, copy_id);
     assert(obj);
     assert(!memcmp(obj->get_writes()->get_int_bitmap(&heap), ref_int_bitmap, dsk.clean_entry_bitmap_size));
 
@@ -528,7 +513,7 @@ void test_modify_bitmap()
     bitmap_clear(ref_int_bitmap, 4096, 16384, dsk.bitmap_granularity);
     assert(!memcmp(obj->get_writes()->get_int_bitmap(&heap), ref_int_bitmap, dsk.clean_entry_bitmap_size));
 
-    int unlock_res = heap.unlock_entry(oid, lsn);
+    int unlock_res = heap.unlock_entry(oid, copy_id);
     assert(unlock_res);
 
     printf("OK test_modify_bitmap\n");
@@ -594,12 +579,11 @@ void test_recheck(bool async, bool csum)
 
         // read object 1 - big_write should be there but small_write should be rechecked and removed
         object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-        uint64_t lsn = 0;
+        heap_object_t *obj = heap.read_entry(oid, NULL);
-        heap_object_t *obj = heap.lock_and_read_entry(oid, lsn);
         assert(obj);
         assert(count_writes(obj) == 1);
         heap_write_t *wr = obj->get_writes();
-        assert(wr->lsn == lsn);
+        assert(wr->lsn == 1);
         assert(wr->version == 1);
         assert(wr->offset == 0);
         assert(wr->len == dsk.data_block_size);
@@ -608,11 +592,11 @@ void test_recheck(bool async, bool csum)
 
         // read object 2 - both writes should be present
         oid = { .inode = INODE_WITH_POOL(1, 2), .stripe = 0 };
-        obj = heap.lock_and_read_entry(oid, lsn);
+        obj = heap.read_entry(oid, NULL);
         assert(obj);
         assert(count_writes(obj) == 2);
         wr = obj->get_writes();
-        assert(wr->lsn == lsn);
+        assert(wr->lsn == 4);
         assert(wr->version == 2);
         assert(wr->offset == 8192);
         assert(wr->len == 4096);
@@ -736,8 +720,8 @@ void test_full_overwrite(bool stable)
 
         // read it to test mvcc
         object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-        uint64_t read_lsn = 0;
+        uint64_t copy_id = 0;
-        heap_object_t *obj = heap.lock_and_read_entry(oid, read_lsn);
+        heap_object_t *obj = heap.lock_and_read_entry(oid, copy_id);
         assert(obj);
 
         // small_write
@@ -745,14 +729,11 @@ void test_full_overwrite(bool stable)
 
         // big_write again
         _test_big_write(heap, dsk, 1, 0, 3, 0x40000, stable, 16384, 4096);
-        if (stable)
+        assert(!heap.is_buffer_area_free(16384, 4096)); // should not be freed because MVCC includes it
-        {
-            assert(heap.is_buffer_area_free(16384, 4096)); // should be freed because it's not in MVCC
-        }
         assert(heap.is_data_used(0x20000)); // should NOT be freed - still referenced by MVCC
 
         // free mvcc
-        heap.unlock_entry(oid, read_lsn);
+        heap.unlock_entry(oid, copy_id);
         if (stable)
         {
             assert(!heap.is_data_used(0x20000)); // should now be freed
@@ -1028,8 +1009,8 @@ void test_destructor_mvcc()
 
         // read it to test mvcc
         object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-        uint64_t read_lsn = 0;
+        uint64_t copy_id = 0;
-        heap_object_t *obj = heap.lock_and_read_entry(oid, read_lsn);
+        heap_object_t *obj = heap.lock_and_read_entry(oid, copy_id);
         assert(obj);
 
         _test_small_write(heap, dsk, 1, 0, 2, 8192, 4096, 16384, true);
@@ -1056,8 +1037,8 @@ void test_rollback()
 
         // read it to test mvcc
         object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
-        uint64_t read_lsn = 0;
+        uint64_t copy_id = 0;
-        heap_object_t *obj = heap.lock_and_read_entry(oid, read_lsn);
+        heap_object_t *obj = heap.lock_and_read_entry(oid, copy_id);
         assert(obj);
 
         // already stable
@@ -1072,9 +1053,10 @@ void test_rollback()
         _test_small_write(heap, dsk, 1, 0, 4, 20480, 4096, 20480, false);
 
         // second read
-        uint64_t read2_lsn = 0;
+        uint64_t copy2_id = 0;
-        obj = heap.lock_and_read_entry(oid, read2_lsn);
+        obj = heap.lock_and_read_entry(oid, copy2_id);
         assert(obj);
+        assert(copy2_id == copy_id);
 
         // rollback
         assert(heap.is_data_used(0x20000));
@@ -1092,16 +1074,14 @@ void test_rollback()
         assert(!heap.is_buffer_area_free(16384, 4096));
         assert(!heap.is_buffer_area_free(20480, 4096));
 
-        // free second mvcc
+        // free mvcc
-        heap.unlock_entry(oid, read2_lsn);
+        heap.unlock_entry(oid, copy2_id);
+        heap.unlock_entry(oid, copy_id);
         assert(heap.is_data_used(0x20000));
         assert(!heap.is_data_used(0x40000));
         assert(!heap.is_buffer_area_free(16384, 4096));
         assert(heap.is_buffer_area_free(20480, 4096));
 
-        // free first mvcc
-        heap.unlock_entry(oid, read_lsn);
-
         // check object data
         obj = heap.read_entry(oid, NULL);
         assert(obj);