From d10fe17809c273dbb278eff02e1ea96005d03107 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Wed, 17 Jul 2019 12:30:38 -0700 Subject: [PATCH] mds: move BatchOp to separate translation unit To simplify logging and reduce coupling. Also a few other miscellaneous refactorings. Signed-off-by: Patrick Donnelly --- src/mds/BatchOp.cc | 35 ++++++++++++++++++++++ src/mds/BatchOp.h | 40 +++++++++++++++++++++++++ src/mds/CDentry.h | 4 +-- src/mds/CInode.h | 23 ++------------- src/mds/CMakeLists.txt | 1 + src/mds/MDCache.cc | 12 +++----- src/mds/Server.cc | 67 +++++++++++++++++++++--------------------- 7 files changed, 117 insertions(+), 65 deletions(-) create mode 100644 src/mds/BatchOp.cc create mode 100644 src/mds/BatchOp.h diff --git a/src/mds/BatchOp.cc b/src/mds/BatchOp.cc new file mode 100644 index 00000000000..c2152a69ba0 --- /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 index 00000000000..b54ad969969 --- /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& mdr) = 0; + virtual void set_request(const ceph::ref_t& 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 f8e408ef468..02ac1ab3179 100644 --- 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_lease_map; - std::map batch_ops; + std::map> batch_ops; protected: diff --git a/src/mds/CInode.h b/src/mds/CInode.h index e8c4b8a1542..8793a4e8196 100644 --- 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 batch_ops; + std::map> batch_ops; std::string_view pin_name(int p) const override; diff --git a/src/mds/CMakeLists.txt b/src/mds/CMakeLists.txt index 025dbdd7f2b..ddb07af3761 100644 --- 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 e703a1eeb6f..e383e61d861 100644 --- 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 7055af0b9c6..cc0662c405b 100644 --- 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 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 r, MDCache* mdc) : server(s), mdr(std::move(r)), mdcache(mdc) {} + void add_request(const ceph::ref_t& m) override { mdr->batch_reqs.push_back(m); } - void set_request(const MDRequestRef& m) override { + void set_request(const ceph::ref_t& 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(*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 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(*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(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(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; -- 2.39.5