Simplify compaction

src/blockstore/blockstore_flush.cpp

@@ -24,10 +24,12 @@ journal_flusher_t::journal_flusher_t(blockstore_impl_t *bs)
     {
         co[i].co_id = i;
         co[i].bs = bs;
-        if (bs->dsk.csum_block_size > bs->dsk.bitmap_granularity)
+        co[i].new_bmp = (uint8_t*)malloc_or_die(3*bs->dsk.clean_entry_bitmap_size);
+        co[i].new_ext_bmp = co[i].new_bmp + bs->dsk.clean_entry_bitmap_size;
+        co[i].punch_bmp = co[i].new_bmp + 2*bs->dsk.clean_entry_bitmap_size;
+        if (bs->dsk.csum_block_size > 0)
         {
             co[i].new_csums = (uint8_t*)malloc_or_die(bs->dsk.data_block_size / bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF));
-            co[i].new_bmp = (uint8_t*)malloc_or_die(bs->dsk.clean_entry_bitmap_size);
         }
         co[i].flusher = this;
     }
@@ -69,6 +71,8 @@ journal_flusher_co::~journal_flusher_co()
         free(new_bmp);
         new_bmp = NULL;
     }
+    new_ext_bmp = NULL;
+    punch_bmp = NULL;
     free_buffers();
 }
 
@@ -218,8 +222,21 @@ resume_1:
     free_buffers();
     copy_count = 0;
     fsynced_lsn = bs->heap->get_fsynced_lsn();
+    bitmap_copied = false;
+    memset(new_bmp, 0, bs->dsk.clean_entry_bitmap_size);
+    csum_copy.clear();
     compact_info = bs->heap->iterate_compaction(cur_obj, fsynced_lsn, flusher->force_start, [&](heap_entry_t *wr)
     {
+        if (!bitmap_copied)
+        {
+            memcpy(new_ext_bmp, wr->get_ext_bitmap(bs->heap), bs->dsk.clean_entry_bitmap_size);
+            bitmap_copied = true;
+        }
+        bitmap_set(new_bmp, wr->small().offset, wr->small().len, bs->dsk.bitmap_granularity);
+        if (bs->dsk.csum_block_size && bs->dsk.csum_block_size <= bs->dsk.bitmap_granularity)
+        {
+            csum_copy.push_back(wr);
+        }
         if (wr->type() == BS_HEAP_SMALL_WRITE ||
             wr->type() == BS_HEAP_INTENT_WRITE && bs->dsk.csum_block_size > bs->dsk.bitmap_granularity)
         {
@@ -234,10 +251,28 @@ resume_1:
         bs->heap->unlock_entry(cur_oid);
         goto resume_0;
     }
+    mem_or(new_bmp, compact_info.clean_wr->get_int_bitmap(bs->heap), bs->dsk.clean_entry_bitmap_size);
+    if (!bitmap_copied)
+    {
+        memcpy(new_ext_bmp, compact_info.clean_wr->get_ext_bitmap(bs->heap), bs->dsk.clean_entry_bitmap_size);
+        bitmap_copied = true;
+    }
+    if (bs->dsk.csum_block_size && bs->dsk.csum_block_size <= bs->dsk.bitmap_granularity)
+    {
+        memcpy(new_csums, compact_info.clean_wr->get_checksums(bs->heap), bs->dsk.data_block_size/bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF));
+        for (size_t i = csum_copy.size(); i > 0; i--)
+        {
+            auto wr = csum_copy[i-1];
+            memcpy(new_csums + wr->small().offset/bs->dsk.csum_block_size*(bs->dsk.data_csum_type & 0xFF),
+                wr->get_checksums(bs->heap), wr->small().len/bs->dsk.csum_block_size*(bs->dsk.data_csum_type & 0xFF));
+        }
+        csum_copy.clear();
+    }
+    clean_loc = compact_info.clean_wr->big_location(bs->heap);
     flusher->active_flushers++;
     if (bs->log_level > 10)
     {
-        printf("Compacting %jx:%jx l%ju .. l%ju\n", cur_oid.inode, cur_oid.stripe, compact_info.clean_lsn, compact_info.compact_lsn);
+        printf("Compacting %jx:%jx l%ju .. l%ju\n", cur_oid.inode, cur_oid.stripe, compact_info.clean_wr->lsn, compact_info.compact_lsn);
     }
     overwrite_start = overwrite_end = 0;
     if (read_vec.size() > 0)
@@ -286,7 +321,7 @@ resume_3:
     {
 resume_4:
         modified_block = UINT32_MAX;
-        res = bs->heap->add_punch_holes(cur_obj, compact_info.clean_lsn, compact_info.clean_version, new_bmp, new_csums, &modified_block);
+        res = bs->heap->punch_holes(compact_info.clean_wr, punch_bmp, new_csums, &modified_block);
         if (res == ENOENT)
         {
             // Abort compaction
@@ -330,7 +365,7 @@ resume_9:
             data->iov = (struct iovec){ read_vec[i].buf + (read_vec[i].copy_flags & COPY_BUF_PADDED
                 ? read_vec[i].offset - read_vec[i].disk_offset : 0), (size_t)read_vec[i].len };
             data->callback = simple_callback_w;
-            io_uring_prep_writev(sqe, bs->dsk.data_fd, &data->iov, 1, bs->dsk.data_offset + compact_info.clean_loc + read_vec[i].offset);
+            io_uring_prep_writev(sqe, bs->dsk.data_fd, &data->iov, 1, bs->dsk.data_offset + clean_loc + read_vec[i].offset);
             wait_count++;
         }
     }
@@ -358,7 +393,8 @@ resume_11:
         flusher->flushing.erase(cur_oid);
         goto resume_0;
     }
