librbd: wait for copyup in unaligned crypto write

librbd copyup is not built to handle unaligned encrypted writes.
Therefore, a such write should kick-off a copyup and wait for it to complete before executing.
This commit implements this logic.

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

diff --git a/src/librbd/crypto/CryptoObjectDispatch.cc b/src/librbd/crypto/CryptoObjectDispatch.cc
index d17dcbcc6de7175a2ddc54060d69252335bc6dbe..b09aad720a6b5d039804da34161f3c1a0e8c0772 100644 (file)
--- a/src/librbd/crypto/CryptoObjectDispatch.cc
+++ b/src/librbd/crypto/CryptoObjectDispatch.cc
@@ -234,7 +234,8 @@ struct C_UnalignedObjectWriteRequest : public Context {
 
       read_req = new C_UnalignedObjectReadRequest<I>(
               image_ctx, crypto, object_no, &extents, io_context,
-              0, 0, parent_trace, &version, 0, ctx);
+              0, io::READ_FLAG_DISABLE_READ_FROM_PARENT, parent_trace,
+              &version, 0, ctx);
     }
 
     void send() {
@@ -324,10 +325,25 @@ struct C_UnalignedObjectWriteRequest : public Context {
       }
     }
 
+    void handle_copyup(int r) {
+      ldout(image_ctx->cct, 20) << "r=" << r << dendl;
+      if (r < 0) {
+        complete(r);
+      } else {
+        restart_request();
+      }
+    }
+
     void handle_read(int r) {
       ldout(image_ctx->cct, 20) << "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;
+        }
         object_exists = false;
       } else if (r < 0) {
         complete(r);
@@ -363,6 +379,16 @@ struct C_UnalignedObjectWriteRequest : public Context {
       write_req->send();
     }
 
+    void restart_request() {
+      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);
+      req->send();
+    }
+
     void handle_write(int r) {
       bool exclusive = write_flags & io::OBJECT_WRITE_FLAG_CREATE_EXCLUSIVE;
       bool restart = false;
@@ -373,13 +399,7 @@ struct C_UnalignedObjectWriteRequest : public Context {
       }
 
       if (restart) {
-        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);
-        req->send();
+        restart_request();
       } else {
         complete(r);
       }

diff --git a/src/librbd/io/Utils.cc b/src/librbd/io/Utils.cc
index 80e8032ef5f369a836c7e58a2ac3457b305a5b3b..99a911410aaed4528de3b8d7488e18250ff9b9b1 100644 (file)
--- a/src/librbd/io/Utils.cc
+++ b/src/librbd/io/Utils.cc
@@ -10,6 +10,7 @@
 #include "librbd/Utils.h"
 #include "librbd/io/AioCompletion.h"
 #include "librbd/io/ImageDispatchSpec.h"
+#include "librbd/io/ObjectRequest.h"
 #include "osd/osd_types.h"
 #include "osdc/Striper.h"
 
@@ -163,6 +164,22 @@ void unsparsify(CephContext* cct, ceph::bufferlist* bl,
   *bl = out_bl;
 }
 
+template <typename I>
+bool trigger_copyup(I* image_ctx, uint64_t object_no, IOContext io_context,
+                    Context* on_finish) {
+  bufferlist bl;
+  auto req = new ObjectWriteRequest<I>(
+          image_ctx, object_no, 0, std::move(bl), io_context, 0, 0,
+          std::nullopt, {}, on_finish);
+  if (!req->has_parent()) {
+    delete req;
+    return false;
+  }
+
+  req->send();
+  return true;
+}
+
 } // namespace util
 } // namespace io
 } // namespace librbd
@@ -172,3 +189,6 @@ template void librbd::io::util::read_parent(
     librados::snap_t snap_id, const ZTracer::Trace &trace, Context* on_finish);
 template int librbd::io::util::clip_request(
     librbd::ImageCtx *image_ctx, Extents *image_extents);
+template bool librbd::io::util::trigger_copyup(
+        librbd::ImageCtx *image_ctx, uint64_t object_no, IOContext io_context,
+        Context* on_finish);

diff --git a/src/librbd/io/Utils.h b/src/librbd/io/Utils.h
index d28786b2a3d79c139f2662b8a5e57d4574e09af2..22b86058a8bbe21d82aed17612db942443d95f0b 100644 (file)
--- a/src/librbd/io/Utils.h
+++ b/src/librbd/io/Utils.h
@@ -8,6 +8,7 @@
 #include "include/buffer_fwd.h"
 #include "include/rados/rados_types.hpp"
 #include "common/zipkin_trace.h"
+#include "librbd/Types.h"
 #include "librbd/io/Types.h"
 #include <map>
 
