Add a workaround for "double claim" bug with the new store

src/blockstore/blockstore_disk.cpp

@@ -96,6 +96,7 @@ void blockstore_disk_t::parse_config(std::map<std::string, std::string> & config
         (config["discard_on_start"] == "true" || config["discard_on_start"] == "1" || config["discard_on_start"] == "yes");
     gc_on_start = config.find("gc_on_start") == config.end() ||
         (config["gc_on_start"] == "true" || config["gc_on_start"] == "1" || config["gc_on_start"] == "yes");
+    skip_double_claim = (config["skip_double_claim"] == "true" || config["skip_double_claim"] == "1" || config["skip_double_claim"] == "yes");
     min_discard_size = parse_size(config["min_discard_size"]);
     if (!min_discard_size)
         min_discard_size = 1024*1024;

src/blockstore/blockstore_disk.h

@@ -59,6 +59,8 @@ struct blockstore_disk_t
     bool discard_on_start = false;
     // GC on start (new store)
     bool gc_on_start = true;
+    // Skip double claim conflicts on start (new store, temporary until the bug is found)
+    bool skip_double_claim = false;
     uint64_t min_discard_size = 1024*1024;
     uint64_t discard_granularity = 0;
 

src/blockstore/blockstore_heap.cpp

@@ -408,6 +408,13 @@ corrupted_object:
                     wr->inode, wr->stripe, wr->version, wr->big_intent().offset, wr->big_intent().len);
                 goto corrupted_object;
             }
+            if ((wr->type() == BS_HEAP_BIG_INTENT || wr->type() == BS_HEAP_BIG_WRITE) &&
+                wr->big().block_num >= dsk->block_count)
+            {
+                fprintf(stderr, "Error: big_write or big_intent entry %jx:%jx v%ju block_num is too large: %u > %lu. Metadata is incompatible with current parameters. ",
+                    wr->inode, wr->stripe, wr->version, wr->big_intent().block_num, dsk->block_count);
+                goto corrupted_object;
+            }
             handle_write(block_num, wr);
             block_offset += wr->size;
         }
