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 <cbodley@redhat.com>

diff --git a/src/common/io_exerciser/RadosIo.cc b/src/common/io_exerciser/RadosIo.cc
index 48ac9bc2b503a184ded47ed85241c2907a6b5d55..ba598fa50a03f8fb0dd40127330db5655e355817 100644 (file)
--- 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<AsyncOpInfo<1>>(std::array<uint64_t, 1>{0},
                                            std::array<uint64_t, 1>{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<AsyncOpInfo<0>>();
-      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<AsyncOpInfo<N>>(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<opType, N> writeOp) {
     auto op_info =
         std::make_shared<AsyncOpInfo<N>>(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<opType, N> writeOp) {
     auto op_info =
         std::make_shared<AsyncOpInfo<N>>(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 a5c66ad4768abbcbe362956932ba4242a00a4d5a..d72c941cdf4153afe1db32165ce3253e89b8e9bd 100644 (file)
--- a/src/common/io_exerciser/RadosIo.h
+++ b/src/common/io_exerciser/RadosIo.h
@@ -51,8 +51,6 @@ class RadosIo : public Model {
   template <int N>
   class AsyncOpInfo {
    public:
-    librados::ObjectReadOperation rop;
-    librados::ObjectWriteOperation wop;
     std::array<ceph::bufferlist, N> bufferlist;
     std::array<uint64_t, N> offset;
     std::array<uint64_t, N> 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 7d767603b63849244173095e56a36bd8bcb8da52..00a82f285172cff3784b86e7af4aa70503e735aa 100644 (file)
--- 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 <boost::asio::execution::executor IoExecutor, typename CompletionToken>
 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<bufferlist>;
   using Signature = typename Op::Signature;
   return boost::asio::async_initiate<CompletionToken, Signature>(
       [] (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<IoCtx&>(i);
-        int ret = io.aio_operate(oid, op.aio_completion.get(), read_op,
+        auto& io = const_cast<IoCtx&>(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 <boost::asio::execution::executor IoExecutor, typename CompletionToken>
 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<void>;
   using Signature = typename Op::Signature;
   return boost::asio::async_initiate<CompletionToken, Signature>(
       [] (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<IoCtx&>(i);
-        int ret = io.aio_operate(oid, op.aio_completion.get(), write_op, flags, trace_ctx);
+        auto& io = const_cast<IoCtx&>(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 a36e7a58170ee0c13ce372dfc21766f1022c9169..0a15f87959ab14d5a6333020a0e1fb5ae82948fc 100644 (file)
--- 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 e3a455e233cb76d74f6253ebc19a2332273d015b..dfe3306fdb2c6738d9d1a6f0dc4ea4f4c2a954b9 100644 (file)
--- 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 e9af5719e24f2c06670e8539ff2be97369348384..27271442b6c7c15ef80454ddf961424f91c9717f 100644 (file)
--- 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<read_result> 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<version_t> 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();