@@ -49,6 +50,10 @@ void unsparsify(CephContext* cct, ceph::bufferlist* bl,
                 const Extents& extent_map, uint64_t bl_off,
                 uint64_t out_bl_len);
 
+template <typename ImageCtxT = librbd::ImageCtx>
+bool trigger_copyup(ImageCtxT *image_ctx, uint64_t object_no,
+                    IOContext io_context, Context* on_finish);
+
 } // namespace util
 } // namespace io
 } // namespace librbd

diff --git a/src/librbd/operation/FlattenRequest.cc b/src/librbd/operation/FlattenRequest.cc
index 2ecdf4ec81f126d9b327282ae75a07a0c88bc712..76455221726a90e11f8db5bb52eaf7a4c73ee353 100644 (file)
--- a/src/librbd/operation/FlattenRequest.cc
+++ b/src/librbd/operation/FlattenRequest.cc
@@ -9,6 +9,7 @@
 #include "librbd/image/DetachParentRequest.h"
 #include "librbd/Types.h"
 #include "librbd/io/ObjectRequest.h"
+#include "librbd/io/Utils.h"
 #include "common/dout.h"
 #include "common/errno.h"
 #include <boost/lambda/bind.hpp>
@@ -54,18 +55,13 @@ public:
       }
     }
 
-    bufferlist bl;
-    auto req = new io::ObjectWriteRequest<I>(&image_ctx, m_object_no, 0,
-                                             std::move(bl), m_io_context, 0, 0,
-                                             std::nullopt, {}, this);
-    if (!req->has_parent()) {
+    if (!io::util::trigger_copyup(
+            &image_ctx, m_object_no, m_io_context, this)) {
       // stop early if the parent went away - it just means
       // another flatten finished first or the image was resized
-      delete req;
       return 1;
     }
 
-    req->send();
     return 0;
   }
 

diff --git a/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc b/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc
index 181cc2caf943548343c101e863d1f62adc13847e..805a83d75bda1ebd1494c44e8dace4014a871aea 100644 (file)
--- a/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc
+++ b/src/test/librbd/crypto/test_mock_CryptoObjectDispatch.cc
@@ -4,11 +4,21 @@
 #include "test/librbd/test_mock_fixture.h"
 #include "test/librbd/test_support.h"
 #include "test/librbd/mock/MockImageCtx.h"
+#include "test/librbd/mock/MockObjectMap.h"
 #include "test/librbd/mock/crypto/MockCryptoInterface.h"
 #include "librbd/crypto/CryptoObjectDispatch.h"
 #include "librbd/io/ObjectDispatchSpec.h"
 #include "librbd/io/Utils.h"
 
