librbd: don't restart empty copyups in crypto layer

This commit fixes a bug where an empty parent copyup is restarted indefinitely.

Signed-off-by: Or Ozeri <oro@il.ibm.com>

diff --git a/src/librbd/crypto/CryptoObjectDispatch.cc b/src/librbd/crypto/CryptoObjectDispatch.cc
index 9cac7140761dab5d2ed6b4c4bca9afb257ebdd84..244f52dec2bd220eb1602ab2ffa0f75d6d6f1e0a 100644 (file)
--- a/src/librbd/crypto/CryptoObjectDispatch.cc
+++ b/src/librbd/crypto/CryptoObjectDispatch.cc
@@ -201,6 +201,7 @@ struct C_UnalignedObjectWriteRequest : public Context {
     int* object_dispatch_flags;
     uint64_t* journal_tid;
     Context* on_finish;
+    bool may_copyup;
     ceph::bufferlist aligned_data;
     io::ReadExtents extents;
     uint64_t version;
@@ -214,14 +215,15 @@ struct C_UnalignedObjectWriteRequest : public Context {
             IOContext io_context, int op_flags, int write_flags,
             std::optional<uint64_t> assert_version,
             const ZTracer::Trace &parent_trace, int* object_dispatch_flags,
-            uint64_t* journal_tid,Context* on_dispatched
+            uint64_t* journal_tid, Context* on_dispatched, bool may_copyup
             ) : image_ctx(image_ctx), crypto(crypto), object_no(object_no),
                 object_off(object_off), data(data), cmp_data(cmp_data),
                 mismatch_offset(mismatch_offset), io_context(io_context),
                 op_flags(op_flags), write_flags(write_flags),
                 assert_version(assert_version), parent_trace(parent_trace),
                 object_dispatch_flags(object_dispatch_flags),
-                journal_tid(journal_tid), on_finish(on_dispatched) {
+                journal_tid(journal_tid), on_finish(on_dispatched),
+                may_copyup(may_copyup) {
       // build read extents
       auto [pre_align, post_align] = crypto->get_pre_and_post_align(
               object_off, data.length());
@@ -337,7 +339,7 @@ struct C_UnalignedObjectWriteRequest : public Context {
       if (r < 0) {
         complete(r);
       } else {
-        restart_request();
+        restart_request(false);
       }
     }
 
@@ -345,13 +347,16 @@ struct C_UnalignedObjectWriteRequest : public Context {
       ldout(image_ctx->cct, 20) << "unaligned write r=" << r << dendl;
 
       if (r == -ENOENT) {
-        auto ctx = create_context_callback<
-                C_UnalignedObjectWriteRequest<I>,
-                &C_UnalignedObjectWriteRequest<I>::handle_copyup>(this);
-        if (io::util::trigger_copyup(image_ctx, object_no, io_context, ctx)) {
-          return;
+        if (may_copyup) {
+          auto ctx = create_context_callback<
+                  C_UnalignedObjectWriteRequest<I>,
+                  &C_UnalignedObjectWriteRequest<I>::handle_copyup>(this);
+          if (io::util::trigger_copyup(
+                  image_ctx, object_no, io_context, ctx)) {
+            return;
+          }
+          delete ctx;
         }
-        delete ctx;
         object_exists = false;
       } else if (r < 0) {
         complete(r);
@@ -388,13 +393,13 @@ struct C_UnalignedObjectWriteRequest : public Context {
       write_req->send();
     }
 
-    void restart_request() {
+    void restart_request(bool may_copyup) {
       auto req = new C_UnalignedObjectWriteRequest<I>(
               image_ctx, crypto, object_no, object_off,
               std::move(data), std::move(cmp_data),
               mismatch_offset, io_context, op_flags, write_flags,
               assert_version, parent_trace,
-              object_dispatch_flags, journal_tid, this);
+              object_dispatch_flags, journal_tid, this, may_copyup);
       req->send();
     }
 
@@ -409,7 +414,7 @@ struct C_UnalignedObjectWriteRequest : public Context {
       }
 
       if (restart) {
-        restart_request();
+        restart_request(may_copyup);
       } else {
         complete(r);
       }
@@ -491,7 +496,8 @@ bool CryptoObjectDispatch<I>::write(
     auto req = new C_UnalignedObjectWriteRequest<I>(
             m_image_ctx, m_crypto, object_no, object_off, std::move(data), {},
             nullptr, io_context, op_flags, write_flags, assert_version,
-            parent_trace, object_dispatch_flags, journal_tid, on_dispatched);
+            parent_trace, object_dispatch_flags, journal_tid, on_dispatched,
+            true);
     req->send();
   }
 
@@ -552,7 +558,7 @@ bool CryptoObjectDispatch<I>::compare_and_write(
           m_image_ctx, m_crypto, object_no, object_off, std::move(write_data),
           std::move(cmp_data), mismatch_offset, io_context, op_flags, 0,
           std::nullopt, parent_trace, object_dispatch_flags, journal_tid,
-          on_dispatched);
+          on_dispatched, true);
   req->send();
 
   return true;

diff --git a/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc b/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc
index 2f6972217fdf0f5de6b2060a0f07752b1bbbf701..f0160f1074a6eef2286b69ee78e5d30b7496200c 100644 (file)
--- a/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc
+++ b/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc
@@ -543,6 +543,50 @@ TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteCopyup) {
   ASSERT_EQ(0, dispatched_cond.wait());
 }
 
+TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteEmptyCopyup) {
+  MockObjectMap mock_object_map;
+  mock_image_ctx->object_map = &mock_object_map;
+  MockExclusiveLock mock_exclusive_lock;
+  mock_image_ctx->exclusive_lock = &mock_exclusive_lock;
+
+  ceph::bufferlist write_data;
+  write_data.append(std::string(8192, '1'));
+  io::ReadExtents extents = {{0, 4096}, {8192, 4096}};
+  expect_object_read(&extents);
+  ASSERT_TRUE(mock_crypto_object_dispatch->write(
+          0, 1, std::move(write_data), mock_image_ctx->get_data_io_context(),
+          0, 0, std::nullopt, {}, nullptr, nullptr, &dispatch_result,
+          &on_finish, on_dispatched));
+  ASSERT_EQ(dispatch_result, io::DISPATCH_RESULT_COMPLETE);
+  ASSERT_EQ(on_finish, &finished_cond);
+
+  expect_get_object_size();
+  expect_get_parent_overlap(mock_image_ctx->layout.object_size);
+  expect_remap_extents(0, mock_image_ctx->layout.object_size);
+  expect_prune_parent_extents(mock_image_ctx->layout.object_size);
+  EXPECT_CALL(mock_exclusive_lock, is_lock_owner()).WillRepeatedly(
+          Return(true));
+  EXPECT_CALL(*mock_image_ctx->object_map, object_may_exist(0)).WillOnce(
+          Return(false));
+  MockAbstractObjectWriteRequest *write_request = nullptr;
+  expect_copyup(&write_request, 0);
+
+  // unaligned write restarted
+  expect_object_read(&extents);
+  dispatcher_ctx->complete(-ENOENT); // complete first read
+  ASSERT_EQ(ETIMEDOUT, dispatched_cond.wait_for(0));
+
+  auto expected_data =
+        std::string(1, '\0') + std::string(8192, '1') +
+        std::string(4095, '\0');
+  expect_object_write(0, expected_data, io::OBJECT_WRITE_FLAG_CREATE_EXCLUSIVE,
+                      std::nullopt);
+  dispatcher_ctx->complete(-ENOENT); // complete second read
+  ASSERT_EQ(ETIMEDOUT, dispatched_cond.wait_for(0));
+  dispatcher_ctx->complete(0); // complete write
+  ASSERT_EQ(0, dispatched_cond.wait());
+}
+
 TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteFailVersionCheck) {
   ceph::bufferlist write_data;
   uint64_t version = 1234;