Merge pull request #7303 from cbodley/wip-tx-move

os/ObjectStore: add custom move operations for ObjectStore::Transaction

Reviewed-by: Kefu Chai <kchai@redhat.com>

src/os/ObjectStore.h

@@ -431,6 +431,36 @@ class ObjectStore {
         largest_data_off_in_tbl(0),
 	fadvise_flags(0) { }
 
+      // override default move operations to reset default values
+      TransactionData(TransactionData&& other) :
+        ops(other.ops),
+        largest_data_len(other.largest_data_len),
+        largest_data_off(other.largest_data_off),
+        largest_data_off_in_tbl(other.largest_data_off_in_tbl),
+        fadvise_flags(other.fadvise_flags) {
+        other.ops = 0;
+        other.largest_data_len = 0;
+        other.largest_data_off = 0;
+        other.largest_data_off_in_tbl = 0;
+        other.fadvise_flags = 0;
+      }
+      TransactionData& operator=(TransactionData&& other) {
+        ops = other.ops;
+        largest_data_len = other.largest_data_len;
+        largest_data_off = other.largest_data_off;
+        largest_data_off_in_tbl = other.largest_data_off_in_tbl;
+        fadvise_flags = other.fadvise_flags;
+        other.ops = 0;
+        other.largest_data_len = 0;
+        other.largest_data_off = 0;
+        other.largest_data_off_in_tbl = 0;
+        other.fadvise_flags = 0;
+        return *this;
+      }
+
+      TransactionData(const TransactionData& other) = default;
+      TransactionData& operator=(const TransactionData& other) = default;
+
       void encode(bufferlist& bl) const {
         bl.append((char*)this, sizeof(TransactionData));
       }
@@ -442,16 +472,16 @@ class ObjectStore {
   private:
     TransactionData data;
 
-    void *osr; // NULL on replay
+    void *osr {nullptr}; // NULL on replay
 
-    bool use_tbl;   //use_tbl for encode/decode
+    bool use_tbl {false};   //use_tbl for encode/decode
     bufferlist tbl;
 
     map<coll_t, __le32> coll_index;
     map<ghobject_t, __le32, ghobject_t::BitwiseComparator> object_index;
 
-    __le32 coll_id;
-    __le32 object_id;
+    __le32 coll_id {0};
+    __le32 object_id {0};
 
     bufferlist data_bl;
     bufferlist op_bl;
@@ -463,6 +493,62 @@ class ObjectStore {
     list<Context *> on_applied_sync;
 
   public:
+    Transaction() = default;
+
+    Transaction(bufferlist::iterator &dp) {
+      decode(dp);
+    }
+    Transaction(bufferlist &nbl) {
+      bufferlist::iterator dp = nbl.begin();
+      decode(dp);
+    }
+
+    // override default move operations to reset default values
+    Transaction(Transaction&& other) :
+      data(std::move(other.data)),
+      osr(other.osr),
+      use_tbl(other.use_tbl),
+      tbl(std::move(other.tbl)),
+      coll_index(std::move(other.coll_index)),
+      object_index(std::move(other.object_index)),
+      coll_id(other.coll_id),
+      object_id(other.object_id),
+      data_bl(std::move(other.data_bl)),
+      op_bl(std::move(other.op_bl)),
+      op_ptr(std::move(other.op_ptr)),
+      on_applied(std::move(other.on_applied)),
+      on_commit(std::move(other.on_commit)),
+      on_applied_sync(std::move(other.on_applied_sync)) {
+      other.osr = nullptr;
+      other.use_tbl = false;
+      other.coll_id = 0;
+      other.object_id = 0;
+    }
+
+    Transaction& operator=(Transaction&& other) {
+      data = std::move(other.data);
+      osr = other.osr;
+      use_tbl = other.use_tbl;
+      tbl = std::move(other.tbl);
+      coll_index = std::move(other.coll_index);
+      object_index = std::move(other.object_index);
+      coll_id = other.coll_id;
+      object_id = other.object_id;
+      data_bl = std::move(other.data_bl);
+      op_bl = std::move(other.op_bl);
+      op_ptr = std::move(other.op_ptr);
+      on_applied = std::move(other.on_applied);
+      on_commit = std::move(other.on_commit);
+      on_applied_sync = std::move(other.on_applied_sync);
+      other.osr = nullptr;
+      other.use_tbl = false;
+      other.coll_id = 0;
+      other.object_id = 0;
+      return *this;
+    }
+
+    Transaction(const Transaction& other) = default;
+    Transaction& operator=(const Transaction& other) = default;
 
     /* Operations on callback contexts */
     void register_on_applied(Context *c) {
@@ -1588,30 +1674,6 @@ class ObjectStore {
       data.ops++;
     }
 
-    // etc.
-    Transaction() :
-      osr(NULL),
-      use_tbl(false),
-      coll_id(0),
-      object_id(0) { }
-
-    Transaction(bufferlist::iterator &dp) :
-      osr(NULL),
-      use_tbl(false),
-      coll_id(0),
-      object_id(0) {
-      decode(dp);
-    }
-
-    Transaction(bufferlist &nbl) :
-      osr(NULL),
-      use_tbl(false),
-      coll_id(0),
-      object_id(0) {
-      bufferlist::iterator dp = nbl.begin();
-      decode(dp);
-    }
-
     void encode(bufferlist& bl) const {
       if (use_tbl) {
         uint64_t ops = data.ops;

src/test/CMakeLists.txt

@@ -2160,6 +2160,12 @@ target_link_libraries(test_objectstore_workloadgen
   ${CMAKE_DL_LIBS}
   )
 
+# test_transaction
+add_executable(unittest_transaction objectstore/test_transaction.cc)
+add_test(unittest_transaction unittest_transaction)
+set_target_properties(unittest_transaction PROPERTIES COMPILE_FLAGS ${UNITTEST_CXX_FLAGS})
+target_link_libraries(unittest_transaction os common ${UNITTEST_LIBS})
+
 #test_filestore
 add_executable(test_filestore filestore/TestFileStore.cc)
 set_target_properties(test_filestore PROPERTIES COMPILE_FLAGS

src/test/Makefile-server.am

@@ -94,6 +94,11 @@ ceph_test_filestore_idempotent_sequence_SOURCES = \
 ceph_test_filestore_idempotent_sequence_LDADD = $(LIBOS) $(CEPH_GLOBAL)
 bin_DEBUGPROGRAMS += ceph_test_filestore_idempotent_sequence
 
+unittest_transaction_SOURCES = test/objectstore/test_transaction.cc
+unittest_transaction_LDADD = $(LIBOS) $(UNITTEST_LDADD) $(CEPH_GLOBAL)
+unittest_transaction_CXXFLAGS = $(UNITTEST_CXXFLAGS)
+check_TESTPROGRAMS += unittest_transaction
+
 ceph_xattr_bench_SOURCES = test/xattr_bench.cc
 ceph_xattr_bench_LDADD = $(LIBOS) $(UNITTEST_LDADD) $(CEPH_GLOBAL)
 ceph_xattr_bench_CXXFLAGS = $(UNITTEST_CXXFLAGS)

src/test/objectstore/test_transaction.cc

@@ -0,0 +1,75 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+/*
+ * Ceph - scalable distributed file system
+ *
+ * Copyright (C) 2016 Casey Bodley <cbodley@redhat.com>
+ *
+ * This is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2.1, as published by the Free Software
+ * Foundation.  See file COPYING.
+ *
+ */
+
+#include "os/ObjectStore.h"
+#include <gtest/gtest.h>
+
+TEST(Transaction, MoveConstruct)
+{
+  auto a = ObjectStore::Transaction{};
+  a.nop();
+  ASSERT_FALSE(a.empty());
+
+  // move-construct in b
+  auto b = std::move(a);
+  ASSERT_TRUE(a.empty());
+  ASSERT_FALSE(b.empty());
+}
+
+TEST(Transaction, MoveAssign)
+{
+  auto a = ObjectStore::Transaction{};
+  a.nop();
+  ASSERT_FALSE(a.empty());
+
+  auto b = ObjectStore::Transaction{};
+  b = std::move(a); // move-assign to b
+  ASSERT_TRUE(a.empty());
+  ASSERT_FALSE(b.empty());
+}
+
+TEST(Transaction, CopyConstruct)
+{
+  auto a = ObjectStore::Transaction{};
+  a.nop();
+  ASSERT_FALSE(a.empty());
+
+  auto b = a; // copy-construct in b
+  ASSERT_FALSE(a.empty());
+  ASSERT_FALSE(b.empty());
+}
+
+TEST(Transaction, CopyAssign)
+{
+  auto a = ObjectStore::Transaction{};
+  a.nop();
+  ASSERT_FALSE(a.empty());
+
+  auto b = ObjectStore::Transaction{};
+  b = a; // copy-assign to b
+  ASSERT_FALSE(a.empty());
+  ASSERT_FALSE(b.empty());
+}
+
+TEST(Transaction, Swap)
+{
+  auto a = ObjectStore::Transaction{};
+  a.nop();
+  ASSERT_FALSE(a.empty());
+
+  auto b = ObjectStore::Transaction{};
+  std::swap(a, b); // swap a and b
+  ASSERT_TRUE(a.empty());
+  ASSERT_FALSE(b.empty());
+}