From cdd6cbfdfe5a77008ba298667bb7add8c236027a Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 9 Dec 2016 09:39:39 -0500 Subject: [PATCH] librbd: block concurrent in-flight object map updates for the same object Signed-off-by: Jason Dillaman (cherry picked from commit 7d743bfce61c6ede0a34fc0982e52be1d367d772) --- src/librbd/ObjectMap.cc | 77 ++++++- src/librbd/ObjectMap.h | 42 +++- src/test/librbd/CMakeLists.txt | 1 + src/test/librbd/test_mock_ObjectMap.cc | 272 +++++++++++++++++++++++++ 4 files changed, 381 insertions(+), 11 deletions(-) create mode 100644 src/test/librbd/test_mock_ObjectMap.cc diff --git a/src/librbd/ObjectMap.cc b/src/librbd/ObjectMap.cc index 9d57ea8b5a124..c59366eceb0b4 100644 --- a/src/librbd/ObjectMap.cc +++ b/src/librbd/ObjectMap.cc @@ -2,6 +2,7 @@ // vim: ts=8 sw=2 smarttab #include "librbd/ObjectMap.h" +#include "librbd/BlockGuard.h" #include "librbd/ExclusiveLock.h" #include "librbd/ImageCtx.h" #include "librbd/object_map/RefreshRequest.h" @@ -26,14 +27,20 @@ #define dout_subsys ceph_subsys_rbd #undef dout_prefix -#define dout_prefix *_dout << "librbd::ObjectMap: " +#define dout_prefix *_dout << "librbd::ObjectMap: " << this << " " << __func__ \ + << ": " namespace librbd { template ObjectMap::ObjectMap(I &image_ctx, uint64_t snap_id) - : m_image_ctx(image_ctx), m_snap_id(snap_id) -{ + : m_image_ctx(image_ctx), m_snap_id(snap_id), + m_update_guard(new UpdateGuard(m_image_ctx.cct)) { +} + +template +ObjectMap::~ObjectMap() { + delete m_update_guard; } template @@ -92,8 +99,7 @@ bool ObjectMap::object_may_exist(uint64_t object_no) const RWLock::RLocker l(m_image_ctx.object_map_lock); uint8_t state = (*this)[object_no]; bool exists = (state != OBJECT_NONEXISTENT); - ldout(m_image_ctx.cct, 20) << &m_image_ctx << " object_may_exist: " - << "object_no=" << object_no << " r=" << exists + ldout(m_image_ctx.cct, 20) << "object_no=" << object_no << " r=" << exists << dendl; return exists; } @@ -202,6 +208,63 @@ void ObjectMap::aio_resize(uint64_t new_size, uint8_t default_object_state, req->send(); } +template +void ObjectMap::detained_aio_update(UpdateOperation &&op) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 20) << dendl; + + assert(m_image_ctx.snap_lock.is_locked()); + assert(m_image_ctx.object_map_lock.is_wlocked()); + + BlockGuardCell *cell; + int r = m_update_guard->detain({op.start_object_no, op.end_object_no}, + &op, &cell); + if (r < 0) { + lderr(cct) << "failed to detain object map update: " << cpp_strerror(r) + << dendl; + m_image_ctx.op_work_queue->queue(op.on_finish, r); + return; + } else if (r > 0) { + ldout(cct, 20) << "detaining object map update due to in-flight update: " + << "start=" << op.start_object_no << ", " + << "end=" << op.end_object_no << ", " + << (op.current_state ? + stringify(static_cast(*op.current_state)) : + "") + << "->" << static_cast(op.new_state) << dendl; + return; + } + + ldout(cct, 20) << "in-flight update cell: " << cell << dendl; + Context *on_finish = op.on_finish; + Context *ctx = new FunctionContext([this, cell, on_finish](int r) { + handle_detained_aio_update(cell, r, on_finish); + }); + aio_update(CEPH_NOSNAP, op.start_object_no, op.end_object_no, op.new_state, + op.current_state, ctx); +} + +template +void ObjectMap::handle_detained_aio_update(BlockGuardCell *cell, int r, + Context *on_finish) { + CephContext *cct = m_image_ctx.cct; + ldout(cct, 20) << "cell=" << cell << ", r=" << r << dendl; + + typename UpdateGuard::BlockOperations block_ops; + m_update_guard->release(cell, &block_ops); + + { + RWLock::RLocker owner_locker(m_image_ctx.owner_lock); + RWLock::RLocker snap_locker(m_image_ctx.snap_lock); + RWLock::WLocker object_map_locker(m_image_ctx.object_map_lock); + for (auto &op : block_ops) { + detained_aio_update(std::move(op)); + } + } + + on_finish->complete(r); +} + template void ObjectMap::aio_update(uint64_t snap_id, uint64_t start_object_no, uint64_t end_object_no, uint8_t new_state, @@ -217,8 +280,8 @@ void ObjectMap::aio_update(uint64_t snap_id, uint64_t start_object_no, assert(start_object_no < end_object_no); CephContext *cct = m_image_ctx.cct; - ldout(cct, 20) << &m_image_ctx << " aio_update: start=" << start_object_no - << ", end=" << end_object_no << ", " + ldout(cct, 20) << "start=" << start_object_no << ", " + << "end=" << end_object_no << ", " << (current_state ? stringify(static_cast(*current_state)) : "") << "->" << static_cast(new_state) << dendl; diff --git a/src/librbd/ObjectMap.h b/src/librbd/ObjectMap.h index 3ac857322f17e..a3d5ea74e5396 100644 --- a/src/librbd/ObjectMap.h +++ b/src/librbd/ObjectMap.h @@ -19,6 +19,8 @@ namespace librados { namespace librbd { +template class BlockGuard; +struct BlockGuardCell; class ImageCtx; template @@ -29,6 +31,7 @@ public: } ObjectMap(ImageCtxT &image_ctx, uint64_t snap_id); + ~ObjectMap(); static int remove(librados::IoCtx &io_ctx, const std::string &image_id); static std::string object_map_name(const std::string &image_id, @@ -77,11 +80,17 @@ public: if (object_no == end_object_no) { return false; } - } - aio_update(snap_id, start_object_no, end_object_no, new_state, - current_state, - util::create_context_callback(callback_object)); + UpdateOperation update_operation(start_object_no, end_object_no, + new_state, current_state, + util::create_context_callback( + callback_object)); + detained_aio_update(std::move(update_operation)); + } else { + aio_update(snap_id, start_object_no, end_object_no, new_state, + current_state, + util::create_context_callback(callback_object)); + } return true; } @@ -90,10 +99,35 @@ public: void snapshot_remove(uint64_t snap_id, Context *on_finish); private: + struct UpdateOperation { + uint64_t start_object_no; + uint64_t end_object_no; + uint8_t new_state; + boost::optional current_state; + Context *on_finish; + + UpdateOperation(uint64_t start_object_no, uint64_t end_object_no, + uint8_t new_state, + const boost::optional ¤t_state, + Context *on_finish) + : start_object_no(start_object_no), end_object_no(end_object_no), + new_state(new_state), current_state(current_state), + on_finish(on_finish) { + } + }; + + typedef BlockGuard UpdateGuard; + ImageCtxT &m_image_ctx; ceph::BitVector<2> m_object_map; uint64_t m_snap_id; + UpdateGuard *m_update_guard = nullptr; + + void detained_aio_update(UpdateOperation &&update_operation); + void handle_detained_aio_update(BlockGuardCell *cell, int r, + Context *on_finish); + void aio_update(uint64_t snap_id, uint64_t start_object_no, uint64_t end_object_no, uint8_t new_state, const boost::optional ¤t_state, diff --git a/src/test/librbd/CMakeLists.txt b/src/test/librbd/CMakeLists.txt index b2781979393fd..06d091092b888 100644 --- a/src/test/librbd/CMakeLists.txt +++ b/src/test/librbd/CMakeLists.txt @@ -21,6 +21,7 @@ set(unittest_librbd_srcs test_mock_AioImageRequest.cc test_mock_ExclusiveLock.cc test_mock_Journal.cc + test_mock_ObjectMap.cc test_mock_ObjectWatcher.cc exclusive_lock/test_mock_AcquireRequest.cc exclusive_lock/test_mock_ReleaseRequest.cc diff --git a/src/test/librbd/test_mock_ObjectMap.cc b/src/test/librbd/test_mock_ObjectMap.cc new file mode 100644 index 0000000000000..f14c6b9a5b274 --- /dev/null +++ b/src/test/librbd/test_mock_ObjectMap.cc @@ -0,0 +1,272 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab + +#include "test/librbd/test_mock_fixture.h" +#include "test/librbd/test_support.h" +#include "test/librbd/mock/MockImageCtx.h" +#include "librbd/ObjectMap.h" +#include "librbd/object_map/RefreshRequest.h" +#include "librbd/object_map/UnlockRequest.h" +#include "librbd/object_map/UpdateRequest.h" + +namespace librbd { + +namespace { + +struct MockTestImageCtx : public MockImageCtx { + MockTestImageCtx(ImageCtx &image_ctx) : MockImageCtx(image_ctx) { + } +}; + +} // anonymous namespace + +namespace object_map { + +template <> +struct RefreshRequest { + Context *on_finish = nullptr; + ceph::BitVector<2u> *object_map = nullptr; + static RefreshRequest *s_instance; + static RefreshRequest *create(MockTestImageCtx &image_ctx, + ceph::BitVector<2u> *object_map, + uint64_t snap_id, Context *on_finish) { + assert(s_instance != nullptr); + s_instance->on_finish = on_finish; + s_instance->object_map = object_map; + return s_instance; + } + + MOCK_METHOD0(send, void()); + + RefreshRequest() { + s_instance = this; + } +}; + +template <> +struct UnlockRequest { + Context *on_finish = nullptr; + static UnlockRequest *s_instance; + static UnlockRequest *create(MockTestImageCtx &image_ctx, + Context *on_finish) { + assert(s_instance != nullptr); + s_instance->on_finish = on_finish; + return s_instance; + } + + MOCK_METHOD0(send, void()); + UnlockRequest() { + s_instance = this; + } +}; + +template <> +struct UpdateRequest { + Context *on_finish = nullptr; + static UpdateRequest *s_instance; + static UpdateRequest *create(MockTestImageCtx &image_ctx, + ceph::BitVector<2u> *object_map, + uint64_t snap_id, + uint64_t start_object_no, uint64_t end_object_no, + uint8_t new_state, + const boost::optional ¤t_state, + Context *on_finish) { + assert(s_instance != nullptr); + s_instance->on_finish = on_finish; + s_instance->construct(snap_id, start_object_no, end_object_no, new_state, + current_state); + return s_instance; + } + + MOCK_METHOD5(construct, void(uint64_t snap_id, uint64_t start_object_no, + uint64_t end_object_no, uint8_t new_state, + const boost::optional ¤t_state)); + MOCK_METHOD0(send, void()); + UpdateRequest() { + s_instance = this; + } +}; + +RefreshRequest *RefreshRequest::s_instance = nullptr; +UnlockRequest *UnlockRequest::s_instance = nullptr; +UpdateRequest *UpdateRequest::s_instance = nullptr; + +} // namespace object_map +} // namespace librbd + +#include "librbd/ObjectMap.cc" + +namespace librbd { + +using testing::_; +using testing::InSequence; +using testing::Invoke; + +class TestMockObjectMap : public TestMockFixture { +public: + typedef ObjectMap MockObjectMap; + typedef object_map::RefreshRequest MockRefreshRequest; + typedef object_map::UnlockRequest MockUnlockRequest; + typedef object_map::UpdateRequest MockUpdateRequest; + + void expect_refresh(MockTestImageCtx &mock_image_ctx, + MockRefreshRequest &mock_refresh_request, + const ceph::BitVector<2u> &object_map, int r) { + EXPECT_CALL(mock_refresh_request, send()) + .WillOnce(Invoke([&mock_image_ctx, &mock_refresh_request, &object_map, r]() { + *mock_refresh_request.object_map = object_map; + mock_image_ctx.image_ctx->op_work_queue->queue(mock_refresh_request.on_finish, r); + })); + } + + void expect_unlock(MockTestImageCtx &mock_image_ctx, + MockUnlockRequest &mock_unlock_request, int r) { + EXPECT_CALL(mock_unlock_request, send()) + .WillOnce(Invoke([&mock_image_ctx, &mock_unlock_request, r]() { + mock_image_ctx.image_ctx->op_work_queue->queue(mock_unlock_request.on_finish, r); + })); + } + + void expect_update(MockTestImageCtx &mock_image_ctx, + MockUpdateRequest &mock_update_request, + uint64_t snap_id, uint64_t start_object_no, + uint64_t end_object_no, uint8_t new_state, + const boost::optional ¤t_state, + Context **on_finish) { + EXPECT_CALL(mock_update_request, construct(snap_id, start_object_no, + end_object_no, new_state, + current_state)) + .Times(1); + EXPECT_CALL(mock_update_request, send()) + .WillOnce(Invoke([&mock_image_ctx, &mock_update_request, on_finish]() { + *on_finish = mock_update_request.on_finish; + })); + } + +}; + +TEST_F(TestMockObjectMap, NonDetainedUpdate) { + REQUIRE_FEATURE(RBD_FEATURE_OBJECT_MAP); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + + InSequence seq; + ceph::BitVector<2u> object_map; + object_map.resize(4); + MockRefreshRequest mock_refresh_request; + expect_refresh(mock_image_ctx, mock_refresh_request, object_map, 0); + + MockUpdateRequest mock_update_request; + Context *finish_update_1; + expect_update(mock_image_ctx, mock_update_request, CEPH_NOSNAP, + 0, 1, 1, {}, &finish_update_1); + Context *finish_update_2; + expect_update(mock_image_ctx, mock_update_request, CEPH_NOSNAP, + 1, 2, 1, {}, &finish_update_2); + + MockUnlockRequest mock_unlock_request; + expect_unlock(mock_image_ctx, mock_unlock_request, 0); + + MockObjectMap mock_object_map(mock_image_ctx, CEPH_NOSNAP); + C_SaferCond open_ctx; + mock_object_map.open(&open_ctx); + ASSERT_EQ(0, open_ctx.wait()); + + C_SaferCond update_ctx1; + C_SaferCond update_ctx2; + { + RWLock::RLocker snap_locker(mock_image_ctx.snap_lock); + RWLock::WLocker object_map_locker(mock_image_ctx.object_map_lock); + mock_object_map.aio_update(CEPH_NOSNAP, 0, 1, {}, &update_ctx1); + mock_object_map.aio_update(CEPH_NOSNAP, 1, 1, {}, &update_ctx2); + } + + finish_update_2->complete(0); + ASSERT_EQ(0, update_ctx2.wait()); + + finish_update_1->complete(0); + ASSERT_EQ(0, update_ctx1.wait()); + + C_SaferCond close_ctx; + mock_object_map.close(&close_ctx); + ASSERT_EQ(0, close_ctx.wait()); +} + +TEST_F(TestMockObjectMap, DetainedUpdate) { + REQUIRE_FEATURE(RBD_FEATURE_OBJECT_MAP); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + MockTestImageCtx mock_image_ctx(*ictx); + + InSequence seq; + ceph::BitVector<2u> object_map; + object_map.resize(4); + MockRefreshRequest mock_refresh_request; + expect_refresh(mock_image_ctx, mock_refresh_request, object_map, 0); + + MockUpdateRequest mock_update_request; + Context *finish_update_1; + expect_update(mock_image_ctx, mock_update_request, CEPH_NOSNAP, + 1, 4, 1, {}, &finish_update_1); + Context *finish_update_2 = nullptr; + expect_update(mock_image_ctx, mock_update_request, CEPH_NOSNAP, + 1, 3, 1, {}, &finish_update_2); + Context *finish_update_3 = nullptr; + expect_update(mock_image_ctx, mock_update_request, CEPH_NOSNAP, + 2, 3, 1, {}, &finish_update_3); + Context *finish_update_4 = nullptr; + expect_update(mock_image_ctx, mock_update_request, CEPH_NOSNAP, + 0, 2, 1, {}, &finish_update_4); + + MockUnlockRequest mock_unlock_request; + expect_unlock(mock_image_ctx, mock_unlock_request, 0); + + MockObjectMap mock_object_map(mock_image_ctx, CEPH_NOSNAP); + C_SaferCond open_ctx; + mock_object_map.open(&open_ctx); + ASSERT_EQ(0, open_ctx.wait()); + + C_SaferCond update_ctx1; + C_SaferCond update_ctx2; + C_SaferCond update_ctx3; + C_SaferCond update_ctx4; + { + RWLock::RLocker snap_locker(mock_image_ctx.snap_lock); + RWLock::WLocker object_map_locker(mock_image_ctx.object_map_lock); + mock_object_map.aio_update(CEPH_NOSNAP, 1, 4, 1, {}, &update_ctx1); + mock_object_map.aio_update(CEPH_NOSNAP, 1, 3, 1, {}, &update_ctx2); + mock_object_map.aio_update(CEPH_NOSNAP, 2, 3, 1, {}, &update_ctx3); + mock_object_map.aio_update(CEPH_NOSNAP, 0, 2, 1, {}, &update_ctx4); + } + + // updates 2, 3, and 4 are blocked on update 1 + ASSERT_EQ(nullptr, finish_update_2); + finish_update_1->complete(0); + ASSERT_EQ(0, update_ctx1.wait()); + + // updates 3 and 4 are blocked on update 2 + ASSERT_NE(nullptr, finish_update_2); + ASSERT_EQ(nullptr, finish_update_3); + ASSERT_EQ(nullptr, finish_update_4); + finish_update_2->complete(0); + ASSERT_EQ(0, update_ctx2.wait()); + + ASSERT_NE(nullptr, finish_update_3); + ASSERT_NE(nullptr, finish_update_4); + finish_update_3->complete(0); + finish_update_4->complete(0); + ASSERT_EQ(0, update_ctx3.wait()); + ASSERT_EQ(0, update_ctx4.wait()); + + C_SaferCond close_ctx; + mock_object_map.close(&close_ctx); + ASSERT_EQ(0, close_ctx.wait()); +} + +} // namespace librbd + -- 2.39.5