mds: move BatchOp to separate translation unit

To simplify logging and reduce coupling. Also a few other miscellaneous
refactorings.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

diff --git a/src/mds/BatchOp.cc b/src/mds/BatchOp.cc
new file mode 100644 (file)
index 0000000..c2152a6
--- /dev/null
+++ b/src/mds/BatchOp.cc
@@ -0,0 +1,35 @@
+// -*- 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) 2019 Red Hat
+ *
+ * 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 "common/debug.h"
+#define dout_context g_ceph_context
+#define dout_subsys ceph_subsys_mds
+
+#include "BatchOp.h"
+
+void BatchOp::forward(mds_rank_t target)
+{
+  dout(20) << __func__ << ": forwarding batch ops to " << target << ": ";
+  print(*_dout);
+  *_dout << dendl;
+  _forward(target);
+}
+
+void BatchOp::respond(int r)
+{
+  dout(20) << __func__ << ": responding to batch ops with result=" << r << ": ";
+  print(*_dout);
+  *_dout << dendl;
+  _respond(r);
+}

diff --git a/src/mds/BatchOp.h b/src/mds/BatchOp.h
new file mode 100644 (file)
index 0000000..b54ad96
--- /dev/null
+++ b/src/mds/BatchOp.h
@@ -0,0 +1,40 @@
+// -*- 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) 2012 Red Hat
+ *
+ * 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.
+ *
+ */
+
+
+#ifndef MDS_BATCHOP_H
+#define MDS_BATCHOP_H
+
+#include "common/ref.h"
+
+#include "mdstypes.h"
+
+class BatchOp {
+public:
+  virtual ~BatchOp() {}
+
+  virtual void add_request(const ceph::ref_t<class MDRequestImpl>& mdr) = 0;
+  virtual void set_request(const ceph::ref_t<class MDRequestImpl>& mdr) = 0;
+
+  virtual void print(std::ostream&) = 0;
+
+  void forward(mds_rank_t target);
+  void respond(int r);
+
+protected:
+  virtual void _forward(mds_rank_t) = 0;
+  virtual void _respond(mds_rank_t) = 0;
+};
+
+#endif

diff --git a/src/mds/CDentry.h b/src/mds/CDentry.h
index f8e408ef4689473306cce654c337c0354eceb0e5..02ac1ab3179fc65cb0a32e4722026e38b24e85a3 100644 (file)
--- a/src/mds/CDentry.h
+++ b/src/mds/CDentry.h
@@ -28,6 +28,7 @@
 #include "include/elist.h"
 #include "include/filepath.h"
 
+#include "BatchOp.h"
 #include "MDSCacheObject.h"
 #include "MDSContext.h"
 #include "SimpleLock.h"
@@ -41,7 +42,6 @@ class CDentry;
 class LogSegment;
 
 class Session;
-class BatchOp;
 
 // define an ordering
 bool operator<(const CDentry& l, const CDentry& r);
@@ -353,7 +353,7 @@ public:
   LocalLock versionlock; // FIXME referenced containers not in mempool
 
   mempool::mds_co::map<client_t,ClientLease*> client_lease_map;
-  std::map<int, BatchOp*> batch_ops;
+  std::map<int, std::unique_ptr<BatchOp>> batch_ops;
 
 
 protected:

diff --git a/src/mds/CInode.h b/src/mds/CInode.h
index e8c4b8a1542856687f7538b6b6c6f9bf614f24ae..8793a4e81969debd235fb89b407d7a6a7e203120 100644 (file)
--- a/src/mds/CInode.h
+++ b/src/mds/CInode.h
@@ -31,6 +31,7 @@
 #include "MDSContext.h"
 #include "flock.h"
 
+#include "BatchOp.h"
 #include "CDentry.h"
 #include "SimpleLock.h"
 #include "ScatterLock.h"
@@ -58,26 +59,6 @@ struct cinode_lock_info_t {
   int wr_caps;
 };
 
