From 29bae567fc951d7a8d663eaa0cb7d9602b265c94 Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Wed, 3 Jun 2026 10:03:45 +0000 Subject: [PATCH 1/6] test/objectstore/store_test: Add more tests for unsharing blobs More test verifying if bluestore is properly revering shared blobs to regular blobs. Signed-off-by: Adam Kupczyk --- src/os/bluestore/BlueStore.h | 10 ++ src/test/objectstore/store_test.cc | 194 ++++++++++++++++++++++++++++- 2 files changed, 203 insertions(+), 1 deletion(-) diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index c4ef3c0e9e6..6beb08754a2 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -3646,6 +3646,16 @@ public: void debug_set_prefer_deferred_size(uint64_t s) { prefer_deferred_size = s; } + OnodeRef debug_get_onode(const coll_t& cid, const ghobject_t& hoid) { + std::shared_lock l(coll_lock); + auto cp = coll_map.find(cid); + if (cp == coll_map.end()) + return OnodeRef(); + auto& c = cp->second; + std::shared_lock ll(c->lock); + OnodeRef o = c->get_onode(hoid, false); + return o; + } inline void log_latency(const char* name, int idx, const ceph::timespan& lat, diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc index 655a9d25a0c..0ddd1ae50a0 100644 --- a/src/test/objectstore/store_test.cc +++ b/src/test/objectstore/store_test.cc @@ -365,6 +365,117 @@ public: umount(); } }; + +class UnsharingBlobsOnRemove : public CheckedUmount { +public: + struct MyCond : public C_SaferCond { + void reset() { + done = false; + } + }; + + std::string get_data_dir() { + return data_dir; + } + + void write_object( + ghobject_t hoid, + size_t pos, + size_t size) + { + ObjectStore::Transaction t; + bufferlist bl; + bl.append(std::string(size, 'x')); + t.write(cid, hoid, pos, bl.length(), bl); + int r = queue_transaction(store, ch, std::move(t)); + EXPECT_EQ(r, 0); + } + + size_t count_shared_blob_trackers() + { + BlueStore* bstore = dynamic_cast (store.get()); + auto* kv = bstore->get_kv(); + // to be inline with BlueStore.cc + const string PREFIX_SHARED_BLOB = "X"; + size_t cnt = 0; + auto it = kv->get_iterator(PREFIX_SHARED_BLOB); + ceph_assert(it); + for (it->lower_bound(string()); it->valid(); it->next()) { + ++cnt; + } + return cnt; + } + + void commit_transaction() + { + t.register_on_commit(&mycond); + int r = queue_transaction(store, ch, std::move(t)); + EXPECT_EQ(r, 0); + mycond.wait(); + mycond.reset(); + t = ObjectStore::Transaction(); + } + + BlueStore::OnodeRef get_onode(const coll_t& cid, const ghobject_t& hoid) { + BlueStore* bstore = dynamic_cast (store.get()); + return bstore->debug_get_onode(cid, hoid); + } + + size_t count_shared_blobs(BlueStore::OnodeRef o) + { + std::set visited; + size_t cnt = 0; + for (const auto& e : o->extent_map.extent_map) { + if (e.blob->get_blob().is_shared()) { + if (visited.emplace(e.blob.get()).second) { + cnt++; + } + } + } + return cnt; + } + + coll_t cid; + ObjectStore::CollectionHandle ch; + uint16_t poolid; + ObjectStore::Transaction t; + MyCond mycond; + void prepare_store() + { + static constexpr uint64_t _1G = uint64_t(1024)*1024*1024; + SetVal(g_conf(), "bluestore_block_size", stringify(10 * _1G).c_str()); + g_conf().apply_changes(nullptr); + DeferredSetup(); + poolid = 1234; + cid = coll_t(spg_t(pg_t(1, poolid), shard_id_t::NO_SHARD)); + + ch = store->create_new_collection(cid); + { + ObjectStore::Transaction t; + t.create_collection(cid, 0); + int r = queue_transaction(store, ch, std::move(t)); + ASSERT_EQ(r, 0); + } + + ch.reset(); + umount(); + } + + void cleanup_store() + { + mount(); + ch = store->open_collection(cid); + { + ObjectStore::Transaction t; + t.remove_collection(cid); + int r = queue_transaction(store, ch, std::move(t)); + ASSERT_EQ(r, 0); + } + ch.reset(); + umount(); + } +}; + #endif // WITH_BLUESTORE class StoreTestSpecificAUSize : public StoreTestDeferredSetup { @@ -7610,6 +7721,12 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Values("bluestore") ); +INSTANTIATE_TEST_SUITE_P( + BlueStore, + UnsharingBlobsOnRemove, + ::testing::Values("bluestore") +); + #endif // WITH_BLUESTORE struct deferred_test_t { @@ -11851,6 +11968,80 @@ TEST_P(CorruptedOnodesTest, Fsck_Fix3ExtraShards) { cleanup_store(); } +TEST_P(UnsharingBlobsOnRemove, FullCloneAndErase) { + SetVal(g_conf(), "bluestore_debug_inject_allocation_from_file_failure", "0"); + prepare_store(); + + mount(); + ch = store->create_new_collection(cid); + ghobject_t hoid_head(hobject_t( + sobject_t("LoremIpsum", CEPH_NOSNAP), "", 0x12345678, poolid, "")); + + write_object(hoid_head, 0x0, 0x10000); + + ghobject_t hoid_snap_1 = hoid_head; + hoid_snap_1.hobj.snap = 1; + + t.clone(cid, hoid_head, hoid_snap_1); + commit_transaction(); + + BlueStore::OnodeRef oo = get_onode(cid, hoid_head); + size_t shared_blob_count = count_shared_blobs(oo); + EXPECT_GE(shared_blob_count, 1); + size_t shared_blob_trackers = count_shared_blob_trackers(); + EXPECT_GE(shared_blob_trackers, 1); + + t.remove(cid, hoid_snap_1); + commit_transaction(); + + shared_blob_count = count_shared_blobs(oo); + EXPECT_EQ(shared_blob_count, 0); + shared_blob_trackers = count_shared_blob_trackers(); + EXPECT_EQ(shared_blob_trackers, 0); + oo.reset(); + + umount(); + cleanup_store(); +} + +TEST_P(UnsharingBlobsOnRemove, UnshareOnPartial) { + SetVal(g_conf(), "bluestore_debug_inject_allocation_from_file_failure", "0"); + SetVal(g_conf(), "bluestore_min_alloc_size", "16384"); + prepare_store(); + + mount(); + ch = store->create_new_collection(cid); + + ghobject_t hoid_head(hobject_t( + sobject_t("LoremIpsum", CEPH_NOSNAP), "", 0x12345678, poolid, "")); + + write_object(hoid_head, 0x0, 0xe000); + + ghobject_t hoid_snap_1 = hoid_head; + hoid_snap_1.hobj.snap = 1; + + t.clone(cid, hoid_head, hoid_snap_1); + commit_transaction(); + + BlueStore::OnodeRef oo = get_onode(cid, hoid_head); + size_t shared_blob_count = count_shared_blobs(oo); + EXPECT_GE(shared_blob_count, 1); + size_t shared_blob_trackers = count_shared_blob_trackers(); + EXPECT_GE(shared_blob_trackers, 1); + + t.remove(cid, hoid_snap_1); + commit_transaction(); + + shared_blob_count = count_shared_blobs(oo); + EXPECT_EQ(shared_blob_count, 0); + shared_blob_trackers = count_shared_blob_trackers(); + EXPECT_EQ(shared_blob_trackers, 0); + oo.reset(); + + umount(); + cleanup_store(); +} + #endif // WITH_BLUESTORE TEST_P(StoreTestSpecificAUSize, BluestoreEnforceHWSettingsHdd) { @@ -12608,8 +12799,9 @@ TEST_P(StoreTest, BlueFS_truncate_remove_race) { #endif // WITH_BLUESTORE int main(int argc, char **argv) { + std::map defaults = {{"debug_rocksdb","0/0"}}; auto args = argv_to_vec(argc, argv); - auto cct = global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, + auto cct = global_init(&defaults, args, CEPH_ENTITY_TYPE_CLIENT, CODE_ENVIRONMENT_UTILITY, CINIT_FLAG_NO_DEFAULT_CONFIG_FILE); common_init_finish(g_ceph_context); From 50a1037da8da6e438d30d8816ecfd04eaaf38be5 Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Mon, 1 Jun 2026 09:01:50 +0000 Subject: [PATCH 2/6] os/bluestore: Split BlueStore::_do_remove() Split BlueStore::_do_remove into 2 parts: - part that removes object (in _do_remove) - part that processes head object and tries to unshare blobs (new function _maybe_unshare_on_remove) No logic changes. Signed-off-by: Adam Kupczyk --- src/os/bluestore/BlueStore.cc | 12 ++++++++++-- src/os/bluestore/BlueStore.h | 4 ++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index ec470298ea4..4852c020f6b 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18325,10 +18325,18 @@ int BlueStore::_do_remove( nogen.hobj.snap = CEPH_NOSNAP; OnodeRef h = c->get_onode(nogen, false); - if (!h || !h->exists) { - return 0; + if (h && h->exists) { + return _maybe_unshare_on_remove(txc, c, h, std::move(maybe_unshared_blobs)); } + return 0; +} +int BlueStore::_maybe_unshare_on_remove( + TransContext *txc, + CollectionRef& c, + OnodeRef& h, + std::set&& maybe_unshared_blobs) +{ //Populate the extent map structure from DB; required for shared blob processing below. h->extent_map.fault_range(db, 0, h->onode.size); // Set maybe_unshared_blobs contains those shared blobs that have all nref=1. diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 6beb08754a2..4457dc86cda 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -3970,6 +3970,10 @@ private: int _do_remove(TransContext *txc, CollectionRef& c, OnodeRef& o); + int _maybe_unshare_on_remove(TransContext *txc, + CollectionRef& c, + OnodeRef& head_o, + std::set&& maybe_unshared_blobs); int _setattr(TransContext *txc, CollectionRef& c, OnodeRef& o, From e3d24c98271847eaf6f86438338352366bef773f Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Mon, 1 Jun 2026 10:05:12 +0000 Subject: [PATCH 3/6] os/bluestore: Fix unsharing compressed blobs Unsharing compressed blobs used object offsets, not disk offsets. Signed-off-by: Adam Kupczyk --- src/os/bluestore/BlueStore.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 4852c020f6b..18ea2d84972 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18357,7 +18357,12 @@ int BlueStore::_maybe_unshare_on_remove( sb->loaded && maybe_unshared_blobs.count(sb)) { if (b.is_compressed()) { - expect[sb].get(0, b.get_ondisk_size()); + b.map(0, b.get_ondisk_size(), [&](uint64_t off, uint64_t len) { + expect[sb].get(off, len); + return 0; + }); + // Do not account second time. + maybe_unshared_blobs.erase(sb); } else { // todo: it seems to be an overkill to go through map() b.map(e.blob_offset, e.length, [&](uint64_t off, uint64_t len) { @@ -18370,7 +18375,7 @@ int BlueStore::_maybe_unshare_on_remove( // expect has now refs set exactly as .head is using it vector unshared_blobs; - unshared_blobs.reserve(maybe_unshared_blobs.size()); + unshared_blobs.reserve(expect.size()); for (auto& p : expect) { dout(20) << " ? " << *p.first << " vs " << p.second << dendl; if (p.first->persistent->ref_map == p.second) { From 1cebcd999d5f97134cf954bc8b6b9682a700b13e Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Mon, 1 Jun 2026 10:50:18 +0000 Subject: [PATCH 4/6] os/bluestore: Modify maybe_unshare_on_remove Use Blob* as key for processing 'expect' shared blobs. Opens access to bluestore_blob_t. Signed-off-by: Adam Kupczyk --- src/os/bluestore/BlueStore.cc | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 18ea2d84972..df32b4dcbd9 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18349,16 +18349,17 @@ int BlueStore::_maybe_unshare_on_remove( // that is not yet loaded! We must have had inspected it to even check nrefs. dout(20) << __func__ << " checking for unshareable blobs on " << h << " " << h->oid << dendl; - map expect; + map expect; for (auto& e : h->extent_map.extent_map) { - const bluestore_blob_t& b = e.blob->get_blob(); - SharedBlob *sb = e.blob->get_shared_blob().get(); + const Blob* B = e.blob.get(); + const bluestore_blob_t& b = B->get_blob(); + SharedBlob *sb = B->get_shared_blob().get(); if (b.is_shared() && sb->loaded && maybe_unshared_blobs.count(sb)) { if (b.is_compressed()) { b.map(0, b.get_ondisk_size(), [&](uint64_t off, uint64_t len) { - expect[sb].get(off, len); + expect[B].get(off, len); return 0; }); // Do not account second time. @@ -18366,7 +18367,7 @@ int BlueStore::_maybe_unshare_on_remove( } else { // todo: it seems to be an overkill to go through map() b.map(e.blob_offset, e.length, [&](uint64_t off, uint64_t len) { - expect[sb].get(off, len); + expect[B].get(off, len); return 0; }); } @@ -18376,11 +18377,11 @@ int BlueStore::_maybe_unshare_on_remove( // expect has now refs set exactly as .head is using it vector unshared_blobs; unshared_blobs.reserve(expect.size()); - for (auto& p : expect) { - dout(20) << " ? " << *p.first << " vs " << p.second << dendl; - if (p.first->persistent->ref_map == p.second) { + for (const auto& [B, expect_refs] : expect) { + SharedBlob* sb = B->get_shared_blob().get(); + dout(20) << __func__ << " ? " << *sb << " vs " << expect_refs << dendl; + if (sb->persistent->ref_map == expect_refs) { // yup, .head is only one that is using the shared blob now - SharedBlob *sb = p.first; dout(20) << __func__ << " unsharing " << *sb << dendl; unshared_blobs.push_back(sb); txc->unshare_blob(sb); From 96a1a2e25dc382745812b5fff59ec76e85248958 Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Tue, 2 Jun 2026 05:59:39 +0000 Subject: [PATCH 5/6] os/bluestore: Make maybe_unshare_extents use allocations Blob is tracking whole AUs in SharedBlob tracker. But the extents may account for just a portion of AUs. We need to compare what would Blob release against SharedBlob tracker. Signed-off-by: Adam Kupczyk --- src/os/bluestore/BlueStore.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index df32b4dcbd9..66846ce8010 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18365,11 +18365,12 @@ int BlueStore::_maybe_unshare_on_remove( // Do not account second time. maybe_unshared_blobs.erase(sb); } else { - // todo: it seems to be an overkill to go through map() - b.map(e.blob_offset, e.length, [&](uint64_t off, uint64_t len) { - expect[B].get(off, len); - return 0; - }); + for (const auto& e: b.get_extents()) { + if (e.is_valid()) { + expect[B].get(e.offset, e.length); + } + } + maybe_unshared_blobs.erase(sb); } } } From 72fe925e93df9b5b26f489e52163e2903168b2d0 Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Tue, 2 Jun 2026 06:21:35 +0000 Subject: [PATCH 6/6] os/bluestore: Simplify _maybe_unshare_on_remove Unify compressed and uncompressed blobs handling. Signed-off-by: Adam Kupczyk --- src/os/bluestore/BlueStore.cc | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 66846ce8010..c640ff132c1 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18354,24 +18354,13 @@ int BlueStore::_maybe_unshare_on_remove( const Blob* B = e.blob.get(); const bluestore_blob_t& b = B->get_blob(); SharedBlob *sb = B->get_shared_blob().get(); - if (b.is_shared() && - sb->loaded && - maybe_unshared_blobs.count(sb)) { - if (b.is_compressed()) { - b.map(0, b.get_ondisk_size(), [&](uint64_t off, uint64_t len) { - expect[B].get(off, len); - return 0; - }); - // Do not account second time. - maybe_unshared_blobs.erase(sb); - } else { - for (const auto& e: b.get_extents()) { - if (e.is_valid()) { - expect[B].get(e.offset, e.length); - } + if (b.is_shared() && sb->loaded && maybe_unshared_blobs.count(sb)) { + for (const auto& e: b.get_extents()) { + if (e.is_valid()) { + expect[B].get(e.offset, e.length); } - maybe_unshared_blobs.erase(sb); } + maybe_unshared_blobs.erase(sb); } }