+#include "librbd/io/Utils.cc"
+template bool librbd::io::util::trigger_copyup(
+        MockImageCtx *image_ctx, uint64_t object_no, IOContext io_context,
+        Context* on_finish);
+
+#include "librbd/io/ObjectRequest.cc"
+template class librbd::io::ObjectWriteRequest<librbd::MockImageCtx>;
+template class librbd::io::AbstractObjectWriteRequest<librbd::MockImageCtx>;
+
 namespace librbd {
 
 namespace util {
@@ -20,6 +30,29 @@ inline ImageCtx *get_image_ctx(MockImageCtx *image_ctx) {
 } // namespace util
 
 namespace io {
+
+template <>
+struct CopyupRequest<librbd::MockImageCtx> {
+    MOCK_METHOD0(send, void());
+    MOCK_METHOD2(append_request, void(
+            AbstractObjectWriteRequest<librbd::MockImageCtx>*,
+            const Extents&));
+
+    static CopyupRequest* s_instance;
+    static CopyupRequest* create(librbd::MockImageCtx *ictx,
+                                 uint64_t objectno, Extents &&image_extents,
+                                 const ZTracer::Trace &parent_trace) {
+      return s_instance;
+    }
+
+    CopyupRequest() {
+      s_instance = this;
+    }
+};
+
+CopyupRequest<librbd::MockImageCtx>* CopyupRequest<
+        librbd::MockImageCtx>::s_instance = nullptr;
+
 namespace util {
 
 namespace {
@@ -63,10 +96,14 @@ using ::testing::_;
 using ::testing::ElementsAre;
 using ::testing::Invoke;
 using ::testing::Pair;
+using ::testing::Return;
 using ::testing::WithArg;
 
 struct TestMockCryptoCryptoObjectDispatch : public TestMockFixture {
   typedef CryptoObjectDispatch<librbd::MockImageCtx> MockCryptoObjectDispatch;
+  typedef io::AbstractObjectWriteRequest<librbd::MockImageCtx>
+          MockAbstractObjectWriteRequest;
+  typedef io::CopyupRequest<librbd::MockImageCtx> MockCopyupRequest;
   typedef io::util::Mock MockUtils;
 
   MockCryptoInterface* crypto;
@@ -83,6 +120,7 @@ struct TestMockCryptoCryptoObjectDispatch : public TestMockFixture {
   io::Extents extent_map;
   int object_dispatch_flags = 0;
   MockUtils mock_utils;
+  MockCopyupRequest copyup_request;
 
   void SetUp() override {
     TestMockFixture::SetUp();
@@ -91,7 +129,8 @@ struct TestMockCryptoCryptoObjectDispatch : public TestMockFixture {
     ASSERT_EQ(0, open_image(m_image_name, &ictx));
     crypto = new MockCryptoInterface();
     mock_image_ctx = new MockImageCtx(*ictx);
-    mock_crypto_object_dispatch = new MockCryptoObjectDispatch(mock_image_ctx, crypto);
+    mock_crypto_object_dispatch = new MockCryptoObjectDispatch(
+            mock_image_ctx, crypto);
     data.append(std::string(4096, '1'));
   }
 
@@ -136,7 +175,8 @@ struct TestMockCryptoCryptoObjectDispatch : public TestMockFixture {
                           int r) {
     EXPECT_CALL(mock_utils,
                 read_parent(_, object_no, extents, snap_id, _, _))
-            .WillOnce(WithArg<5>(CompleteContext(r, static_cast<asio::ContextWQ*>(nullptr))));
+            .WillOnce(WithArg<5>(CompleteContext(
+                    r, static_cast<asio::ContextWQ*>(nullptr))));
   }
 
   void expect_object_write(uint64_t object_off, const std::string& data,
@@ -172,6 +212,37 @@ struct TestMockCryptoCryptoObjectDispatch : public TestMockFixture {
             }));
   }
 
+  void expect_get_object_size() {
+    EXPECT_CALL(*mock_image_ctx, get_object_size()).WillOnce(Return(
+            mock_image_ctx->layout.object_size));
+  }
+
+  void expect_get_parent_overlap(uint64_t overlap) {
+    EXPECT_CALL(*mock_image_ctx, get_parent_overlap(_, _))
+            .WillOnce(WithArg<1>(Invoke([overlap](uint64_t *o) {
+                *o = overlap;
+                return 0;
+            })));
+  }
+
+  void expect_prune_parent_extents(uint64_t object_overlap) {
+    EXPECT_CALL(*mock_image_ctx, prune_parent_extents(_, _))
+            .WillOnce(Return(object_overlap));
+  }
+
+  void expect_copyup(MockAbstractObjectWriteRequest** write_request, int r) {
+    EXPECT_CALL(copyup_request, append_request(_, _))
+            .WillOnce(WithArg<0>(
+                    Invoke([write_request](
+                            MockAbstractObjectWriteRequest *req) {
+                        *write_request = req;
+                    })));
+    EXPECT_CALL(copyup_request, send())
+            .WillOnce(Invoke([write_request, r]() {
+                (*write_request)->handle_copyup(r);
+            }));
+  }
+
   void expect_encrypt(int count = 1) {
     EXPECT_CALL(*crypto, encrypt(_, _)).Times(count);
   }
@@ -365,6 +436,8 @@ TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteWithNoObject) {
   ASSERT_EQ(dispatch_result, io::DISPATCH_RESULT_COMPLETE);
   ASSERT_EQ(on_finish, &finished_cond);
 
+  expect_get_object_size();
+  expect_get_parent_overlap(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,
@@ -387,6 +460,8 @@ TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteFailCreate) {
   ASSERT_EQ(dispatch_result, io::DISPATCH_RESULT_COMPLETE);
   ASSERT_EQ(on_finish, &finished_cond);
 
+  expect_get_object_size();
+  expect_get_parent_overlap(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,
@@ -410,6 +485,50 @@ TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteFailCreate) {
   ASSERT_EQ(0, dispatched_cond.wait());
 }
 
+TEST_F(TestMockCryptoCryptoObjectDispatch, UnalignedWriteCopyup) {
+  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_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
+  uint64_t version = 1234;
+  extents[0].bl.append(std::string(4096, '2'));
+  extents[1].bl.append(std::string(4096, '3'));
+  expect_object_read(&extents, version);
+  dispatcher_ctx->complete(-ENOENT); // complete first read
+  ASSERT_EQ(ETIMEDOUT, dispatched_cond.wait_for(0));
+
+  auto expected_data =
+        std::string("2") + std::string(8192, '1') + std::string(4095, '3');
+  expect_object_write(0, expected_data, 0, std::make_optional(version));
+  dispatcher_ctx->complete(0); // 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;