@@ -593,6 +600,11 @@ void blockstore_heap_t::fill_recheck_queue()
 int blockstore_heap_t::mark_used_blocks()
 {
     int res = 0;
+    std::vector<heap_list_item_t*> used_by;
+    if (dsk->skip_double_claim)
+    {
+        used_by.resize(dsk->block_count);
+    }
     for (auto & pgp: block_index)
     {
         for (auto & ip: pgp.second)
@@ -646,10 +658,34 @@ int blockstore_heap_t::mark_used_blocks()
                     {
                         if (is_data_used(wr->big_location(this)))
                         {
-                            fprintf(stderr, "Error: double-claimed data block %u, second time by %jx:%jx l%ju\n",
+                            if (dsk->skip_double_claim)
-                                wr->big().block_num, wr->inode, wr->stripe, wr->lsn);
+                            {
-                            res = EDOM;
+                                // There is a BUG currently:
-                            return;
+                                // Sometimes (under unknown conditions) deletion entries are removed from the disk
+                                // earlier than previous big_writes.
+                                // Until it's fixed, we provide a way to ignore such objects on start.
+                                auto prev_li = used_by[wr->big().block_num];
+                                assert(prev_li);
+                                // Newer LSN must be trusted. Remove the older object.
+                                fprintf(stderr, "Block %u is double-claimed by entries %jx:%jx l%ju and %jx:%jx l%ju\n",
+                                    wr->big().block_num, prev_li->entry.inode, prev_li->entry.stripe, prev_li->entry.lsn, wr->inode, wr->stripe, wr->lsn);
+                                if (init_erase_double_claim(prev_li, li))
+                                {
+                                    return;
+                                }
+                            }
+                            else
+                            {
+                                fprintf(stderr, "Error: double-claimed data block %u, second time by %jx:%jx l%ju\n",
+                                    wr->big().block_num, wr->inode, wr->stripe, wr->lsn);
+                                res = EDOM;
+                                return;
+                            }
+                        }
+                        if (dsk->skip_double_claim)
+                        {
+                            // Record the object which uses the data block
+                            used_by[wr->big().block_num] = li;
                         }
                         use_data(wr->inode, wr->big_location(this));
                     }
@@ -673,6 +709,116 @@ int blockstore_heap_t::mark_used_blocks()
     return res;
 }
 
+void blockstore_heap_t::init_free_bad_entry(heap_entry_t *wr)
+{
+    if (wr->type() == BS_HEAP_SMALL_WRITE)
+    {
+        free_buffer_area(wr->inode, wr->small().location, wr->small().len);
+    }
+    else if (wr->type() == BS_HEAP_BIG_WRITE || wr->type() == BS_HEAP_BIG_INTENT)
+    {
+        free_data(wr->inode, wr->big_location(this));
+    }
+}
+
+void blockstore_heap_t::init_erase_bad_entry(heap_list_item_t *li)
+{
+    modify_alloc(li->block_num, [&](heap_block_info_t & inf)
+    {
+        for (size_t i = 0; i < inf.entries.size(); i++)
+        {
+            if (inf.entries[i] == li)
+            {
+                inf.entries.erase(inf.entries.begin()+i);
+                break;
+            }
+        }
+        inf.used_space -= li->entry.size;
+        inf.garbage_space -= (li->entry.is_garbage() ? li->entry.size : 0);
+    });
+    recheck_modified_blocks.insert(li->block_num);
+    unlink_list_item(li);
+}
+
+bool blockstore_heap_t::init_erase_double_claim(heap_list_item_t *prev_li, heap_list_item_t *cur_li)
+{
+    bool erase_prev = false;
+    bool erase_cur = false;
+    if (prev_li->entry.lsn < cur_li->entry.lsn)
+    {
+        erase_prev = true;
+        auto latest_li = prev_li;
+        while (latest_li->next)
+        {
+            latest_li = latest_li->next;
+        }
+        if (latest_li->entry.lsn >= cur_li->entry.lsn)
+        {
+            // LSN ranges intersect, erase both
+            erase_cur = true;
+        }
+    }
+    else
+    {
+        erase_cur = true;
+        auto latest_li = cur_li;
+        while (latest_li->next)
+        {
+            latest_li = latest_li->next;
+        }
+        if ((latest_li->entry.inode != prev_li->entry.inode ||
+            latest_li->entry.stripe != prev_li->entry.stripe) &&
+            latest_li->entry.lsn >= prev_li->entry.lsn)
+        {
+            // LSN ranges intersect, erase both
+            erase_prev = true;
+        }
+    }
+    if (erase_prev)
+    {
+        fprintf(stderr, "Erasing object %jx:%jx due to double-claim\n", prev_li->entry.inode, prev_li->entry.stripe);
+        auto erase_li = prev_li;
+        while (erase_li->next)
+        {
+            erase_li = erase_li->next;
+        }
+        bool overwritten = false;
+        while (erase_li)
+        {
+            auto prev_erase_li = erase_li->prev;
+            if (!overwritten)
+            {
+                init_free_bad_entry(&erase_li->entry);
+                overwritten = erase_li->entry.is_overwrite();
+            }
+            init_erase_bad_entry(erase_li);
+            erase_li = prev_erase_li;
+        }
+    }
+    if (erase_cur)
+    {
+        fprintf(stderr, "Erasing object %jx:%jx due to double-claim\n", cur_li->entry.inode, cur_li->entry.stripe);
+        auto erase_li = cur_li->next;
+        while (erase_li)
+        {
+            // Only newer entries are marked as used
+            auto next_erase_li = erase_li->next;
+            init_free_bad_entry(&erase_li->entry);
+            init_erase_bad_entry(erase_li);
+            erase_li = next_erase_li;
+        }
+        erase_li = cur_li;
+        // Older ones are not
+        while (erase_li)
+        {
+            auto prev_erase_li = erase_li->prev;
+            init_erase_bad_entry(erase_li);
+            erase_li = prev_erase_li;
+        }
+    }
+    return erase_cur;
+}
+
 void blockstore_heap_t::recheck_full_gc()
 {
     uint32_t block_num = 0;
@@ -2390,6 +2536,22 @@ void blockstore_heap_t::apply_inflight(heap_inflight_lsn_t & inflight)
 }
 
 void blockstore_heap_t::remove_list_item(heap_list_item_t *li)
+{
+    if (!li->next)
+    {
+        // The last freed entry must be a deletion
+        assert(!li->prev);
+        assert(li->entry.entry_type == BS_HEAP_DELETE|BS_HEAP_STABLE);
+    }
+    else if (!li->prev && li->next->entry.entry_type == (BS_HEAP_DELETE|BS_HEAP_STABLE))
+    {
+        // free BS_HEAP_DELETEs when all previous entries are also freed
+        mark_garbage(li->next->block_num, &li->next->entry, UINT32_MAX);
+    }
+    unlink_list_item(li);
+}
+
+void blockstore_heap_t::unlink_list_item(heap_list_item_t *li)
 {
     auto prev = li->prev;
     auto next = li->next;
@@ -2399,25 +2561,20 @@ void blockstore_heap_t::remove_list_item(heap_list_item_t *li)
     }
     if (!next)
     {
-        // The last freed entry must be a deletion
-        assert(!prev);
         auto wr = &li->entry;
-        assert(wr->entry_type == BS_HEAP_DELETE|BS_HEAP_STABLE);
         auto & pg_idx = block_index[get_pg_id(wr->inode, wr->stripe)];
         auto & inode_idx = pg_idx[wr->inode];
         heap_inode_map_t::iterator li_it;
         heap_list_item_t *old_li = NULL;
         inode_map_get(inode_idx, li_it, old_li, wr->stripe);
-        inode_map_erase(pg_idx, inode_idx, li_it, old_li);
+        if (!prev)
+            inode_map_erase(pg_idx, inode_idx, li_it, old_li);
+        else
+            inode_map_replace(inode_idx, li_it, prev);
     }
     else
     {
         next->prev = prev;
-        if (!prev && next->entry.entry_type == (BS_HEAP_DELETE|BS_HEAP_STABLE))
-        {
-            // free BS_HEAP_DELETEs when all previous entries are also freed
-            mark_garbage(next->block_num, &next->entry, UINT32_MAX);
-        }
     }
     if (li->entry.is_garbage())
     {

src/blockstore/blockstore_heap.h

@@ -220,6 +220,9 @@ class blockstore_heap_t
     bool validate_object(heap_entry_t *obj);
     void fill_recheck_queue();
     int mark_used_blocks();
+    void init_free_bad_entry(heap_entry_t *wr);
+    void init_erase_bad_entry(heap_list_item_t *li);
+    bool init_erase_double_claim(heap_list_item_t *prev_li, heap_list_item_t *cur_li);
     void recheck_full_gc();
     void recheck_buffer(heap_entry_t *cwr, uint8_t *buf);
     void defragment_block(uint32_t block_num);
@@ -229,6 +232,7 @@ class blockstore_heap_t
     int allocate_entry(uint32_t entry_size, uint32_t *block_num, bool allow_last_free);
     void insert_list_item(heap_list_item_t *li);
     void remove_list_item(heap_list_item_t *li);
+    void unlink_list_item(heap_list_item_t *li);
     int add_entry(uint32_t wr_size, uint32_t *modified_block, bool allow_last_free,
         bool explicit_complete, std::function<void(heap_entry_t *wr)> fill_entry);
     int add_simple(heap_entry_t *obj, uint64_t version, uint32_t *modified_block, uint32_t entry_type);

src/test/test_heap.cpp

@@ -20,7 +20,7 @@ static int count_writes(blockstore_heap_t & heap, heap_entry_t *obj)
     return n;
 }
 
-#define FREE_SPACE_BIT 0x8000
+#define BS_HEAP_FREE_SPACE 0xAB8F
 #define GARBAGE_BIT ((uint64_t)1 << 63)
 
 bool check_used_space(blockstore_heap_t & heap, blockstore_disk_t & dsk, uint32_t block_num)
@@ -30,10 +30,14 @@ bool check_used_space(blockstore_heap_t & heap, blockstore_disk_t & dsk, uint32_
     uint8_t *data = buf;
     uint8_t *end = data+dsk.meta_block_size;
     uint32_t used = 0;
-    while (data < end)
+    while (data <= end-4)
     {
         heap_entry_t *wr = ((heap_entry_t*)data);
-        if (!(wr->size & FREE_SPACE_BIT) && !wr->is_garbage())
+        if (wr->entry_type == BS_HEAP_FREE_SPACE)
+        {
+            break;
+        }
+        if (!wr->is_garbage())
         {
             used += wr->size;
         }
@@ -41,7 +45,7 @@ bool check_used_space(blockstore_heap_t & heap, blockstore_disk_t & dsk, uint32_
         {
             break;
         }
-        data += (wr->size & ~FREE_SPACE_BIT);
+        data += wr->size;
     }
     free(buf);
     return used == heap.get_meta_block_used_space(block_num);
@@ -2147,6 +2151,193 @@ void test_explicit_complete()
     printf("OK test_explicit_complete\n");
 }
 
+void test_skip_double_claim()
+{
+    blockstore_disk_t dsk;
+    _test_init(dsk, false);
+    dsk.skip_double_claim = true;
+    std::vector<uint8_t> tmp(dsk.meta_block_size);
+    std::vector<uint8_t> out(dsk.meta_block_size*3);
+    std::vector<uint8_t> buffer_area(dsk.journal_device_size);
+    heap_entry_t *wr1 = NULL, *wr2 = NULL, *wr3 = NULL, *wr4 = NULL;
+    uint32_t total_size = 0;
+
+    {
+        blockstore_heap_t heap(&dsk, buffer_area.data());
+
+        wr1 = (heap_entry_t*)(tmp.data() + total_size);
+        wr1->size = heap.get_big_entry_size();
+        wr1->entry_type = BS_HEAP_BIG_WRITE|BS_HEAP_STABLE;
+        wr1->lsn = 1;
+        wr1->inode = INODE_WITH_POOL(1, 1);
+        wr1->stripe = 0;
+        wr1->version = 1;
+        wr1->set_big_location(&heap, 0x40000); // <-- overwritten
+        wr1->crc32c = wr1->calc_crc32c();
+        total_size += wr1->size;
+
+        wr2 = (heap_entry_t*)(tmp.data() + total_size);
+        wr2->size = heap.get_big_entry_size();
+        wr2->entry_type = BS_HEAP_BIG_WRITE|BS_HEAP_STABLE;
+        wr2->lsn = 2;
+        wr2->inode = INODE_WITH_POOL(1, 1);
+        wr2->stripe = 0;
+        wr2->version = 2;
+        wr2->set_big_location(&heap, 0); // <-- double claimed
+        wr2->crc32c = wr2->calc_crc32c();
+        total_size += wr2->size;
+
+        wr3 = (heap_entry_t*)(tmp.data() + total_size);
+        wr3->size = heap.get_big_entry_size();
+        wr3->entry_type = BS_HEAP_BIG_WRITE|BS_HEAP_STABLE;
+        wr3->lsn = 3;
+        wr3->inode = INODE_WITH_POOL(1, 1);
+        wr3->stripe = 0x20000;
+        wr3->version = 1;
+        wr3->set_big_location(&heap, 0); // <-- double claimed
+        wr3->crc32c = wr3->calc_crc32c();
+        total_size += wr3->size;
+
+        wr4 = (heap_entry_t*)(tmp.data() + total_size);
+        wr4->size = heap.get_big_entry_size();
+        wr4->entry_type = BS_HEAP_BIG_WRITE; // <-- unstable
+        wr4->lsn = 4;
+        wr4->inode = INODE_WITH_POOL(1, 1);
+        wr4->stripe = 0x20000;
+        wr4->version = 2;
+        wr4->set_big_location(&heap, 0x20000);
+        wr4->crc32c = wr4->calc_crc32c();
+        total_size += wr4->size;
+
+        *(uint16_t*)(tmp.data() + total_size) = dsk.meta_block_size - total_size;
+        *(uint16_t*)(tmp.data() + total_size + 2) = BS_HEAP_FREE_SPACE;
+
+        uint64_t entries_loaded;
+        heap.load_blocks(0, dsk.meta_block_size, tmp.data(), false, entries_loaded);
+        heap.finish_load();
+        bool done = heap.recheck_small_writes([&](bool, uint64_t, uint64_t, uint8_t*, std::function<void()> cb) {}, 1);
+        assert(done);
+        heap.finish_recheck();
+        auto mod = heap.get_recheck_modified_blocks();
+        assert(mod.size() == 1);
+        assert(mod[0] == 0);
+
+        // [1 2] [3 4] - should erase first
+
+        object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
+        heap_entry_t *obj = heap.read_entry(oid);
+        assert(!obj);
+
+        oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0x20000 };
+        obj = heap.read_entry(oid);
+        assert(obj);
+
+        assert(heap.is_data_used(0));
+        assert(heap.is_data_used(0x20000));
+        assert(!heap.is_data_used(0x40000));
+
+        assert(check_used_space(heap, dsk, 0));
+
+        heap.get_meta_block(0, out.data());
+    }
+
+    {
+        blockstore_heap_t heap(&dsk, buffer_area.data());
+
+        wr1->lsn = 1;
+        wr1->crc32c = wr1->calc_crc32c();
+        wr2->lsn = 3;
+        wr2->crc32c = wr2->calc_crc32c();
+        wr3->lsn = 2;
+        wr3->crc32c = wr3->calc_crc32c();
+        wr4->lsn = 4;
+        wr4->crc32c = wr4->calc_crc32c();
+
+        uint64_t entries_loaded;
+        heap.load_blocks(0, dsk.meta_block_size, tmp.data(), false, entries_loaded);
+        heap.finish_load();
+        bool done = heap.recheck_small_writes([&](bool, uint64_t, uint64_t, uint8_t*, std::function<void()> cb) {}, 1);
+        assert(done);
+        heap.finish_recheck();
+        auto mod = heap.get_recheck_modified_blocks();
+        assert(mod.size() == 1);
+        assert(mod[0] == 0);
+
+        // [1 [2 3] 4] - intersect - should erase both
+
+        object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
+        heap_entry_t *obj = heap.read_entry(oid);
+        assert(!obj);
+
+        oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0x20000 };
+        obj = heap.read_entry(oid);
+        assert(!obj);
+
+        assert(!heap.is_data_used(0));
+        assert(!heap.is_data_used(0x20000));
+        assert(!heap.is_data_used(0x40000));
+
+        assert(check_used_space(heap, dsk, 0));
+
+        heap.get_meta_block(0, out.data()+dsk.meta_block_size);
+    }
+
+    {
+        blockstore_heap_t heap(&dsk, buffer_area.data());
+
+        // [3 4] [1 2] - should erase second
+
+        wr1->lsn = 3;
+        wr1->crc32c = wr1->calc_crc32c();
+        wr2->lsn = 4;
+        wr2->crc32c = wr2->calc_crc32c();
+        wr3->lsn = 1;
+        wr3->crc32c = wr3->calc_crc32c();
+        wr4->lsn = 2;
+        wr4->crc32c = wr4->calc_crc32c();
+
+        uint64_t entries_loaded;
+        heap.load_blocks(0, dsk.meta_block_size, tmp.data(), false, entries_loaded);
+        heap.finish_load();
+        bool done = heap.recheck_small_writes([&](bool, uint64_t, uint64_t, uint8_t*, std::function<void()> cb) {}, 1);
+        assert(done);
+        heap.finish_recheck();
+        auto mod = heap.get_recheck_modified_blocks();
+        assert(mod.size() == 1);
+        assert(mod[0] == 0);
+
+        object_id oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0 };
+        heap_entry_t *obj = heap.read_entry(oid);
+        assert(obj);
+
+        oid = { .inode = INODE_WITH_POOL(1, 1), .stripe = 0x20000 };
+        obj = heap.read_entry(oid);
+        assert(!obj);
+
+        assert(heap.is_data_used(0));
+        assert(!heap.is_data_used(0x20000));
+        assert(!heap.is_data_used(0x40000));
+
+        assert(check_used_space(heap, dsk, 0));
+
+        heap.get_meta_block(0, out.data()+dsk.meta_block_size*2);
+    }
+
+    // Validate persisted variants
+    for (int i = 0; i < 3; i++)
+    {
+        blockstore_heap_t heap(&dsk, buffer_area.data());
+        uint64_t entries_loaded;
+        heap.load_blocks(0, dsk.meta_block_size, out.data() + dsk.meta_block_size*i, false, entries_loaded);
+        heap.finish_load();
+        bool done = heap.recheck_small_writes([&](bool, uint64_t, uint64_t, uint8_t*, std::function<void()> cb) {}, 1);
+        assert(done);
+        heap.finish_recheck();
+        auto mod = heap.get_recheck_modified_blocks();
+        assert(mod.size() == 0);
+    }
+}
+
 // FIXME: Add a test for big_intent, incl. explicit_complete with big_intent over big_write over deletion over big_write :)
 
 int main(int narg, char *args[])
@@ -2187,5 +2378,6 @@ int main(int narg, char *args[])
     test_recalc_stats();
     test_redirect_intent_csums();
     test_explicit_complete();
+    test_skip_double_claim();
     return 0;
 }