From 66847653aa162a3e52e0b8bbacd27921bcb35218 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Fri, 4 Jul 2025 02:24:27 +0300 Subject: [PATCH] WIP Only save MVCC copy when overwriting an object --- src/blockstore/blockstore_heap.cpp | 55 ++++++++++----- src/blockstore/blockstore_heap.h | 6 +- src/test/test_heap.cpp | 104 ++++++++++++----------------- 3 files changed, 83 insertions(+), 82 deletions(-) diff --git a/src/blockstore/blockstore_heap.cpp b/src/blockstore/blockstore_heap.cpp index dc9aced9..b13b9ecb 100644 --- a/src/blockstore/blockstore_heap.cpp +++ b/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 = object_mvcc[(heap_object_lsn_t){ .oid = oid, .lsn = lsn }]; - mvcc.readers++; - if (mvcc.entry_copy) + auto mvcc_it = object_mvcc.lower_bound({ .oid = { .inode = oid.inode, .stripe = oid.stripe+1 }, .lsn = 0 }); + copy_id = 1; + if (mvcc_it != object_mvcc.begin()) { - 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.find((heap_object_lsn_t){ .oid = oid, .lsn = lsn }); - if (mvcc_it == object_mvcc.end()) + auto mvcc_it = object_mvcc.lower_bound({ .oid = { .inode = oid.inode, .stripe = oid.stripe+1 }, .lsn = 0 }); + if (mvcc_it == object_mvcc.begin()) { 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 - heap_object_t *obj_copy = mvcc_save_copy(obj); + // Save a copy of the object - only when overwriting + 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; diff --git a/src/blockstore/blockstore_heap.h b/src/blockstore/blockstore_heap.h index 12e6a6df..f5bb7a3c 100644 --- a/src/blockstore/blockstore_heap.h +++ b/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 diff --git a/src/test/test_heap.cpp b/src/test/test_heap.cpp index a0d9c76d..021d2655 100644 --- a/src/test/test_heap.cpp +++ b/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; - heap_object_t *obj = heap.lock_and_read_entry(oid, lsn); + uint64_t copy_id = 0; + 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(obj); + assert(heap.read_locked_entry(oid, copy_id) == obj); // small_write isn't MVCCed + + _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->version == 1); - assert(wr->offset == 0); - assert(wr->len == dsk.data_block_size); - assert(wr->location == 0); + assert(wr->lsn == 3); + assert(wr->version == 3); assert(wr->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE); - obj = heap.read_entry(oid, NULL); - assert(obj); + assert(heap.read_locked_entry(oid, copy_id) != obj); // big_write is MVCCed + obj = heap.read_locked_entry(oid, copy_id); assert(count_writes(obj) == 2); wr = obj->get_writes(); - assert(wr->lsn > lsn); - 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(wr->lsn == 2); 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; - heap_object_t *obj = heap.lock_and_read_entry(oid, lsn); + uint64_t copy_id = 0; + 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(obj->get_writes()->flags == BS_HEAP_BIG_WRITE|BS_HEAP_STABLE); + assert(count_writes(obj) == 2); 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.lock_and_read_entry(oid, lsn); + heap_object_t *obj = heap.read_entry(oid, NULL); 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; - heap_object_t *obj = heap.lock_and_read_entry(oid, read_lsn); + uint64_t copy_id = 0; + 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 be freed because it's not in MVCC - } + assert(!heap.is_buffer_area_free(16384, 4096)); // should not be freed because MVCC includes it 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; - heap_object_t *obj = heap.lock_and_read_entry(oid, read_lsn); + uint64_t copy_id = 0; + 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; - heap_object_t *obj = heap.lock_and_read_entry(oid, read_lsn); + uint64_t copy_id = 0; + 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; - obj = heap.lock_and_read_entry(oid, read2_lsn); + uint64_t copy2_id = 0; + 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 - heap.unlock_entry(oid, read2_lsn); + // free mvcc + 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);