-    bs->heap->add_compact(cur_obj, compact_info.compact_lsn, &modified_block, new_csums);
+    bs->heap->add_compact(cur_obj, compact_info.compact_version, compact_info.compact_lsn, clean_loc,
+        compact_info.do_delete, &modified_block, new_bmp, new_ext_bmp, new_csums);
 resume_12:
 resume_13:
     if (!write_meta_block(12))
@@ -414,11 +450,11 @@ void journal_flusher_co::fill_partial_checksum_blocks()
                 .copy_flags = COPY_BUF_DATA | copy_flags,
                 .offset = blk_begin,
                 .len = blk_end - blk_begin,
-                .disk_loc = compact_info.clean_loc,
+                .disk_loc = clean_loc,
                 .disk_offset = blk_begin,
                 .disk_len = blk_end - blk_begin,
                 .buf = (uint8_t*)memalign_or_die(MEM_ALIGNMENT, blk_end - blk_begin),
-                .wr_lsn = compact_info.clean_lsn,
+                .wr = compact_info.clean_wr,
             });
         }
         auto & vec = read_vec[read_vec.size()-1];
@@ -468,22 +504,16 @@ int journal_flusher_co::check_and_punch_checksums()
         auto & vec = read_vec[i];
         if (!(vec.copy_flags & (COPY_BUF_COALESCED|COPY_BUF_ZERO|COPY_BUF_SKIP_CSUM)))
         {
-            heap_entry_t *wr = cur_obj;
+            uint32_t *csums = (uint32_t*)(vec.wr->get_checksums(bs->heap)
-            while (wr && wr->lsn != vec.wr_lsn) // FIXME: Skip compacted
-            {
-                wr = bs->heap->prev(wr);
-            }
-            assert(wr);
-            uint32_t *csums = (uint32_t*)(wr->get_checksums(bs->heap)
                 + (vec.disk_offset/bs->dsk.csum_block_size)*(bs->dsk.data_csum_type & 0xFF)
-                - ((wr->type() == BS_HEAP_BIG_WRITE || wr->type() == BS_HEAP_BIG_INTENT)
+                - ((vec.wr->type() == BS_HEAP_BIG_WRITE || vec.wr->type() == BS_HEAP_BIG_INTENT)
-                    ? 0 : (wr->small().offset/bs->dsk.csum_block_size)*(bs->dsk.data_csum_type & 0xFF)));
+                    ? 0 : (vec.wr->small().offset/bs->dsk.csum_block_size)*(bs->dsk.data_csum_type & 0xFF)));
             bs->heap->calc_block_checksums(
-                csums, vec.buf, wr->get_int_bitmap(bs->heap), vec.disk_offset, vec.disk_offset+vec.disk_len, false,
+                csums, vec.buf, vec.wr->get_int_bitmap(bs->heap), vec.disk_offset, vec.disk_offset+vec.disk_len, false,
                 [&](uint32_t mismatch_pos, uint32_t expected_csum, uint32_t real_csum)
                 {
                     printf("Checksum mismatch during compaction in object %jx:%jx v%ju, offset 0x%x in %s area at offset 0x%jx: got %08x, expected %08x\n",
-                        cur_oid.inode, cur_oid.stripe, wr->version, mismatch_pos,
+                        cur_oid.inode, cur_oid.stripe, vec.wr->version, mismatch_pos,
                         (vec.copy_flags & COPY_BUF_JOURNAL ? "buffer" : "data"),
                         vec.disk_loc+vec.disk_offset, real_csum, expected_csum);
                     csum_ok = false;
@@ -502,24 +532,8 @@ int journal_flusher_co::check_and_punch_checksums()
         // Nothing to do
         return 0;
     }
-    heap_entry_t *clean_wr = NULL;
+    memcpy(punch_bmp, compact_info.clean_wr->get_int_bitmap(bs->heap), bs->dsk.clean_entry_bitmap_size);
-    for (auto wr = cur_obj; wr; wr = bs->heap->prev(wr))
+    memcpy(new_csums, compact_info.clean_wr->get_checksums(bs->heap), bs->dsk.data_block_size/bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF));
-    {
-        if (wr->is_overwrite() && wr->lsn > compact_info.clean_lsn &&
-            wr->lsn <= fsynced_lsn)
-        {
-            // Object is overwritten, abort compaction
-            return ENOENT;
-        }
-        if (wr->lsn == compact_info.clean_lsn)
-        {
-            clean_wr = wr;
-            break;
-        }
-    }
-    assert(clean_wr);
-    memcpy(new_bmp, clean_wr->get_int_bitmap(bs->heap), bs->dsk.clean_entry_bitmap_size);
-    memcpy(new_csums, clean_wr->get_checksums(bs->heap), bs->dsk.data_block_size/bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF));
     // Clear bits
     for (auto & vec: read_vec)
     {
@@ -530,7 +544,7 @@ int journal_flusher_co::check_and_punch_checksums()
         if (!(vec.copy_flags & COPY_BUF_COALESCED) &&
             ((vec.offset % bs->dsk.csum_block_size) || (vec.len % bs->dsk.csum_block_size)))
         {
-            bitmap_clear(new_bmp, vec.offset, vec.len, bs->dsk.bitmap_granularity);
+            bitmap_clear(punch_bmp, vec.offset, vec.len, bs->dsk.bitmap_granularity);
         }
     }
     // Update partial block checksums
