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 <dillaman@redhat.com>
(cherry picked from commit fb255e6c3cd44c8d24c53e3cd70395a11a712574)

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index a73fb72049227087b80c63c18e0cec98186ffe71..46c515af90122e5c23d1a8a858778a035bb71fb7 100644 (file)
--- 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 52b5d457267348aba1ffeac328ba5b9dbd2e5fe0..901abc8f9dda179986fcb33f717228a6ec1c0860 100644 (file)
--- 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 ece07b1e9d8c1739374f28195767dfc35d1cdb14..758fc456fdb34b59cab205cf5643256df33d75bf 100644 (file)
--- 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 <boost/scope_exit.hpp>
 #include <boost/assign/list_of.hpp>
 #include <utility>
@@ -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<librbd::snap_info_t> 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<librbd::snap_info_t> 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 db573efa10f91e24bfaad11ce290440aee2dd04a..5b5adf3d2d387b7bc75ac62eb168bf2f5149c758 100644 (file)
--- 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 <sstream>
 
 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 63c5e3a3455817b4367ad2624833992235bc40c7..3a2298e820876fc0ae55a7ae22efcf902252fa05 100644 (file)
--- 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)) {    \