From 8b3479e2bd70bc036dc0ca527636b71ea233c395 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Thu, 31 Oct 2024 16:56:44 -0400 Subject: [PATCH] librados/asio: async_operate() takes ownership of ObjectOperation args for these async_operate() overloads, deferred initiation makes it possible for the ObjectOperations arguments, passed by pointer, to go out of scope before use because ObjectOperation is move-only, we can't pass it by reference like IoCtx& and expect the initiation functions to make a copy when necessary. so our only option is to use move operations instead, requiring that the caller pass an rvalue in the first place Signed-off-by: Casey Bodley --- src/common/io_exerciser/RadosIo.cc | 33 ++++++++++++---------- src/common/io_exerciser/RadosIo.h | 4 +-- src/librados/librados_asio.h | 20 +++++++------- src/rgw/driver/rados/rgw_tools.cc | 4 +-- src/rgw/rgw_aio.cc | 2 +- src/test/librados/asio.cc | 44 ++++++++++++++++-------------- 6 files changed, 57 insertions(+), 50 deletions(-) diff --git a/src/common/io_exerciser/RadosIo.cc b/src/common/io_exerciser/RadosIo.cc index 48ac9bc2b503..ba598fa50a03 100644 --- a/src/common/io_exerciser/RadosIo.cc +++ b/src/common/io_exerciser/RadosIo.cc @@ -109,26 +109,28 @@ void RadosIo::applyIoOp(IoOp& op) { std::make_shared>(std::array{0}, std::array{opSize}); op_info->bufferlist[0] = db->generate_data(0, opSize); - op_info->wop.write_full(op_info->bufferlist[0]); + librados::ObjectWriteOperation wop; + wop.write_full(op_info->bufferlist[0]); auto create_cb = [this](boost::system::error_code ec, version_t ver) { ceph_assert(ec == boost::system::errc::success); finish_io(); }; librados::async_operate(asio.get_executor(), io, oid, - &op_info->wop, 0, nullptr, create_cb); + std::move(wop), 0, nullptr, create_cb); break; } case OpType::Remove: { start_io(); auto op_info = std::make_shared>(); - op_info->wop.remove(); + librados::ObjectWriteOperation wop; + wop.remove(); auto remove_cb = [this](boost::system::error_code ec, version_t ver) { ceph_assert(ec == boost::system::errc::success); finish_io(); }; librados::async_operate(asio.get_executor(), io, oid, - &op_info->wop, 0, nullptr, remove_cb); + std::move(wop), 0, nullptr, remove_cb); break; } case OpType::Read: @@ -171,10 +173,11 @@ void RadosIo::applyReadWriteOp(IoOp& op) { auto op_info = std::make_shared>(readOp.offset, readOp.length); + librados::ObjectReadOperation rop; for (int i = 0; i < N; i++) { - op_info->rop.read(readOp.offset[i] * block_size, - readOp.length[i] * block_size, &op_info->bufferlist[i], - nullptr); + rop.read(readOp.offset[i] * block_size, + readOp.length[i] * block_size, &op_info->bufferlist[i], + nullptr); } auto read_cb = [this, op_info](boost::system::error_code ec, version_t ver, bufferlist bl) { @@ -186,7 +189,7 @@ void RadosIo::applyReadWriteOp(IoOp& op) { finish_io(); }; librados::async_operate(asio.get_executor(), io, oid, - &op_info->rop, 0, nullptr, read_cb); + std::move(rop), 0, nullptr, read_cb); num_io++; }; @@ -194,18 +197,19 @@ void RadosIo::applyReadWriteOp(IoOp& op) { ReadWriteOp writeOp) { auto op_info = std::make_shared>(writeOp.offset, writeOp.length); + librados::ObjectWriteOperation wop; for (int i = 0; i < N; i++) { op_info->bufferlist[i] = db->generate_data(writeOp.offset[i], writeOp.length[i]); - op_info->wop.write(writeOp.offset[i] * block_size, - op_info->bufferlist[i]); + wop.write(writeOp.offset[i] * block_size, + op_info->bufferlist[i]); } auto write_cb = [this](boost::system::error_code ec, version_t ver) { ceph_assert(ec == boost::system::errc::success); finish_io(); }; librados::async_operate(asio.get_executor(), io, oid, - &op_info->wop, 0, nullptr, write_cb); + std::move(wop), 0, nullptr, write_cb); num_io++; }; @@ -213,11 +217,12 @@ void RadosIo::applyReadWriteOp(IoOp& op) { ReadWriteOp writeOp) { auto op_info = std::make_shared>(writeOp.offset, writeOp.length); + librados::ObjectWriteOperation wop; for (int i = 0; i < N; i++) { op_info->bufferlist[i] = db->generate_data(writeOp.offset[i], writeOp.length[i]); - op_info->wop.write(writeOp.offset[i] * block_size, - op_info->bufferlist[i]); + wop.write(writeOp.offset[i] * block_size, + op_info->bufferlist[i]); } auto write_cb = [this, writeOp](boost::system::error_code ec, version_t ver) { @@ -225,7 +230,7 @@ void RadosIo::applyReadWriteOp(IoOp& op) { finish_io(); }; librados::async_operate(asio.get_executor(), io, oid, - &op_info->wop, 0, nullptr, write_cb); + std::move(wop), 0, nullptr, write_cb); num_io++; }; diff --git a/src/common/io_exerciser/RadosIo.h b/src/common/io_exerciser/RadosIo.h index a5c66ad4768a..d72c941cdf41 100644 --- a/src/common/io_exerciser/RadosIo.h +++ b/src/common/io_exerciser/RadosIo.h @@ -51,8 +51,6 @@ class RadosIo : public Model { template class AsyncOpInfo { public: - librados::ObjectReadOperation rop; - librados::ObjectWriteOperation wop; std::array bufferlist; std::array offset; std::array length; @@ -71,4 +69,4 @@ class RadosIo : public Model { void applyInjectOp(IoOp& op); }; } // namespace io_exerciser -} // namespace ceph \ No newline at end of file +} // namespace ceph diff --git a/src/librados/librados_asio.h b/src/librados/librados_asio.h index 7d767603b638..00a82f285172 100644 --- a/src/librados/librados_asio.h +++ b/src/librados/librados_asio.h @@ -213,20 +213,20 @@ auto async_write(IoExecutor ex, IoCtx& io, const std::string& oid, /// instance must preserve its underlying implementation until completion. template auto async_operate(IoExecutor ex, IoCtx& io, const std::string& oid, - ObjectReadOperation *read_op, int flags, + ObjectReadOperation read_op, int flags, const jspan_context* trace_ctx, CompletionToken&& token) { using Op = detail::AsyncOp; using Signature = typename Op::Signature; return boost::asio::async_initiate( [] (auto handler, IoExecutor ex, const IoCtx& i, const std::string& oid, - ObjectReadOperation *read_op, int flags) { + ObjectReadOperation read_op, int flags) { constexpr bool is_read = true; auto p = Op::create(ex, is_read, std::move(handler)); auto& op = p->user_data; - IoCtx& io = const_cast(i); - int ret = io.aio_operate(oid, op.aio_completion.get(), read_op, + auto& io = const_cast(i); + int ret = io.aio_operate(oid, op.aio_completion.get(), &read_op, flags, &op.result); if (ret < 0) { auto ec = boost::system::error_code{-ret, librados::detail::err_category()}; @@ -234,7 +234,7 @@ auto async_operate(IoExecutor ex, IoCtx& io, const std::string& oid, } else { p.release(); // release ownership until completion } - }, token, ex, io, oid, read_op, flags); + }, token, ex, io, oid, std::move(read_op), flags); } /// Calls IoCtx::aio_operate() and arranges for the AioCompletion to call a @@ -244,28 +244,28 @@ auto async_operate(IoExecutor ex, IoCtx& io, const std::string& oid, /// instance must preserve its underlying implementation until completion. template auto async_operate(IoExecutor ex, IoCtx& io, const std::string& oid, - ObjectWriteOperation *write_op, int flags, + ObjectWriteOperation write_op, int flags, const jspan_context* trace_ctx, CompletionToken &&token) { using Op = detail::AsyncOp; using Signature = typename Op::Signature; return boost::asio::async_initiate( [] (auto handler, IoExecutor ex, const IoCtx& i, const std::string& oid, - ObjectWriteOperation *write_op, int flags, + ObjectWriteOperation write_op, int flags, const jspan_context* trace_ctx) { constexpr bool is_read = false; auto p = Op::create(ex, is_read, std::move(handler)); auto& op = p->user_data; - IoCtx& io = const_cast(i); - int ret = io.aio_operate(oid, op.aio_completion.get(), write_op, flags, trace_ctx); + auto& io = const_cast(i); + int ret = io.aio_operate(oid, op.aio_completion.get(), &write_op, flags, trace_ctx); if (ret < 0) { auto ec = boost::system::error_code{-ret, librados::detail::err_category()}; ceph::async::post(std::move(p), ec, 0); } else { p.release(); // release ownership until completion } - }, token, ex, io, oid, write_op, flags, trace_ctx); + }, token, ex, io, oid, std::move(write_op), flags, trace_ctx); } /// Calls IoCtx::aio_notify() and arranges for the AioCompletion to call a diff --git a/src/rgw/driver/rados/rgw_tools.cc b/src/rgw/driver/rados/rgw_tools.cc index a36e7a58170e..0a15f87959ab 100644 --- a/src/rgw/driver/rados/rgw_tools.cc +++ b/src/rgw/driver/rados/rgw_tools.cc @@ -208,7 +208,7 @@ int rgw_rados_operate(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, con auto& yield = y.get_yield_context(); auto ex = yield.get_executor(); boost::system::error_code ec; - auto [ver, bl] = librados::async_operate(ex, ioctx, oid, op, + auto [ver, bl] = librados::async_operate(ex, ioctx, oid, std::move(*op), flags, trace_info, yield[ec]); if (pbl) { *pbl = std::move(bl); @@ -234,7 +234,7 @@ int rgw_rados_operate(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, con auto& yield = y.get_yield_context(); auto ex = yield.get_executor(); boost::system::error_code ec; - version_t ver = librados::async_operate(ex, ioctx, oid, op, + version_t ver = librados::async_operate(ex, ioctx, oid, std::move(*op), flags, trace_info, yield[ec]); if (pver) { *pver = ver; diff --git a/src/rgw/rgw_aio.cc b/src/rgw/rgw_aio.cc index e3a455e233cb..dfe3306fdb2c 100644 --- a/src/rgw/rgw_aio.cc +++ b/src/rgw/rgw_aio.cc @@ -97,7 +97,7 @@ Aio::OpFunc aio_abstract(librados::IoCtx ctx, Op&& op, // executor so it can safely call back into Aio without locking auto ex = yield.get_executor(); - librados::async_operate(ex, ctx, r.obj.oid, &op, 0, trace_ctx, + librados::async_operate(ex, ctx, r.obj.oid, std::move(op), 0, trace_ctx, bind_executor(ex, Handler{aio, ctx, r})); }; } diff --git a/src/test/librados/asio.cc b/src/test/librados/asio.cc index e9af5719e24f..27271442b6c7 100644 --- a/src/test/librados/asio.cc +++ b/src/test/librados/asio.cc @@ -244,7 +244,8 @@ TEST_F(AsioRados, AsyncReadOperationCallback) EXPECT_LT(0, ver); EXPECT_EQ("hello", bl.to_str()); }; - librados::async_operate(ex, io, "exist", &op, 0, nullptr, success_cb); + librados::async_operate(ex, io, "exist", std::move(op), + 0, nullptr, success_cb); } { librados::ObjectReadOperation op; @@ -254,7 +255,8 @@ TEST_F(AsioRados, AsyncReadOperationCallback) EXPECT_EQ(0, ver); EXPECT_EQ(0, bl.length()); }; - librados::async_operate(ex, io, "noexist", &op, 0, nullptr, failure_cb); + librados::async_operate(ex, io, "noexist", std::move(op), + 0, nullptr, failure_cb); } service.run(); } @@ -267,15 +269,15 @@ TEST_F(AsioRados, AsyncReadOperationFuture) { librados::ObjectReadOperation op; op.read(0, 0, nullptr, nullptr); - f1 = librados::async_operate(ex, io, "exist", &op, 0, nullptr, - boost::asio::use_future); + f1 = librados::async_operate(ex, io, "exist", std::move(op), + 0, nullptr, boost::asio::use_future); } std::future f2; { librados::ObjectReadOperation op; op.read(0, 0, nullptr, nullptr); - f2 = librados::async_operate(ex, io, "noexist", &op, 0, nullptr, - boost::asio::use_future); + f2 = librados::async_operate(ex, io, "noexist", std::move(op), + 0, nullptr, boost::asio::use_future); } service.run(); @@ -295,7 +297,7 @@ TEST_F(AsioRados, AsyncReadOperationYield) librados::ObjectReadOperation op; op.read(0, 0, nullptr, nullptr); error_code ec; - auto [ver, bl] = librados::async_operate(ex, io, "exist", &op, + auto [ver, bl] = librados::async_operate(ex, io, "exist", std::move(op), 0, nullptr, yield[ec]); EXPECT_FALSE(ec); EXPECT_LT(0, ver); @@ -307,7 +309,7 @@ TEST_F(AsioRados, AsyncReadOperationYield) librados::ObjectReadOperation op; op.read(0, 0, nullptr, nullptr); error_code ec; - auto [ver, bl] = librados::async_operate(ex, io, "noexist", &op, + auto [ver, bl] = librados::async_operate(ex, io, "noexist", std::move(op), 0, nullptr, yield[ec]); EXPECT_EQ(boost::system::errc::no_such_file_or_directory, ec); EXPECT_EQ(0, ver); @@ -333,7 +335,8 @@ TEST_F(AsioRados, AsyncWriteOperationCallback) EXPECT_FALSE(ec); EXPECT_LT(0, ver); }; - librados::async_operate(ex, io, "exist", &op, 0, nullptr, success_cb); + librados::async_operate(ex, io, "exist", std::move(op), + 0, nullptr, success_cb); } { librados::ObjectWriteOperation op; @@ -342,7 +345,8 @@ TEST_F(AsioRados, AsyncWriteOperationCallback) EXPECT_EQ(boost::system::errc::read_only_file_system, ec); EXPECT_EQ(0, ver); }; - librados::async_operate(ex, snapio, "exist", &op, 0, nullptr, failure_cb); + librados::async_operate(ex, snapio, "exist", std::move(op), + 0, nullptr, failure_cb); } service.run(); } @@ -359,15 +363,15 @@ TEST_F(AsioRados, AsyncWriteOperationFuture) { librados::ObjectWriteOperation op; op.write_full(bl); - f1 = librados::async_operate(ex, io, "exist", &op, 0, nullptr, - boost::asio::use_future); + f1 = librados::async_operate(ex, io, "exist", std::move(op), + 0, nullptr, boost::asio::use_future); } std::future f2; { librados::ObjectWriteOperation op; op.write_full(bl); - f2 = librados::async_operate(ex, snapio, "exist", &op, 0, nullptr, - boost::asio::use_future); + f2 = librados::async_operate(ex, snapio, "exist", std::move(op), + 0, nullptr, boost::asio::use_future); } service.run(); @@ -387,7 +391,7 @@ TEST_F(AsioRados, AsyncWriteOperationYield) librados::ObjectWriteOperation op; op.write_full(bl); error_code ec; - auto ver = librados::async_operate(ex, io, "exist", &op, + auto ver = librados::async_operate(ex, io, "exist", std::move(op), 0, nullptr, yield[ec]); EXPECT_FALSE(ec); EXPECT_LT(0, ver); @@ -398,7 +402,7 @@ TEST_F(AsioRados, AsyncWriteOperationYield) librados::ObjectWriteOperation op; op.write_full(bl); error_code ec; - auto ver = librados::async_operate(ex, snapio, "exist", &op, + auto ver = librados::async_operate(ex, snapio, "exist", std::move(op), 0, nullptr, yield[ec]); EXPECT_EQ(boost::system::errc::read_only_file_system, ec); EXPECT_EQ(0, ver); @@ -425,7 +429,7 @@ TEST_F(AsioRados, AsyncReadOperationCancelTerminal) librados::ObjectReadOperation op; op.assert_exists(); - librados::async_operate(ex, io, "noexist", &op, 0, nullptr, + librados::async_operate(ex, io, "noexist", std::move(op), 0, nullptr, capture(signal, result)); service.poll(); @@ -457,7 +461,7 @@ TEST_F(AsioRados, AsyncReadOperationCancelTotal) librados::ObjectReadOperation op; op.assert_exists(); - librados::async_operate(ex, io, "noexist", &op, 0, nullptr, + librados::async_operate(ex, io, "noexist", std::move(op), 0, nullptr, capture(signal, result)); service.poll(); @@ -489,7 +493,7 @@ TEST_F(AsioRados, AsyncWriteOperationCancelTerminal) librados::ObjectWriteOperation op; op.assert_exists(); - librados::async_operate(ex, io, "noexist", &op, 0, nullptr, + librados::async_operate(ex, io, "noexist", std::move(op), 0, nullptr, capture(signal, result)); service.poll(); @@ -517,7 +521,7 @@ TEST_F(AsioRados, AsyncWriteOperationCancelTotal) librados::ObjectWriteOperation op; op.assert_exists(); - librados::async_operate(ex, io, "noexist", &op, 0, nullptr, + librados::async_operate(ex, io, "noexist", std::move(op), 0, nullptr, capture(signal, ec)); service.poll(); -- 2.47.3