librbd: properly handle object map open returning error code

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 570de56bfc7fd2edd5cf1cf5b7b92084cd3352a3)

diff --git a/src/librbd/exclusive_lock/AcquireRequest.cc b/src/librbd/exclusive_lock/AcquireRequest.cc
index 4008c827a1d2623c8fc3a9de8b64c778a0615192..4a4ee6d051124d73609aebd75da344c845f99b3a 100644 (file)
--- a/src/librbd/exclusive_lock/AcquireRequest.cc
+++ b/src/librbd/exclusive_lock/AcquireRequest.cc
@@ -250,8 +250,15 @@ Context *AcquireRequest<I>::handle_open_object_map(int *ret_val) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << __func__ << ": r=" << *ret_val << dendl;
 
-  // object map should never result in an error
-  assert(*ret_val == 0);
+  if (*ret_val < 0) {
+    lderr(cct) << "failed to open object map: " << cpp_strerror(*ret_val)
+               << dendl;
+
+    *ret_val = 0;
+    delete m_object_map;
+    m_object_map = nullptr;
+  }
+
   return send_open_journal();
 }
 

diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc
index b499b0c86953cbbd9353770554a699439b90c4cc..dedf3073832a62553986046bdb3aa6ac19fbd368 100644 (file)
--- a/src/librbd/image/RefreshRequest.cc
+++ b/src/librbd/image/RefreshRequest.cc
@@ -607,7 +607,13 @@ Context *RefreshRequest<I>::handle_v2_open_object_map(int *result) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << ": r=" << *result << dendl;
 
-  assert(*result == 0);
+  if (*result < 0) {
+    lderr(cct) << "failed to open object map: " << cpp_strerror(*result)
+               << dendl;
+    delete m_object_map;
+    m_object_map = nullptr;
+  }
+
   send_v2_open_journal();
   return nullptr;
 }

diff --git a/src/librbd/image/SetSnapRequest.cc b/src/librbd/image/SetSnapRequest.cc
index f30f4a97fe75a874e50d4458057896a456252269..44da6730bd7ba503da8bfa1549a4988e63723d88 100644 (file)
--- a/src/librbd/image/SetSnapRequest.cc
+++ b/src/librbd/image/SetSnapRequest.cc
@@ -272,8 +272,12 @@ Context *SetSnapRequest<I>::handle_open_object_map(int *result) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << __func__ << ": r=" << *result << dendl;
 
-  // object map should never report errors
-  assert(*result == 0);
+  if (*result < 0) {
+    lderr(cct) << "failed to open object map: " << cpp_strerror(*result)
+               << dendl;
+    delete m_object_map;
+    m_object_map = nullptr;
+  }
 
   *result = apply();
   if (*result < 0) {

diff --git a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
index b5eef00b650f13c72e817fbebbe385474899c6f4..e4ba0ec81e71bb9f35ae4a0d595b7e98d38b0d06 100644 (file)
--- a/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
+++ b/src/test/librbd/exclusive_lock/test_mock_AcquireRequest.cc
@@ -65,9 +65,9 @@ public:
   }
 
   void expect_open_object_map(MockImageCtx &mock_image_ctx,
-                              MockObjectMap &mock_object_map) {
+                              MockObjectMap &mock_object_map, int r) {
     EXPECT_CALL(mock_object_map, open(_))
-                  .WillOnce(CompleteContext(0, mock_image_ctx.image_ctx->op_work_queue));
+                  .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue));
   }
 
   void expect_close_object_map(MockImageCtx &mock_image_ctx,
@@ -192,7 +192,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, Success) {
   MockObjectMap mock_object_map;
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
   expect_create_object_map(mock_image_ctx, &mock_object_map);
-  expect_open_object_map(mock_image_ctx, mock_object_map);
+  expect_open_object_map(mock_image_ctx, mock_object_map, 0);
 
   MockJournal mock_journal;
   MockJournalPolicy mock_journal_policy;
@@ -228,7 +228,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, SuccessJournalDisabled) {
   MockObjectMap mock_object_map;
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
   expect_create_object_map(mock_image_ctx, &mock_object_map);
-  expect_open_object_map(mock_image_ctx, mock_object_map);
+  expect_open_object_map(mock_image_ctx, mock_object_map, 0);
 
   expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, false);
 
@@ -291,7 +291,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, JournalError) {
   MockObjectMap *mock_object_map = new MockObjectMap();
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
   expect_create_object_map(mock_image_ctx, mock_object_map);
-  expect_open_object_map(mock_image_ctx, *mock_object_map);
+  expect_open_object_map(mock_image_ctx, *mock_object_map, 0);
 
   MockJournal *mock_journal = new MockJournal();
   expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, true);
