crimson/os/seastore: fix potential leak for onodes to live across transactions

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/tree.h b/src/crimson/os/seastore/onode_manager/staged-fltree/tree.h
index b6a282e03b3516f76ab3a75205ca71cf4a38a1c0..656f216bf38141e2a0237a720bd27b1eee3e4820 100644 (file)
--- a/src/crimson/os/seastore/onode_manager/staged-fltree/tree.h
+++ b/src/crimson/os/seastore/onode_manager/staged-fltree/tree.h
@@ -25,8 +25,8 @@
  * - Specially optimized for onode key structures and seastore
  *   delta/transaction semantics;
  *
- * Note: User should not hold any Cursor/Value when call
- * submit_transaction() because of validations implemented in ~tree_cursor_t().
+ * Note: Cursor/Value are transactional, they cannot be used outside the scope
+ * of the according transaction, or the behavior is undefined.
  */
 
 namespace crimson::os::seastore::onode {

diff --git a/src/crimson/os/seastore/seastore.cc b/src/crimson/os/seastore/seastore.cc
index 98b32eaabb0222f378c7fffbfaf55be2091ce5b1..3b31a694f7572fe53299a79d0ac32b93350af566 100644 (file)
--- a/src/crimson/os/seastore/seastore.cc
+++ b/src/crimson/os/seastore/seastore.cc
@@ -886,13 +886,14 @@ seastar::future<> SeaStore::do_transaction(
       return with_trans_intr(*ctx.transaction, [&, this](auto &t) {
         return onode_manager->get_or_create_onodes(
           *ctx.transaction, ctx.iter.get_objects()
-        ).si_then([this, &ctx](auto &&read_onodes) {
-          ctx.onodes = std::move(read_onodes);
-          return trans_intr::repeat(
-            [this, &ctx]() -> tm_iertr::future<seastar::stop_iteration> {
+        ).si_then([this, &ctx](auto &&onodes) {
+          return seastar::do_with(std::move(onodes), [this, &ctx](auto& onodes) {
+            return trans_intr::repeat(
+              [this, &ctx, &onodes]() -> tm_iertr::future<seastar::stop_iteration>
+            {
               if (ctx.iter.have_op()) {
                 return _do_transaction_step(
-                  ctx, ctx.ch, ctx.onodes, ctx.iter
+                  ctx, ctx.ch, onodes, ctx.iter
                 ).si_then([] {
                   return seastar::make_ready_future<seastar::stop_iteration>(
                     seastar::stop_iteration::no);
@@ -901,15 +902,11 @@ seastar::future<> SeaStore::do_transaction(
                 return seastar::make_ready_future<seastar::stop_iteration>(
                   seastar::stop_iteration::yes);
               };
-            }
-          );
-        }).si_then([this, &ctx] {
-          return onode_manager->write_dirty(*ctx.transaction, ctx.onodes);
+            }).si_then([this, &ctx, &onodes] {
+              return onode_manager->write_dirty(*ctx.transaction, onodes);
+            });
+          });
         }).si_then([this, &ctx] {
-          // There are some validations in onode tree during onode value
-          // destruction in debug mode, which need to be done before calling
-          // submit_transaction().
-          ctx.onodes.clear();
           return transaction_manager->submit_transaction(*ctx.transaction);
         });
       }).safe_then([&ctx]() {

diff --git a/src/crimson/os/seastore/seastore.h b/src/crimson/os/seastore/seastore.h
index d5a79a3fdb84c150e0644c2dab92e481ad10d476..d1c325432d7ff93b485f47e2611586d6c6e31f6d 100644 (file)
--- a/src/crimson/os/seastore/seastore.h
+++ b/src/crimson/os/seastore/seastore.h
@@ -182,14 +182,12 @@ private:
        iter(ext_transaction.begin()) {}
 
     TransactionRef transaction;
-    std::vector<OnodeRef> onodes;
 
     ceph::os::Transaction::iterator iter;
     std::chrono::steady_clock::time_point begin_timestamp = std::chrono::steady_clock::now();
 
     void reset_preserve_handle(TransactionManager &tm) {
       tm.reset_transaction_preserve_handle(*transaction);
-      onodes.clear();
       iter = ext_transaction.begin();
     }
   };
@@ -240,9 +238,9 @@ private:
     F &&f) const {
     auto begin_time = std::chrono::steady_clock::now();
     return seastar::do_with(
-      oid, Ret{}, OnodeRef(), std::forward<F>(f),
+      oid, Ret{}, std::forward<F>(f),
       [this, src, op_type, begin_time, tname
-      ](auto &oid, auto &ret, auto &onode, auto &f)
+      ](auto &oid, auto &ret, auto &f)
     {
       return repeat_eagain([&, this, src, tname] {
         return transaction_manager->with_transaction_intr(
@@ -251,11 +249,11 @@ private:
           [&, this](auto& t)
         {
           return onode_manager->get_onode(t, oid
-          ).si_then([&](auto onode_ret) {
-            onode = std::move(onode_ret);
-            return f(t, *onode);
-          }).si_then([&ret, &onode](auto _ret) {
-           onode.reset();
+          ).si_then([&](auto onode) {
+            return seastar::do_with(std::move(onode), [&](auto& onode) {
+              return f(t, *onode);
+            });
+          }).si_then([&ret](auto _ret) {
             ret = _ret;
           });
         });