From: Or Ozeri Date: Thu, 29 Oct 2020 11:42:17 +0000 (+0200) Subject: librbd: wait for copyup in unaligned crypto write X-Git-Tag: v16.1.0~637^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=27c24d39ed00ea2273dce59d56c3540c93be2a4f;p=ceph.git 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 --- diff --git a/src/librbd/crypto/CryptoObjectDispatch.cc b/src/librbd/crypto/CryptoObjectDispatch.cc index d17dcbcc6de7..b09aad720a6b 100644 --- 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( 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, + &C_UnalignedObjectWriteRequest::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( + 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( - 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 80e8032ef5f3..99a911410aae 100644 --- 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 +bool trigger_copyup(I* image_ctx, uint64_t object_no, IOContext io_context, + Context* on_finish) { + bufferlist bl; + auto req = new ObjectWriteRequest( + 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 d28786b2a3d7..22b86058a8bb 100644 --- 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 @@ -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 +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 2ecdf4ec81f1..76455221726a 100644 --- 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 @@ -54,18 +55,13 @@ public: } } - bufferlist bl; - auto req = new io::ObjectWriteRequest(&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 181cc2caf943..805a83d75bda 100644 --- 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; +template class librbd::io::AbstractObjectWriteRequest; + namespace librbd { namespace util { @@ -20,6 +30,29 @@ inline ImageCtx *get_image_ctx(MockImageCtx *image_ctx) { } // namespace util namespace io { + +template <> +struct CopyupRequest { + MOCK_METHOD0(send, void()); + MOCK_METHOD2(append_request, void( + AbstractObjectWriteRequest*, + 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* 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 MockCryptoObjectDispatch; + typedef io::AbstractObjectWriteRequest + MockAbstractObjectWriteRequest; + typedef io::CopyupRequest 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(nullptr)))); + .WillOnce(WithArg<5>(CompleteContext( + r, static_cast(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;