@@ -326,7 +326,7 @@ TEST_F(TestMockExclusiveLockAcquireRequest, AllocateJournalTagError) {
   MockObjectMap *mock_object_map = new MockObjectMap();
   expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
   expect_create_object_map(mock_image_ctx, mock_object_map);
-  expect_open_object_map(mock_image_ctx, *mock_object_map);
+  expect_open_object_map(mock_image_ctx, *mock_object_map, 0);
 
   MockJournal *mock_journal = new MockJournal();
   MockJournalPolicy mock_journal_policy;
@@ -653,5 +653,42 @@ TEST_F(TestMockExclusiveLockAcquireRequest, BreakLockError) {
   ASSERT_EQ(-EINVAL, ctx.wait());
 }
 
+TEST_F(TestMockExclusiveLockAcquireRequest, OpenObjectMapError) {
+  REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockImageCtx mock_image_ctx(*ictx);
+  expect_op_work_queue(mock_image_ctx);
+
+  InSequence seq;
+  expect_flush_notifies(mock_image_ctx);
+  expect_lock(mock_image_ctx, 0);
+
+  MockObjectMap *mock_object_map = new MockObjectMap();
+  expect_test_features(mock_image_ctx, RBD_FEATURE_OBJECT_MAP, true);
+  expect_create_object_map(mock_image_ctx, mock_object_map);
+  expect_open_object_map(mock_image_ctx, *mock_object_map, -EFBIG);
+
+  MockJournal mock_journal;
+  MockJournalPolicy mock_journal_policy;
+  expect_test_features(mock_image_ctx, RBD_FEATURE_JOURNALING, true);
+  expect_create_journal(mock_image_ctx, &mock_journal);
+  expect_open_journal(mock_image_ctx, mock_journal, 0);
+  expect_get_journal_policy(mock_image_ctx, mock_journal_policy);
+  expect_allocate_journal_tag(mock_image_ctx, mock_journal_policy, 0);
+
+  C_SaferCond acquire_ctx;
+  C_SaferCond ctx;
+  MockAcquireRequest *req = MockAcquireRequest::create(mock_image_ctx,
+                                                       TEST_COOKIE,
+                                                       &acquire_ctx, &ctx);
+  req->send();
+  ASSERT_EQ(0, acquire_ctx.wait());
+  ASSERT_EQ(0, ctx.wait());
+  ASSERT_EQ(nullptr, mock_image_ctx.object_map);
+}
+
 } // namespace exclusive_lock
 } // namespace librbd

diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc
index 6af5f46f46c1cec795ee9de2235c02f148c0457e..01a7381064628d7b8d3e45bf450c52896be4406e 100644 (file)
--- a/src/test/librbd/image/test_mock_RefreshRequest.cc
+++ b/src/test/librbd/image/test_mock_RefreshRequest.cc
@@ -261,10 +261,10 @@ public:
   }
 
   void expect_open_object_map(MockRefreshImageCtx &mock_image_ctx,
-                              MockObjectMap &mock_object_map, int r) {
+                              MockObjectMap *mock_object_map, int r) {
     EXPECT_CALL(mock_image_ctx, create_object_map(_))
-                  .WillOnce(Return(&mock_object_map));
-    EXPECT_CALL(mock_object_map, open(_))
+                  .WillOnce(Return(mock_object_map));
+    EXPECT_CALL(*mock_object_map, open(_))
                   .WillOnce(CompleteContext(r, mock_image_ctx.image_ctx->op_work_queue));
   }
 
@@ -419,7 +419,7 @@ TEST_F(TestMockImageRefreshRequest, SuccessSetSnapshotV2) {
   expect_get_snapshots(mock_image_ctx, 0);
   expect_refresh_parent_is_required(mock_refresh_parent_request, false);
   if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
-    expect_open_object_map(mock_image_ctx, mock_object_map, 0);
+    expect_open_object_map(mock_image_ctx, &mock_object_map, 0);
   }
   expect_add_snap(mock_image_ctx, "snap", ictx->snap_ids.begin()->second);
   expect_get_snap_id(mock_image_ctx, "snap", ictx->snap_ids.begin()->second);
@@ -664,7 +664,7 @@ TEST_F(TestMockImageRefreshRequest, EnableObjectMapWithExclusiveLock) {
   expect_get_mutable_metadata(mock_image_ctx, 0);
   expect_get_flags(mock_image_ctx, 0);
   expect_refresh_parent_is_required(mock_refresh_parent_request, false);
-  expect_open_object_map(mock_image_ctx, mock_object_map, 0);
+  expect_open_object_map(mock_image_ctx, &mock_object_map, 0);
 
   C_SaferCond ctx;
   MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
@@ -745,5 +745,40 @@ TEST_F(TestMockImageRefreshRequest, DisableObjectMap) {
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockImageRefreshRequest, OpenObjectMapError) {
+  REQUIRE_FEATURE(RBD_FEATURE_OBJECT_MAP);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  ASSERT_EQ(0, update_features(ictx, RBD_FEATURE_JOURNALING, false));
+
+  MockRefreshImageCtx mock_image_ctx(*ictx);
+  MockRefreshParentRequest mock_refresh_parent_request;
+
+  MockExclusiveLock mock_exclusive_lock;
+  mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
+
+  MockObjectMap *mock_object_map = new MockObjectMap();
+
+  expect_op_work_queue(mock_image_ctx);
+  expect_test_features(mock_image_ctx);
+  expect_is_exclusive_lock_owner(mock_exclusive_lock, true);
+
+  // object map should be immediately opened if exclusive lock owned
+  InSequence seq;
+  expect_get_mutable_metadata(mock_image_ctx, 0);
+  expect_get_flags(mock_image_ctx, 0);
+  expect_refresh_parent_is_required(mock_refresh_parent_request, false);
+  expect_open_object_map(mock_image_ctx, mock_object_map, -EFBIG);
+
+  C_SaferCond ctx;
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, &ctx);
+  req->send();
+
+  ASSERT_EQ(0, ctx.wait());
+  ASSERT_EQ(nullptr, mock_image_ctx.object_map);
+}
+
 } // namespace image
 } // namespace librbd