From e5759fa8d016d3f8e2a1ba79edb0bf90ab24565b Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Fri, 10 Jun 2016 15:44:26 -0400 Subject: [PATCH] librbd: recursive lock possible when disabling journaling If pool-level mirroring is enabled, deleting the journal could cause a deadlock attempting to delete remote peer sync-point snapshots. Fixes: http://tracker.ceph.com/issues/16235 Signed-off-by: Jason Dillaman (cherry picked from commit fb255e6c3cd44c8d24c53e3cd70395a11a712574) --- src/librbd/internal.cc | 61 ++++++++++--------- src/test/librbd/test_librbd.cc | 16 ----- src/test/librbd/test_mirroring.cc | 97 +++++++++++++++++++++++++++++++ src/test/librbd/test_support.cc | 18 ++++++ src/test/librbd/test_support.h | 1 + 5 files changed, 149 insertions(+), 44 deletions(-) diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index a73fb72049227..46c515af90122 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -1646,15 +1646,6 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, return -EINVAL; } - RWLock::RLocker owner_locker(ictx->owner_lock); - r = ictx->aio_work_queue->block_writes(); - BOOST_SCOPE_EXIT_ALL( (ictx) ) { - ictx->aio_work_queue->unblock_writes(); - }; - if (r < 0) { - return r; - } - uint64_t disable_mask = (RBD_FEATURES_MUTABLE | RBD_FEATURES_DISABLE_ONLY); if ((enabled && (features & RBD_FEATURES_MUTABLE) != features) || @@ -1666,6 +1657,35 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, return -EINVAL; } + rbd_mirror_mode_t mirror_mode = RBD_MIRROR_MODE_DISABLED; + if ((features & RBD_FEATURE_JOURNALING) != 0) { + r = librbd::mirror_mode_get(ictx->md_ctx, &mirror_mode); + if (r < 0) { + lderr(cct) << "error in retrieving pool mirroring status: " + << cpp_strerror(r) << dendl; + return r; + } + + // mark mirroring as disabling and prune all sync snapshots + // before acquiring locks + if (mirror_mode == RBD_MIRROR_MODE_POOL && !enabled) { + r = mirror_image_disable_internal(ictx, false, false); + if (r < 0) { + lderr(cct) << "error disabling image mirroring: " + << cpp_strerror(r) << dendl; + } + } + } + + RWLock::RLocker owner_locker(ictx->owner_lock); + r = ictx->aio_work_queue->block_writes(); + BOOST_SCOPE_EXIT_ALL( (ictx) ) { + ictx->aio_work_queue->unblock_writes(); + }; + if (r < 0) { + return r; + } + // avoid accepting new requests from peers while we manipulate // the image features if (ictx->exclusive_lock != nullptr) { @@ -1749,14 +1769,6 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, return r; } - rbd_mirror_mode_t mirror_mode; - r = librbd::mirror_mode_get(ictx->md_ctx, &mirror_mode); - if (r < 0) { - lderr(cct) << "error in retrieving pool mirroring status: " - << cpp_strerror(r) << dendl; - return r; - } - enable_mirroring = (mirror_mode == RBD_MIRROR_MODE_POOL); } @@ -1793,14 +1805,6 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, disable_flags |= RBD_FLAG_FAST_DIFF_INVALID; } if ((features & RBD_FEATURE_JOURNALING) != 0) { - rbd_mirror_mode_t mirror_mode; - r = librbd::mirror_mode_get(ictx->md_ctx, &mirror_mode); - if (r < 0) { - lderr(cct) << "error in retrieving pool mirroring status: " - << cpp_strerror(r) << dendl; - return r; - } - if (mirror_mode == RBD_MIRROR_MODE_IMAGE) { cls::rbd::MirrorImage mirror_image; r = cls_client::mirror_image_get(&ictx->md_ctx, ictx->id, @@ -1816,10 +1820,11 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, return -EINVAL; } } else if (mirror_mode == RBD_MIRROR_MODE_POOL) { - r = mirror_image_disable_internal(ictx, false); + r = cls_client::mirror_image_remove(&ictx->md_ctx, ictx->id); if (r < 0) { - lderr(cct) << "error disabling image mirroring: " - << cpp_strerror(r) << dendl; + lderr(cct) << "failed to remove image from mirroring directory: " + << cpp_strerror(r) << dendl; + return r; } } diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index 52b5d45726734..901abc8f9dda1 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -81,22 +81,6 @@ static int get_features(bool *old_format, uint64_t *features) return 0; } -static int get_image_id(librbd::Image &image, std::string *image_id) -{ - librbd::image_info_t info; - int r = image.stat(info, sizeof(info)); - if (r < 0) { - return r; - } - - char prefix[RBD_MAX_BLOCK_NAME_SIZE + 1]; - strncpy(prefix, info.block_name_prefix, RBD_MAX_BLOCK_NAME_SIZE); - prefix[RBD_MAX_BLOCK_NAME_SIZE] = '\0'; - - *image_id = std::string(prefix + strlen(RBD_DATA_PREFIX)); - return 0; -} - static int create_image_full(rados_ioctx_t ioctx, const char *name, uint64_t size, int *order, int old_format, uint64_t features) diff --git a/src/test/librbd/test_mirroring.cc b/src/test/librbd/test_mirroring.cc index ece07b1e9d8c1..758fc456fdb34 100644 --- a/src/test/librbd/test_mirroring.cc +++ b/src/test/librbd/test_mirroring.cc @@ -22,6 +22,8 @@ #include "librbd/internal.h" #include "librbd/ObjectMap.h" #include "librbd/Operations.h" +#include "librbd/journal/Types.h" +#include "journal/Journaler.h" #include #include #include @@ -255,6 +257,30 @@ public: ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED)); } + void setup_mirror_peer(librados::IoCtx &io_ctx, librbd::Image &image) { + ASSERT_EQ(0, image.snap_create("sync-point-snap")); + + std::string image_id; + ASSERT_EQ(0, get_image_id(image, &image_id)); + + librbd::journal::MirrorPeerClientMeta peer_client_meta( + "remote-image-id", {{"sync-point-snap", boost::none}}, {}); + librbd::journal::ClientData client_data(peer_client_meta); + + journal::Journaler journaler(io_ctx, image_id, "peer-client", 5); + C_SaferCond init_ctx; + journaler.init(&init_ctx); + ASSERT_EQ(-ENOENT, init_ctx.wait()); + + bufferlist client_data_bl; + ::encode(client_data, client_data_bl); + ASSERT_EQ(0, journaler.register_client(client_data_bl)); + + C_SaferCond shut_down_ctx; + journaler.shut_down(&shut_down_ctx); + ASSERT_EQ(0, shut_down_ctx.wait()); + } + }; TEST_F(TestMirroring, EnableImageMirror_In_MirrorModeImage) { @@ -311,6 +337,77 @@ TEST_F(TestMirroring, DisableImageMirror_In_MirrorModeDisabled) { RBD_MIRROR_IMAGE_DISABLED); } +TEST_F(TestMirroring, DisableImageMirrorWithPeer) { + REQUIRE_FEATURE(RBD_FEATURE_JOURNALING); + + ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_IMAGE)); + + uint64_t features = RBD_FEATURE_EXCLUSIVE_LOCK | RBD_FEATURE_JOURNALING; + int order = 20; + ASSERT_EQ(0, m_rbd.create2(m_ioctx, image_name.c_str(), 4096, features, + &order)); + + librbd::Image image; + ASSERT_EQ(0, m_rbd.open(m_ioctx, image, image_name.c_str())); + ASSERT_EQ(0, image.mirror_image_enable()); + + setup_mirror_peer(m_ioctx, image); + + ASSERT_EQ(0, image.mirror_image_disable(false)); + + std::vector snaps; + ASSERT_EQ(0, image.snap_list(snaps)); + ASSERT_TRUE(snaps.empty()); + + librbd::mirror_image_info_t mirror_image; + ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, + sizeof(mirror_image))); + ASSERT_EQ(RBD_MIRROR_IMAGE_DISABLED, mirror_image.state); + + librbd::mirror_image_status_t status; + ASSERT_EQ(0, image.mirror_image_get_status(&status, sizeof(status))); + ASSERT_EQ(MIRROR_IMAGE_STATUS_STATE_UNKNOWN, status.state); + + ASSERT_EQ(0, image.close()); + ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str())); + ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED)); +} + +TEST_F(TestMirroring, DisableJournalingWithPeer) { + REQUIRE_FEATURE(RBD_FEATURE_JOURNALING); + + ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_POOL)); + + uint64_t features = RBD_FEATURE_EXCLUSIVE_LOCK | RBD_FEATURE_JOURNALING; + int order = 20; + ASSERT_EQ(0, m_rbd.create2(m_ioctx, image_name.c_str(), 4096, features, + &order)); + + librbd::Image image; + ASSERT_EQ(0, m_rbd.open(m_ioctx, image, image_name.c_str())); + + setup_mirror_peer(m_ioctx, image); + + ASSERT_EQ(0, image.update_features(RBD_FEATURE_JOURNALING, false)); + + std::vector snaps; + ASSERT_EQ(0, image.snap_list(snaps)); + ASSERT_TRUE(snaps.empty()); + + librbd::mirror_image_info_t mirror_image; + ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, + sizeof(mirror_image))); + ASSERT_EQ(RBD_MIRROR_IMAGE_DISABLED, mirror_image.state); + + librbd::mirror_image_status_t status; + ASSERT_EQ(0, image.mirror_image_get_status(&status, sizeof(status))); + ASSERT_EQ(MIRROR_IMAGE_STATUS_STATE_UNKNOWN, status.state); + + ASSERT_EQ(0, image.close()); + ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str())); + ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED)); +} + TEST_F(TestMirroring, EnableImageMirror_WithoutJournaling) { uint64_t features = 0; features |= RBD_FEATURE_OBJECT_MAP; diff --git a/src/test/librbd/test_support.cc b/src/test/librbd/test_support.cc index db573efa10f91..5b5adf3d2d387 100644 --- a/src/test/librbd/test_support.cc +++ b/src/test/librbd/test_support.cc @@ -1,6 +1,7 @@ // -*- mode:C; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- // vim: ts=8 sw=2 smarttab #include "test/librbd/test_support.h" +#include "include/rbd_types.h" #include bool get_features(uint64_t *features) { @@ -37,3 +38,20 @@ int create_image_pp(librbd::RBD &rbd, librados::IoCtx &ioctx, return rbd.create2(ioctx, name.c_str(), size, features, &order); } } + +int get_image_id(librbd::Image &image, std::string *image_id) +{ + librbd::image_info_t info; + int r = image.stat(info, sizeof(info)); + if (r < 0) { + return r; + } + + char prefix[RBD_MAX_BLOCK_NAME_SIZE + 1]; + strncpy(prefix, info.block_name_prefix, RBD_MAX_BLOCK_NAME_SIZE); + prefix[RBD_MAX_BLOCK_NAME_SIZE] = '\0'; + + *image_id = std::string(prefix + strlen(RBD_DATA_PREFIX)); + return 0; +} + diff --git a/src/test/librbd/test_support.h b/src/test/librbd/test_support.h index 63c5e3a345581..3a2298e820876 100644 --- a/src/test/librbd/test_support.h +++ b/src/test/librbd/test_support.h @@ -9,6 +9,7 @@ bool get_features(uint64_t *features); bool is_feature_enabled(uint64_t feature); int create_image_pp(librbd::RBD &rbd, librados::IoCtx &ioctx, const std::string &name, uint64_t size); +int get_image_id(librbd::Image &image, std::string *image_id); #define REQUIRE_FEATURE(feature) { \ if (!is_feature_enabled(feature)) { \ -- 2.39.5