@@ -539,7 +553,7 @@ int journal_flusher_co::check_and_punch_checksums()
         if (vec.copy_flags & COPY_BUF_CSUM_FILL)
         {
             uint32_t csum_off = vec.offset/bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF);
-            bs->heap->calc_block_checksums((uint32_t*)(new_csums+csum_off), vec.buf, new_bmp, vec.offset, vec.offset+vec.len, true, NULL);
+            bs->heap->calc_block_checksums((uint32_t*)(new_csums+csum_off), vec.buf, punch_bmp, vec.offset, vec.offset+vec.len, true, NULL);
         }
     }
     // Modified, we should add_punch_holes and then write the block to disk
@@ -552,32 +566,7 @@ bool journal_flusher_co::calc_block_checksums()
     {
         return true;
     }
-    heap_entry_t *clean_wr = NULL;
+    memcpy(new_csums, compact_info.clean_wr->get_checksums(bs->heap), bs->dsk.data_block_size/bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF));
-    for (auto wr = cur_obj; wr; wr = bs->heap->prev(wr))
-    {
-        if (wr->is_overwrite() && wr->lsn > compact_info.clean_lsn &&
-            wr->lsn <= fsynced_lsn)
-        {
-            // Object is overwritten, abort compaction
-            return false;
-        }
-        if (wr->lsn == compact_info.clean_lsn)
-        {
-            clean_wr = wr;
-            break;
-        }
-    }
-    assert(clean_wr);
-    memcpy(new_bmp, clean_wr->get_int_bitmap(bs->heap), bs->dsk.clean_entry_bitmap_size);
-    memcpy(new_csums, clean_wr->get_checksums(bs->heap), bs->dsk.data_block_size/bs->dsk.csum_block_size * (bs->dsk.data_csum_type & 0xFF));
-    // Set bits
-    for (auto & vec: read_vec)
-    {
-        if (!(vec.copy_flags & (COPY_BUF_COALESCED|COPY_BUF_CSUM_FILL)))
-        {
-            bitmap_set(new_bmp, vec.offset, vec.len, bs->dsk.bitmap_granularity);
-        }
-    }
     // Update block checksums
     size_t i = 0;
     while (i < read_vec.size() && !(read_vec[i].copy_flags & COPY_BUF_CSUM_FILL))

src/blockstore/blockstore_flush.h

@@ -6,7 +6,7 @@ struct copy_buffer_t
     uint32_t copy_flags;
     uint64_t offset, len, disk_loc, disk_offset, disk_len;
     uint8_t *buf;
-    uint64_t wr_lsn;
+    heap_entry_t *wr;
 };
 
 struct meta_sector_t
@@ -38,6 +38,8 @@ class journal_flusher_co
     struct ring_data_t *data;
     uint8_t *new_csums = NULL;
     uint8_t *new_bmp = NULL;
+    uint8_t *punch_bmp = NULL;
+    uint8_t *new_ext_bmp = NULL;
 
     std::function<void(ring_data_t*)> simple_callback_r, simple_callback_w;
 
@@ -45,10 +47,13 @@ class journal_flusher_co
     heap_entry_t *cur_obj;
     uint64_t fsynced_lsn;
     heap_compact_t compact_info;
+    uint64_t clean_loc;
     uint32_t modified_block;
+    bool bitmap_copied;
     bool should_repeat;
 
     std::vector<copy_buffer_t> read_vec;
+    std::vector<heap_entry_t*> csum_copy;
     uint32_t overwrite_start, overwrite_end;
     int i, res;
     bool read_to_fill_incomplete;

src/blockstore/blockstore_heap.cpp

@@ -1053,10 +1053,7 @@ int blockstore_heap_t::add_small_write(object_id oid, heap_entry_t *old_head, ui
         if (bitmap)
             memcpy(wr->get_ext_bitmap(this), bitmap, dsk->clean_entry_bitmap_size);
         else if (old_head)
-        {
-            old_head = read_entry(oid);
             memcpy(wr->get_ext_bitmap(this), old_head->get_ext_bitmap(this), dsk->clean_entry_bitmap_size);
-        }
         else
             memset(wr->get_ext_bitmap(this), 0, dsk->clean_entry_bitmap_size);
         calc_checksums(wr, (uint8_t*)data, true);
@@ -1136,139 +1133,42 @@ int blockstore_heap_t::add_big_intent(object_id oid, heap_entry_t *old_head, uin
     });
 }
 
