From 78a465b90b1c5187cfec9f25eb58b3bf617ca39b Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 7 Nov 2017 22:05:10 -0600 Subject: [PATCH] os/bluestore: fix SharedBlob unregistration We use the SharedBlobSet remove() in three cases: - from SharedBlob::put(), we try to remove ourselves from the set, but have to deal with a racing lookup, so the removal is conditional on nref still being 0. - from split_cache(), we move the SharedBlob to another collection - from make_blob_unshared(), we remove the entry when we clear the sbid. The problem is that the condtiional remove() (for the first case) was being used for all three cases, and in the second two cases nref is always != 0, so it doesn't actually happen. This can lead to a crash during cache shutdown. Fix by making two variants: remove() that is unconditional, and try_remove() that is conditional. Set the sb->coll pointer after because remove() asserts the parent matches where we are unregistering. Fixes: http://tracker.ceph.com/issues/22039 Signed-off-by: Sage Weil --- src/os/bluestore/BlueStore.cc | 4 ++-- src/os/bluestore/BlueStore.h | 8 +++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 7a86d8ea83434..62f5ccb138814 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -1670,7 +1670,7 @@ void BlueStore::SharedBlob::put() << " removing self from set " << get_parent() << dendl; if (get_parent()) { - if (get_parent()->remove(this)) { + if (get_parent()->try_remove(this)) { delete this; } else { ldout(coll->store->cct, 20) @@ -3323,13 +3323,13 @@ void BlueStore::Collection::split_cache( continue; } ldout(store->cct, 20) << __func__ << " moving " << *sb << dendl; - sb->coll = dest; if (sb->get_sbid()) { ldout(store->cct, 20) << __func__ << " moving registration " << *sb << dendl; shared_blob_set.remove(sb); dest->shared_blob_set.add(dest, sb); } + sb->coll = dest; if (dest->cache != cache) { for (auto& i : sb->bc.buffer_map) { if (!i.second->is_writing()) { diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 474e52ac26cff..298cd0ec94325 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -443,7 +443,7 @@ public: sb->coll = coll; } - bool remove(SharedBlob *sb) { + bool try_remove(SharedBlob *sb) { std::lock_guard l(lock); if (sb->nref == 0) { assert(sb->get_parent() == this); @@ -453,6 +453,12 @@ public: return false; } + void remove(SharedBlob *sb) { + std::lock_guard l(lock); + assert(sb->get_parent() == this); + sb_map.erase(sb->get_sbid()); + } + bool empty() { std::lock_guard l(lock); return sb_map.empty(); -- 2.39.5