diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 59673c1d0c2..de7fae2bf6a 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18347,10 +18347,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. @@ -18363,33 +18371,29 @@ int BlueStore::_do_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(); - if (b.is_shared() && - sb->loaded && - maybe_unshared_blobs.count(sb)) { - if (b.is_compressed()) { - expect[sb].get(0, b.get_ondisk_size()); - } 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); - return 0; - }); + 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)) { + for (const auto& e: b.get_extents()) { + if (e.is_valid()) { + expect[B].get(e.offset, e.length); + } } + maybe_unshared_blobs.erase(sb); } } // expect has now refs set exactly as .head is using it vector unshared_blobs; - unshared_blobs.reserve(maybe_unshared_blobs.size()); - for (auto& p : expect) { - dout(20) << " ? " << *p.first << " vs " << p.second << dendl; - if (p.first->persistent->ref_map == p.second) { + unshared_blobs.reserve(expect.size()); + 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); diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 7a10dcde001..b2309edc143 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, @@ -3960,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, diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc index 5ba86a13e05..f5ae63a3644 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);