-int blockstore_heap_t::add_compact(heap_entry_t *obj, uint64_t to_lsn, uint32_t *modified_block, uint8_t *new_csums)
+int blockstore_heap_t::add_compact(heap_entry_t *obj, uint64_t compact_version, uint64_t compact_lsn, uint64_t compact_location,
+    bool do_delete, uint32_t *modified_block, uint8_t *new_int_bitmap, uint8_t *new_ext_bitmap, uint8_t *new_csums)
 {
-    // Slightly tricky - we don't want to compact an object if it's overwritten or deleted during compaction
+    if (do_delete)
     {
-        heap_entry_t *old_wr = obj;
+        return add_entry(get_simple_entry_size(), modified_block, false, [&](heap_entry_t *wr)
-        while (old_wr && !old_wr->is_overwrite())
         {
-            old_wr = prev(old_wr);
+            wr->entry_type = BS_HEAP_DELETE|BS_HEAP_STABLE;
-        }
+            wr->inode = obj->inode;
-        if (!old_wr)
+            wr->stripe = obj->stripe;
-        {
+            wr->version = 0;
-            // Check if we have to remove the object at all
+            wr->lsn = compact_lsn;
-            bool has_entry = false;
+        });
-            iterate_with_stable(obj, obj->lsn, [&](heap_entry_t *old_wr, bool stable)
-            {
-                has_entry = true;
-                return false;
-            });
-            if (!has_entry)
-            {
-                uint64_t compact_lsn = obj->lsn;
-                return add_entry(get_simple_entry_size(), modified_block, false, [&](heap_entry_t *wr)
-                {
-                    wr->entry_type = BS_HEAP_DELETE|BS_HEAP_STABLE;
-                    wr->inode = obj->inode;
-                    wr->stripe = obj->stripe;
-                    wr->version = 0;
-                    wr->lsn = compact_lsn;
-                });
-            }
-        }
-        else if (old_wr->lsn > to_lsn)
-        {
-            return ENOENT;
-        }
     }
-    auto oid = (object_id){ .inode = obj->inode, .stripe = obj->stripe };
     uint32_t wr_size = get_big_entry_size();
     return add_entry(wr_size, modified_block, true, [&](heap_entry_t *new_wr)
     {
-        // obj and old_wr are invalid, re-read them - the block could have been compacted
+        new_wr->entry_type = BS_HEAP_BIG_WRITE|BS_HEAP_STABLE;
-        obj = read_entry(oid);
-        while (obj && obj->lsn > to_lsn)
-        {
-            // skip new entries
-            obj = prev(obj);
-        }
-        assert(obj);
-        new_wr->entry_type = BS_HEAP_BIG_WRITE | BS_HEAP_STABLE;
         new_wr->inode = obj->inode;
         new_wr->stripe = obj->stripe;
-        memset(new_wr->get_int_bitmap(this), 0, dsk->clean_entry_bitmap_size);
-        bool need_copy = false, bitmap_copied = false;
-        std::vector<heap_entry_t*> cswr;
-        // Determine the latest compacted entry
-        uint64_t compact_lsn = obj->lsn, compact_version = obj->version;
-        iterate_with_stable(obj, to_lsn, [&](heap_entry_t *old_wr, bool stable)
-        {
-            if (!stable)
-            {
-                // This entry is still uncommitted, so it's not compacted and makes a gap
-                compact_lsn = old_wr->lsn-1;
-                compact_version = prev(old_wr)->version;
-            }
-            return !old_wr->is_overwrite();
-        });
         new_wr->version = compact_version;
         new_wr->lsn = compact_lsn;
-        bool found = false;
+        new_wr->set_big_location(this, compact_location);
-        iterate_with_stable(obj, compact_lsn, [&](heap_entry_t *old_wr, bool stable)
+        memcpy(new_wr->get_int_bitmap(this), new_int_bitmap, dsk->clean_entry_bitmap_size);
-        {
+        memcpy(new_wr->get_ext_bitmap(this), new_ext_bitmap, dsk->clean_entry_bitmap_size);
-            if (!stable)
+        if (dsk->data_csum_type && new_csums)
-                return true;
+            memcpy(new_wr->get_checksums(this), new_csums, dsk->data_block_size/dsk->csum_block_size*(dsk->data_csum_type & 0xFF));
-            if (old_wr->type() == BS_HEAP_SMALL_WRITE || old_wr->type() == BS_HEAP_INTENT_WRITE)
-            {
-                if (!bitmap_copied)
-                {
-                    memcpy(new_wr->get_ext_bitmap(this), old_wr->get_ext_bitmap(this), dsk->clean_entry_bitmap_size);
-                    bitmap_copied = true;
-                }
-                bitmap_set(new_wr->get_int_bitmap(this), old_wr->small().offset, old_wr->small().len, dsk->bitmap_granularity);
-                if (dsk->data_csum_type && old_wr->small().len > 0)
-                {
-                    if (dsk->csum_block_size == dsk->bitmap_granularity)
-                        cswr.push_back(old_wr);
-                    else
-                        need_copy = true;
-                }
-            }
-            else if (old_wr->type() == BS_HEAP_BIG_WRITE)
-            {
-                found = true;
-                new_wr->big().block_num = old_wr->big().block_num;
-                mem_or(new_wr->get_int_bitmap(this), old_wr->get_int_bitmap(this), dsk->clean_entry_bitmap_size);
-                if (need_copy)
-                    memcpy(new_wr->get_checksums(this), new_csums, dsk->data_block_size/dsk->csum_block_size*(dsk->data_csum_type & 0xFF));
-                else if (dsk->data_csum_type)
-                {
-                    // Copy checksums in the reverse order
-                    memcpy(new_wr->get_checksums(this), old_wr->get_checksums(this), dsk->data_block_size/dsk->csum_block_size*(dsk->data_csum_type & 0xFF));
-                    for (size_t i = cswr.size(); i > 0; i--)
-                    {
-                        heap_entry_t *old_wr = cswr[i-1];
-                        memcpy(new_wr->get_checksums(this) + old_wr->small().offset/dsk->csum_block_size*(dsk->data_csum_type & 0xFF),
-                            old_wr->get_checksums(this), old_wr->small().len/dsk->csum_block_size*(dsk->data_csum_type & 0xFF));
-                    }
-                }
-                return false;
-            }
-            return true;
-        });
-        assert(found);
     });
 }
 
 // A bit of a hack: overwrite the bitmap in an existing entry