-class BatchOp {
-public:
-  virtual void add_request(const MDRequestRef& mdr) = 0;
-  virtual void set_request(const MDRequestRef& mdr) = 0;
-  virtual void forward_all(mds_rank_t target) = 0;
-  virtual void respond_all(int r) = 0;
-
-  void finish(int r) {
-    respond_all(r);
-    delete this;
-  }
-
-  void forward_requests(mds_rank_t target) {
-    forward_all(target);
-    delete this;
-  }
-
-  virtual ~BatchOp() {}
-};
-
 /**
  * Base class for CInode, containing the backing store data and
  * serialization methods.  This exists so that we can read and
@@ -368,7 +349,7 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Counter<CIno
     ceph_assert(num_exporting_dirs == 0);
   }
 
-  std::map<int, BatchOp*> batch_ops;
+  std::map<int, std::unique_ptr<BatchOp>> batch_ops;
 
   std::string_view pin_name(int p) const override;
 

diff --git a/src/mds/CMakeLists.txt b/src/mds/CMakeLists.txt
index 025dbdd7f2b8253c29508ac6447a3ebdefd64e1f..ddb07af3761d5944ef583553e374660701c22474 100644 (file)
--- a/src/mds/CMakeLists.txt
+++ b/src/mds/CMakeLists.txt
@@ -1,4 +1,5 @@
 set(mds_srcs
+  BatchOp.cc
   Capability.cc
   MDSDaemon.cc
   MDSRank.cc

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index e703a1eeb6f94ecc2a527503f2484da6bcced7ff..e383e61d86120e48b9cc6d79de26fb116b2e6b2c 100644 (file)
--- a/src/mds/MDCache.cc
+++ b/src/mds/MDCache.cc
@@ -9313,7 +9313,6 @@ void MDCache::request_forward(MDRequestRef& mdr, mds_rank_t who, int port)
     dout(7) << "request_forward " << *mdr << " to mds." << who << " req "
             << *mdr->client_request << dendl;
     if (mdr->is_batch_head) {
-      BatchOp* bop = nullptr;
       int mask = mdr->client_request->head.args.getattr.mask;
 
       switch (mdr->client_request->get_op()) {
@@ -9323,7 +9322,8 @@ void MDCache::request_forward(MDRequestRef& mdr, mds_rank_t who, int port)
            if (in) {
              auto it = in->batch_ops.find(mask);
              if (it != in->batch_ops.end()) {
-               bop = it->second;
+                it->second->forward(who);
+                in->batch_ops.erase(it);
              }
            }
            break;
@@ -9334,7 +9334,8 @@ void MDCache::request_forward(MDRequestRef& mdr, mds_rank_t who, int port)
            if (dn) {
              auto it = dn->batch_ops.find(mask);
              if (it != dn->batch_ops.end()) {
-               bop = it->second;
+                it->second->forward(who);
+                dn->batch_ops.erase(it);
              }
            }
            break;
@@ -9342,11 +9343,6 @@ void MDCache::request_forward(MDRequestRef& mdr, mds_rank_t who, int port)
        default:
          ceph_abort();
       }
-      if (bop) {
-       dout(20) << __func__ << ": forward other batch ops(GETATTR/LOOKUP) to "
-         << who << ":" << port <<", too. " << *mdr << dendl;
-       bop->forward_requests(who);
-      }
     } else {
       mds->forward_message_mds(mdr->release_client_request(), who);
     }

diff --git a/src/mds/Server.cc b/src/mds/Server.cc
index 7055af0b9c65e1585dbc1012b4c9735465e79de9..cc0662c405b02a5fee6577ab1f3e2deb046f853e 100644 (file)
--- a/src/mds/Server.cc
+++ b/src/mds/Server.cc
@@ -80,18 +80,18 @@ class ServerContext : public MDSContext {
 class Batch_Getattr_Lookup : public BatchOp {
 protected:
   Server* server;
-  MDRequestRef mdr;
+  ceph::ref_t<MDRequestImpl> mdr;
   MDCache* mdcache;
   int res = 0;
 public:
-  Batch_Getattr_Lookup(Server* s, MDRequestRef& r, MDCache* mdc) : server(s), mdr(r), mdcache(mdc) {}
-  void add_request(const MDRequestRef& m) override {
+  Batch_Getattr_Lookup(Server* s, ceph::ref_t<MDRequestImpl> r, MDCache* mdc) : server(s), mdr(std::move(r)), mdcache(mdc) {}
+  void add_request(const ceph::ref_t<MDRequestImpl>& m) override {
     mdr->batch_reqs.push_back(m);
   }
-  void set_request(const MDRequestRef& m) override {
+  void set_request(const ceph::ref_t<MDRequestImpl>& m) override {
     mdr = m;
   }
-  void forward_all(mds_rank_t t) override {
+  void _forward(mds_rank_t t) override {
     mdcache->mds->forward_message_mds(mdr->release_client_request(), t);
     mdr->set_mds_stamp(ceph_clock_now());
     for (auto& m : mdr->batch_reqs) {
@@ -100,7 +100,7 @@ public:
     }
     mdr->batch_reqs.clear();
   }
-  void respond_all(int r) {
+  void _respond(int r) override {
     mdr->set_mds_stamp(ceph_clock_now());
     for (auto& m : mdr->batch_reqs) {
       if (!m->killed) {
@@ -112,6 +112,9 @@ public:
     mdr->batch_reqs.clear();
     server->reply_client_request(mdr, make_message<MClientReply>(*mdr->client_request, r));
   }
+  void print(std::ostream& o) {
+    o << "[batch front=" << *mdr << "]";
+  }
 };
 
 class ServerLogContext : public MDSLogContextBase {
@@ -1790,22 +1793,20 @@ void Server::respond_to_request(MDRequestRef& mdr, int r)
     if (mdr->is_batch_op() && mdr->is_batch_head) {
       int mask = mdr->client_request->head.args.getattr.mask;
 
-      BatchOp *fin;
+      std::unique_ptr<BatchOp> bop;
       if (mdr->client_request->get_op() == CEPH_MDS_OP_GETATTR) {
        dout(20) << __func__ << ": respond other getattr ops. " << *mdr << dendl;
-       fin = mdr->in[0]->batch_ops[mask];
+        auto it = mdr->in[0]->batch_ops.find(mask);
+        bop = std::move(it->second);
+       mdr->in[0]->batch_ops.erase(it);
       } else {
        dout(20) << __func__ << ": respond other lookup ops. " << *mdr << dendl;
-       fin = mdr->dn[0].back()->batch_ops[mask];
+       auto it = mdr->dn[0].back()->batch_ops.find(mask);
+        bop = std::move(it->second);
+       mdr->dn[0].back()->batch_ops.erase(it);
       }
 
-      fin->finish(r);
-
-      if (mdr->client_request->get_op() == CEPH_MDS_OP_GETATTR) {
-       mdr->in[0]->batch_ops.erase(mask);
-      } else {
-       mdr->dn[0].back()->batch_ops.erase(mask);
-      }
+      bop->respond(r);
     } else {
      reply_client_request(mdr, make_message<MClientReply>(*mdr->client_request, r));
     }
@@ -2329,15 +2330,9 @@ void Server::clear_batch_ops(const MDRequestRef& mdr)
 {
   int mask = mdr->client_request->head.args.getattr.mask;
   if (mdr->client_request->get_op() == CEPH_MDS_OP_GETATTR && mdr->in[0]) {
-    auto it = mdr->in[0]->batch_ops.find(mask);
-    auto bocp = it->second;
-    mdr->in[0]->batch_ops.erase(it);
-    delete bocp;
+    mdr->in[0]->batch_ops.erase(mask);
   } else if (mdr->client_request->get_op() == CEPH_MDS_OP_LOOKUP && mdr->dn[0].size()) {
-    auto it = mdr->dn[0].back()->batch_ops.find(mask);
-    auto bocp = it->second;
-    mdr->dn[0].back()->batch_ops.erase(it);
-    delete bocp;
+    mdr->dn[0].back()->batch_ops.erase(mask);
   }
 }
 
@@ -2373,10 +2368,10 @@ void Server::dispatch_client_request(MDRequestRef& mdr)
        mdr->is_batch_head = true;
        int mask = mdr->client_request->head.args.getattr.mask;
        if (mdr->client_request->get_op() == CEPH_MDS_OP_GETATTR) {
-         auto fin = mdr->in[0]->batch_ops[mask];
+         auto& fin = mdr->in[0]->batch_ops[mask];
          fin->set_request(new_batch_head);
        } else if (mdr->client_request->get_op() == CEPH_MDS_OP_LOOKUP) {
-         auto fin = mdr->dn[0].back()->batch_ops[mask];
+         auto& fin = mdr->dn[0].back()->batch_ops[mask];
          fin->set_request(new_batch_head);
        }
       } else {
@@ -3660,20 +3655,24 @@ void Server::handle_client_getattr(MDRequestRef& mdr, bool is_lookup)
 
   if (!mdr->is_batch_head && mdr->is_batch_op()) {
     if (!is_lookup) {
-      if (ref->batch_ops.count(mask)) {
+      auto em = ref->batch_ops.emplace(std::piecewise_construct, std::forward_as_tuple(mask), std::forward_as_tuple());
+      if (em.second) {
+        em.first->second = std::make_unique<Batch_Getattr_Lookup>(this, mdr, mdcache);
+      } else {
        dout(20) << __func__ << ": GETATTR op, wait for previous same getattr ops to respond. " << *mdr << dendl;
-       ref->batch_ops[mask]->add_request(mdr);
+       em.first->second->add_request(mdr);
        return;
-      } else
-       ref->batch_ops[mask] = new Batch_Getattr_Lookup(this, mdr, mdcache);
+      }
     } else {
       CDentry* dn = mdr->dn[0].back();
-      if (dn->batch_ops.count(mask)) {
+      auto em = dn->batch_ops.emplace(std::piecewise_construct, std::forward_as_tuple(mask), std::forward_as_tuple());
+      if (em.second) {
+        em.first->second = std::make_unique<Batch_Getattr_Lookup>(this, mdr, mdcache);
+      } else {
        dout(20) << __func__ << ": LOOKUP op, wait for previous same getattr ops to respond. " << *mdr << dendl;
-       dn->batch_ops[mask]->add_request(mdr);
+       em.first->second->add_request(mdr);
        return;
-      } else
-       dn->batch_ops[mask] = new Batch_Getattr_Lookup(this, mdr, mdcache);
+      }
     }
   }
   mdr->is_batch_head = true;