-int blockstore_heap_t::add_punch_holes(heap_entry_t *obj, uint64_t to_lsn, uint64_t version, uint8_t *new_bitmap, uint8_t *new_csums, uint32_t *modified_block)
+int blockstore_heap_t::punch_holes(heap_entry_t *wr, uint8_t *new_bitmap, uint8_t *new_csums, uint32_t *modified_block)
 {
     assert(dsk->data_csum_type && dsk->csum_block_size > dsk->bitmap_granularity);
     assert(new_csums);
-    // Abort if the object is overwritten or deleted during compaction
+    uint32_t block_num = list_item(wr)->block_num;
-    heap_entry_t *wr = obj;
-    while (wr && wr->lsn != to_lsn && !wr->is_overwrite())
-    {
-        wr = prev(wr);
-    }
-    if (!wr || wr->lsn > to_lsn)
-    {
-        return ENOENT;
-    }
-    auto & idx = block_index[get_pg_id(obj->inode, obj->stripe)][obj->inode][obj->stripe];
-    assert(idx.ptr);
-    uint32_t block_num = idx.ptr->block_num;
     auto & inf = block_info.at(block_num);
     if (inf.is_writing)
     {
@@ -1566,19 +1466,23 @@ void blockstore_heap_t::iterate_with_stable(heap_entry_t *obj, uint64_t max_lsn,
     }
 }
 
+// Interesting cases:
+// 1) BIG_STABLE(v1 l1) SMALL(v2 l2) SMALL(v3 l3) SMALL(v4 l4) ROLLBACK(v3 l5) COMMIT(v2 l6)
+//    -> compact by adding BIG_STABLE(v2 l2)
+// 2) BIG_STABLE(v1 l1) DELETE(l2) BIG_UNSTABLE(v1 l3) ROLLBACK(v0 l4)
+//    -> compact by adding DELETE(l4)
+// 3) BIG_STABLE(v1 l1) SMALL(v2 l2) SMALL(v3 l3) ROLLBACK(v2 l4) SMALL(v3 l5) COMMIT(v3 l6)
+//    -> compact by adding BIG_STABLE(v3 l6) and skip l3
+// 4) BIG_STABLE(v1 l1) SMALL_STABLE(v2 l2) BIG_UNSTABLE(v3 l3)
+//    -> skip compaction of l2 into l1 if not under pressure
 heap_compact_t blockstore_heap_t::iterate_compaction(heap_entry_t *obj, uint64_t fsynced_lsn, bool under_pressure, std::function<void(heap_entry_t*)> small_wr_cb)
 {
     heap_compact_t res = {};
     uint64_t commit_version = 0, rollback_version = UINT64_MAX;
     bool has_small = false;
+    res.do_delete = true;
     for (heap_entry_t *wr = obj; wr; wr = prev(wr))
     {
-        // 1) 1 2 3 ROLLBACK(2) COMMIT(3) -> impossible
-        // 2) 1 2 3 4 ROLLBACK(3) COMMIT(2) -> OK
-        // 3) 1 2 3 ROLLBACK(2) 3 COMMIT(3) -> first 3 shouldn't be treated as stable
-        // 4) 1 2 3 COMMIT(3) ROLLBACK(2) -> impossible
-        //    I.e. a rollback always has version >= previous commit
-        // 5) 1 2 3 4 5 ROLLBACK(4) 5 ROLLBACK(3)
         if (wr->type() == BS_HEAP_ROLLBACK)
         {
             if (wr->lsn <= fsynced_lsn && !res.compact_lsn)
@@ -1596,54 +1500,57 @@ heap_compact_t blockstore_heap_t::iterate_compaction(heap_entry_t *obj, uint64_t
                 res.compact_lsn = wr->lsn;
                 res.compact_version = wr->version;
             }
+            res.do_delete = false;
             commit_version = wr->version;
             continue;
         }
         bool rolled_back = (wr->version > rollback_version);
-        bool stable = !rolled_back && ((wr->entry_type & BS_HEAP_STABLE) || (wr->version <= commit_version));
+        if (rolled_back)
-        if (!stable || wr->lsn > fsynced_lsn)
         {
-            // Skip unstable or non-fsynced writes
+            continue;
+        }
+        bool stable = (wr->entry_type & BS_HEAP_STABLE);
+        bool committed = (wr->version <= commit_version);
+        if (!stable && !committed || wr->lsn > fsynced_lsn)
+        {
+            // Unstable and non-fsynced writes can't be compacted yet
+            res.do_delete = false;
+            res.compact_lsn = 0;
+            res.compact_version = 0;
             if (!under_pressure && (wr->type() == BS_HEAP_BIG_WRITE || wr->type() == BS_HEAP_DELETE))
             {
                 // We may postpone compaction if we have an unstable overwrite when not under pressure
-                res.compact_lsn = 0;
-                res.compact_version = 0;
                 return res;
             }
             continue;
         }
-        if (wr->type() == BS_HEAP_BIG_WRITE)
+        if (wr->type() == BS_HEAP_BIG_WRITE || wr->type() == BS_HEAP_BIG_INTENT)
         {
-            // Stable big_write is here
+            // Big_write to merge small_writes into is here
-            res.clean_loc = wr->big_location(this);
+            if (!stable && !res.compact_lsn)
-            res.clean_version = wr->version;
+            {
-            res.clean_lsn = wr->lsn;
+                res.compact_lsn = wr->lsn;
+                res.compact_version = wr->version;
+            }
+            res.clean_wr = wr;
+            res.do_delete = false;
             return res;
         }
         if (wr->type() == BS_HEAP_DELETE)
         {
             // Object is deleted
-            assert(!has_small);
+            assert(!has_small && stable); // unstable deletes are not supported
-            if (wr->entry_type & BS_HEAP_STABLE)
-            {
-                // Already have the stable bit, no need to generate a compaction entry
-                res.compact_lsn = 0;
-                res.compact_version = 0;
-            }
             return res;
         }
-        // We finally have something compactable
+        assert(wr->type() == BS_HEAP_SMALL_WRITE || wr->type() == BS_HEAP_INTENT_WRITE);
         if (!res.compact_lsn)
         {
             res.compact_lsn = wr->lsn;
             res.compact_version = wr->version;
         }
-        if (wr->type() == BS_HEAP_SMALL_WRITE || wr->type() == BS_HEAP_INTENT_WRITE)
+        res.do_delete = false;
-        {
+        has_small = true;
-            has_small = true;
+        small_wr_cb(wr);
-            small_wr_cb(wr);
-        }
     }
     return res;
 }

src/blockstore/blockstore_heap.h

@@ -132,7 +132,8 @@ struct heap_inflight_lsn_t
 struct heap_compact_t
 {
     uint64_t compact_lsn, compact_version;
-    uint64_t clean_lsn, clean_version, clean_loc;
+    heap_entry_t *clean_wr;
+    bool do_delete;
 };
 
 struct heap_idx_t
@@ -249,9 +250,10 @@ public:
     int add_big_intent(object_id oid, heap_entry_t *old_head, uint64_t version,
         uint32_t offset, uint32_t len, uint8_t *bitmap, uint8_t *data, uint8_t *checksums, uint32_t *modified_block);
     // adds a compacted up to <version> entry to an object
-    int add_compact(heap_entry_t *obj, uint64_t to_lsn, uint32_t *modified_block, uint8_t *new_csums);
+    int add_compact(heap_entry_t *obj, uint64_t compact_version, uint64_t compact_lsn, uint64_t compact_location,
-    // "punch holes" in a big_entry and make a duplicate big_entry
+        bool do_delete, uint32_t *modified_block, uint8_t *new_int_bitmap, uint8_t *new_ext_bitmap, uint8_t *new_csums);
-    int add_punch_holes(heap_entry_t *obj, uint64_t to_lsn, uint64_t version, uint8_t *new_bitmap, uint8_t *new_csums, uint32_t *modified_block);
+    // "punch holes" in a big_entry
+    int punch_holes(heap_entry_t *wr, uint8_t *new_bitmap, uint8_t *new_csums, uint32_t *modified_block);
     // stabilize an unstable object version
     // return 0 if OK, ENOENT if not exists
     int add_commit(heap_entry_t *obj, uint64_t version, uint32_t *modified_block);

src/blockstore/blockstore_read.cpp

@@ -192,7 +192,7 @@ uint32_t blockstore_impl_t::prepare_read_simple(std::vector<copy_buffer_t> & rea
                 .disk_offset = start,
                 .disk_len = end-start,
                 .buf = buffer_area + wr->small().location + start - wr->small().offset,
-                .wr_lsn = wr->lsn,
+                .wr = wr,
             });
         }
         else if (dsk.csum_block_size <= dsk.bitmap_granularity)
@@ -255,7 +255,6 @@ uint32_t blockstore_impl_t::prepare_read_simple(std::vector<copy_buffer_t> & rea
 void blockstore_impl_t::prepare_disk_read(std::vector<copy_buffer_t> & read_vec, int pos, heap_entry_t *obj, heap_entry_t *wr,
     uint32_t blk_start, uint32_t blk_end, uint32_t start, uint32_t end, uint32_t copy_flags)
 {
-    // Only one INTENT_WRITE is allowed at a time
     uint64_t loc = 0;
     if (wr->type() == BS_HEAP_INTENT_WRITE)
     {
@@ -283,7 +282,7 @@ void blockstore_impl_t::prepare_disk_read(std::vector<copy_buffer_t> & read_vec,
         .disk_loc = loc,
         .disk_offset = blk_start,
         .disk_len = blk_end - blk_start,
-        .wr_lsn = wr->lsn,
+        .wr = wr,
     };
     if (blk_start != start || blk_end != end)
     {
@@ -291,7 +290,7 @@ void blockstore_impl_t::prepare_disk_read(std::vector<copy_buffer_t> & read_vec,
         vec.copy_flags |= COPY_BUF_PADDED;
         if (pos > 0 && read_vec.size() >= pos &&
             read_vec[pos-1].copy_flags == vec.copy_flags &&
-            read_vec[pos-1].wr_lsn == vec.wr_lsn &&
+            read_vec[pos-1].wr == vec.wr &&
             read_vec[pos-1].disk_offset <= vec.disk_offset &&
             read_vec[pos-1].disk_offset+read_vec[pos-1].disk_len >= blk_end)
         {
@@ -383,7 +382,6 @@ void blockstore_impl_t::handle_read_event(ring_data_t *data, blockstore_op_t *op
 
 bool blockstore_impl_t::verify_read_checksums(blockstore_op_t *op)
 {
-    heap_entry_t *obj = heap->read_entry(op->oid);
     auto & rv = PRIV(op)->read_vec;
     for (auto & vec: rv)
     {
@@ -393,16 +391,12 @@ bool blockstore_impl_t::verify_read_checksums(blockstore_op_t *op)
             memcpy(op->buf + vec.offset - op->offset, vec.buf + vec.offset - vec.disk_offset, vec.len);
         if (vec.copy_flags & (COPY_BUF_COALESCED|COPY_BUF_SKIP_CSUM))
             continue;
-        heap_entry_t *wr = obj;
-        while (wr && wr->lsn != vec.wr_lsn)
-            wr = heap->prev(wr);
-        assert(wr);
         uint8_t *buf = vec.buf ? vec.buf : (op->buf + vec.offset - op->offset);
-        uint32_t *csums = (uint32_t*)(wr->get_checksums(heap)
+        uint32_t *csums = (uint32_t*)(vec.wr->get_checksums(heap)
             + (vec.disk_offset/dsk.csum_block_size)*(dsk.data_csum_type & 0xFF)
-            - ((wr->type() == BS_HEAP_BIG_WRITE || wr->type() == BS_HEAP_BIG_INTENT)
+            - ((vec.wr->type() == BS_HEAP_BIG_WRITE || vec.wr->type() == BS_HEAP_BIG_INTENT)
-                ? 0 : (wr->small().offset/dsk.csum_block_size)*(dsk.data_csum_type & 0xFF)));
+                ? 0 : (vec.wr->small().offset/dsk.csum_block_size)*(dsk.data_csum_type & 0xFF)));
-        if (!heap->calc_block_checksums(csums, buf, wr->get_int_bitmap(heap),
+        if (!heap->calc_block_checksums(csums, buf, vec.wr->get_int_bitmap(heap),
             vec.disk_offset, vec.disk_offset+vec.disk_len, false, [&](uint32_t mismatch_pos, uint32_t expected_csum, uint32_t real_csum)
             {
                 printf(

src/test/test_heap.cpp

@@ -328,12 +328,16 @@ void test_defrag_block()
         // The next write should be rejected because allowing it would block compaction
         assert(_test_do_big_write(heap, dsk, 1, (nwr+1)*0x20000, 1, (nwr+1)*0x20000, true, 0, 0, buffer_area.data()) == ENOSPC);
         // Compact all small writes
+        uint8_t bitmap[dsk.clean_entry_bitmap_size];
+        memset(bitmap, 0xFF, dsk.clean_entry_bitmap_size);
         uint32_t mblock = 999999;
         for (uint32_t i = 0; i < nwr; i++)
         {
             auto obj = heap.read_entry((object_id){ .inode = INODE_WITH_POOL(1, 1), .stripe = i*0x20000 });
             assert(obj);
-            int res = heap.add_compact(obj, obj->lsn, &mblock, NULL);
+            assert(heap.prev(obj)->entry_type == (BS_HEAP_BIG_WRITE|BS_HEAP_STABLE));
+            int res = heap.add_compact(obj, obj->version, obj->lsn, heap.prev(obj)->big_location(&heap),
+                false, &mblock, bitmap, bitmap, NULL);
             assert(res == 0);
             heap.start_block_write(mblock);
             heap.complete_block_write(mblock);
@@ -414,8 +418,6 @@ void test_compact(bool csum, bool stable)
     int small_writes = 0;
     heap_entry_t *small_wr = NULL;
     // FIXME: Check more iterate_compaction cases, also check more compact_object cases
-    // At least:
-    // - BIG_STABLE[v=1 l=1] SMALL_U[v=2 l=2] SMALL_U[v=3 l=3] ROLLBACK[v=1 l=4] -> BIG_STABLE[l=4]
     auto compact_info = heap.iterate_compaction(obj, heap.get_fsynced_lsn(), false, [&](heap_entry_t *wr)
     {
         small_wr = wr;
@@ -423,14 +425,20 @@ void test_compact(bool csum, bool stable)
     });
     assert(compact_info.compact_lsn == (stable ? 2 : 4));
     assert(compact_info.compact_version == 3);
-    assert(compact_info.clean_lsn == 1);
+    assert(compact_info.clean_wr->lsn == 1);
-    assert(compact_info.clean_version == 1);
-    assert(compact_info.clean_loc == 0x20000);
     assert(small_writes == 1);
     assert(small_wr->lsn == 2);
 
-    res = heap.add_compact(obj, 4 /*max_lsn*/, &mblock, NULL);
+    bitmap_set(ref_int_bitmap, 8192, 4096, 4096);
-    assert(res == 0);
+    {
+        uint32_t csums[dsk.data_block_size/(dsk.csum_block_size ? dsk.csum_block_size : 4096)] = {};
+        csums[0] = crc32c(0, buffer_area.data(), 4096);
+        csums[2] = crc32c(0, buffer_area.data()+8192, 4096);
+        res = heap.add_compact(obj, compact_info.compact_version, compact_info.compact_lsn,
+            compact_info.clean_wr->big_location(&heap), compact_info.do_delete,
+            &mblock, ref_int_bitmap, ref_int_bitmap, (uint8_t*)csums);
+        assert(res == 0);
+    }
     assert(mblock == 0);
     heap.start_block_write(mblock);
     heap.complete_block_write(mblock);
@@ -444,7 +452,6 @@ void test_compact(bool csum, bool stable)
     assert(obj);
     assert(count_writes(heap, obj) == (stable ? 3 : 4));
     assert(obj->version == 3);
-    bitmap_set(ref_int_bitmap, 8192, 4096, 4096);
     assert(!memcmp(obj->get_int_bitmap(&heap), ref_int_bitmap, dsk.clean_entry_bitmap_size));
     if (csum)
     {
@@ -491,7 +498,7 @@ void test_modify_bitmap()
     new_csums[0] = crc32c(0, buffer_area.data(), 4096);
 
     uint32_t mblock = 999999;
-    int res = heap.add_punch_holes(obj, 999, 1, new_bmp, new_csums, &mblock);
+    int res = heap.punch_holes(heap.prev(obj), new_bmp, new_csums, &mblock);
     assert(res == 0);
     assert(mblock == 0);
     heap.start_block_write(mblock);
@@ -499,7 +506,7 @@ void test_modify_bitmap()
 
     obj = heap.read_entry(oid);
     assert(obj);
-    assert(count_writes(heap, obj) == 2); // duplicate big_write should be inserted after previous big_write, but before small_writes
+    assert(count_writes(heap, obj) == 2);
     assert(memcmp(heap.prev(obj)->get_int_bitmap(&heap), new_bmp, dsk.clean_entry_bitmap_size) == 0);
     assert(memcmp(heap.prev(obj)->get_checksums(&heap), new_csums, dsk.data_block_size/32768*4) == 0);
 
@@ -726,8 +733,11 @@ void test_full_overwrite(bool stable)
             assert(!heap.is_buffer_area_free(16384, 4096));
             assert(!heap.is_buffer_area_free(20480, 4096));
 
+            uint8_t bitmap[dsk.clean_entry_bitmap_size];
+            memset(bitmap, 0xFF, dsk.clean_entry_bitmap_size);
             uint32_t mblock = 999999;
-            res = heap.add_compact(obj, 999, &mblock, NULL);
+            res = heap.add_compact(obj, obj->version, obj->lsn, wr->big_location(&heap),
+                false, &mblock, bitmap, bitmap, NULL);
             assert(res == 0);
             assert(mblock == 0);
             heap.start_block_write(mblock);
@@ -921,6 +931,7 @@ void test_rollback()
         assert(!(wr->entry_type & BS_HEAP_STABLE));
         wr = heap.prev(wr);
         assert(wr->version == 2);
+        assert(wr->lsn == 2);
         assert(wr->entry_type == BS_HEAP_SMALL_WRITE|BS_HEAP_STABLE);
         assert(wr->small().location == 16384);
         assert(wr->small().len == 4096);
@@ -930,7 +941,10 @@ void test_rollback()
         assert(wr->big_location(&heap) == 0x20000);
 
         // compact without rollback (can we do it at all?)
-        res = heap.add_compact(obj, 4, &mblock, NULL);
+        uint8_t bitmap[dsk.clean_entry_bitmap_size];
+        memset(bitmap, 0xFF, dsk.clean_entry_bitmap_size);
+        res = heap.add_compact(obj, 2, 2, wr->big_location(&heap),
+            false, &mblock, bitmap, bitmap, NULL);
         assert(res == 0);
         assert(mblock == 0);
         heap.start_block_write(mblock);
@@ -978,7 +992,7 @@ void test_rollback()
         // But the data is still in place, removed only on compaction
         assert(heap.is_data_used(0x20000));
 
-        res = heap.add_compact(obj, obj->lsn, &mblock, NULL);
+        res = heap.add_compact(obj, 0, 2, 0, true, &mblock, NULL, NULL, NULL);
         assert(res == 0);
         assert(mblock == 0);
         heap.start_block_write(mblock);