mds: hold slave request refernce when dumping MDRequestImpl

dump_ops_in_flight asok command dumps MDRequestImpl without holding
mds_lock. MDS may free MDRequestImpl::slave_request in the middle of
dumping MDRequestImpl.

Fixes: http://tracker.ceph.com/issues/26894
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index ff06e7a614809add834e57b2a48d7637ae0589db..05e837df4d4a55d7fdb122903f454fec583613ed 100644 (file)
--- a/src/mds/MDCache.cc
+++ b/src/mds/MDCache.cc
@@ -2948,9 +2948,7 @@ void MDCache::handle_mds_failure(mds_rank_t who)
        if (!mdr->more()->waiting_on_slave.empty()) {
          assert(mdr->more()->srcdn_auth_mds == mds->get_nodeid());
          // will rollback, no need to wait
-         if (mdr->slave_request) {
-           mdr->slave_request = 0;
-         }
+         mdr->reset_slave_request();
          mdr->more()->waiting_on_slave.clear();
        }
       } else if (!mdr->committing) {
@@ -9210,8 +9208,7 @@ void MDCache::request_forward(MDRequestRef& mdr, mds_rank_t who, int port)
   if (mdr->client_request && mdr->client_request->get_source().is_client()) {
     dout(7) << "request_forward " << *mdr << " to mds." << who << " req "
             << *mdr->client_request << dendl;
-    mds->forward_message_mds(mdr->client_request, who);
-    mdr->client_request = 0;
+    mds->forward_message_mds(mdr->release_client_request(), who);
     if (mds->logger) mds->logger->inc(l_mds_forward);
   } else if (mdr->internal_op >= 0) {
     dout(10) << "request_forward on internal op; cancelling" << dendl;

diff --git a/src/mds/Mutation.cc b/src/mds/Mutation.cc
index 0e84ebc3e95dcbfe03bef4a5c4850966c42928be..9cc831137902595712b1791c310ae76634d09b19 100644 (file)
--- a/src/mds/Mutation.cc
+++ b/src/mds/Mutation.cc
@@ -310,6 +310,25 @@ bool MDRequestImpl::is_queued_for_replay() const
   return client_request ? client_request->is_queued_for_replay() : false;
 }
 
+MClientRequest::const_ref MDRequestImpl::release_client_request()
+{
+  msg_lock.lock();
+  MClientRequest::const_ref req;
+  req.swap(client_request);
+  msg_lock.unlock();
+  return req;
+}
+
+void MDRequestImpl::reset_slave_request(const MMDSSlaveRequest::const_ref& req)
+{
+  msg_lock.lock();
+  MMDSSlaveRequest::const_ref old;
+  old.swap(slave_request);
+  slave_request = req;
+  msg_lock.unlock();
+  old.reset();
+}
+
 void MDRequestImpl::print(ostream &out) const
 {
   out << "request(" << reqid;
@@ -330,31 +349,36 @@ void MDRequestImpl::_dump(Formatter *f) const
   f->dump_string("flag_point", state_string());
   f->dump_stream("reqid") << reqid;
   {
-    if (client_request) {
+    msg_lock.lock();
+    auto _client_request = client_request;
+    auto _slave_request =slave_request;
+    msg_lock.unlock();
+
+    if (_client_request) {
       f->dump_string("op_type", "client_request");
       f->open_object_section("client_info");
-      f->dump_stream("client") << client_request->get_orig_source();
-      f->dump_int("tid", client_request->get_tid());
+      f->dump_stream("client") << _client_request->get_orig_source();
+      f->dump_int("tid", _client_request->get_tid());
       f->close_section(); // client_info
-    } else if (is_slave() && slave_request) { // replies go to an existing mdr
+    } else if (is_slave() && _slave_request) { // replies go to an existing mdr
       f->dump_string("op_type", "slave_request");
       f->open_object_section("master_info");
-      f->dump_stream("master") << slave_request->get_orig_source();
+      f->dump_stream("master") << _slave_request->get_orig_source();
       f->close_section(); // master_info
 
       f->open_object_section("request_info");
-      f->dump_int("attempt", slave_request->get_attempt());
+      f->dump_int("attempt", _slave_request->get_attempt());
       f->dump_string("op_type",
-                     slave_request->get_opname(slave_request->get_op()));
-      f->dump_int("lock_type", slave_request->get_lock_type());
-      f->dump_stream("object_info") << slave_request->get_object_info();
-      f->dump_stream("srcdnpath") << slave_request->srcdnpath;
-      f->dump_stream("destdnpath") << slave_request->destdnpath;
-      f->dump_stream("witnesses") << slave_request->witnesses;
+         MMDSSlaveRequest::get_opname(_slave_request->get_op()));
+      f->dump_int("lock_type", _slave_request->get_lock_type());
+      f->dump_stream("object_info") << _slave_request->get_object_info();
+      f->dump_stream("srcdnpath") << _slave_request->srcdnpath;
+      f->dump_stream("destdnpath") << _slave_request->destdnpath;
+      f->dump_stream("witnesses") << _slave_request->witnesses;
       f->dump_bool("has_inode_export",
-                   slave_request->inode_export.length() != 0);
-      f->dump_int("inode_export_v", slave_request->inode_export_v);
-      f->dump_stream("op_stamp") << slave_request->op_stamp;
+         _slave_request->inode_export_v != 0);
+      f->dump_int("inode_export_v", _slave_request->inode_export_v);
+      f->dump_stream("op_stamp") << _slave_request->op_stamp;
       f->close_section(); // request_info
     }
     else if (internal_op != -1) { // internal request
@@ -378,10 +402,15 @@ void MDRequestImpl::_dump(Formatter *f) const
 
 void MDRequestImpl::_dump_op_descriptor_unlocked(ostream& stream) const
 {
-  if (client_request) {
-    client_request->print(stream);
-  } else if (slave_request) {
-    slave_request->print(stream);
+  msg_lock.lock();
+  auto _client_request = client_request;
+  auto _slave_request = slave_request;
+  msg_lock.unlock();
+
+  if (_client_request) {
+    _client_request->print(stream);
+  } else if (_slave_request) {
+    _slave_request->print(stream);
   } else if (internal_op >= 0) {
     stream << "internal op " << ceph_mds_op_name(internal_op) << ":" << reqid;
   } else {

diff --git a/src/mds/Mutation.h b/src/mds/Mutation.h
index c69af3d1891ec3cc04ad60d57fcc3c7bba9599bc..00a99285ead7a2a46ab6967b7625140173237bcc 100644 (file)
--- a/src/mds/Mutation.h
+++ b/src/mds/Mutation.h
@@ -347,11 +347,16 @@ struct MDRequestImpl : public MutationImpl {
   void print(ostream &out) const override;
   void dump(Formatter *f) const override;
 
+  MClientRequest::const_ref release_client_request();
+  void reset_slave_request(const MMDSSlaveRequest::const_ref& req=nullptr);
+
   // TrackedOp stuff
   typedef boost::intrusive_ptr<MDRequestImpl> Ref;
 protected:
   void _dump(Formatter *f) const override;
   void _dump_op_descriptor_unlocked(ostream& stream) const override;
+private:
+  mutable ceph::spinlock msg_lock;
 };
 
 typedef boost::intrusive_ptr<MDRequestImpl> MDRequestRef;

diff --git a/src/mds/Server.cc b/src/mds/Server.cc
index f1077a78ace9bfc729e0617c2c84b8a2522c82e9..5197e2320844d6689f971c5169f10ee9521ebe25 100644 (file)
--- a/src/mds/Server.cc
+++ b/src/mds/Server.cc
@@ -2105,7 +2105,7 @@ void Server::handle_slave_request(const MMDSSlaveRequest::const_ref &m)
     return;
   }
 
-  mdr->slave_request = m;
+  mdr->reset_slave_request(m);
   
   dispatch_slave_request(mdr);
 }
@@ -2263,7 +2263,7 @@ void Server::dispatch_slave_request(MDRequestRef& mdr)
       }
 
       // done.
-      mdr->slave_request = 0;
+      mdr->reset_slave_request();
     }
     break;
 
@@ -2286,13 +2286,13 @@ void Server::dispatch_slave_request(MDRequestRef& mdr)
        mds->locker->issue_caps(static_cast<CInode*>(lock->get_parent()));
 
       // done.  no ack necessary.
-      mdr->slave_request = 0;
+      mdr->reset_slave_request();
     }
     break;
 
   case MMDSSlaveRequest::OP_DROPLOCKS:
     mds->locker->drop_locks(mdr.get());
-    mdr->slave_request = 0;
+    mdr->reset_slave_request();
     break;
 
   case MMDSSlaveRequest::OP_AUTHPIN:
@@ -2441,7 +2441,7 @@ void Server::handle_slave_auth_pin(MDRequestRef& mdr)
   mds->send_message_mds(reply, mdr->slave_to_mds);
   
   // clean up this request
-  mdr->slave_request = 0;
+  mdr->reset_slave_request();
   return;
 }
 
@@ -5925,7 +5925,7 @@ void Server::_logged_slave_link(MDRequestRef& mdr, CInode *targeti, bool adjust_
   mds->balancer->hit_inode(targeti, META_POP_IWR);
 
   // done.
-  mdr->slave_request = 0;
+  mdr->reset_slave_request();
 
   if (adjust_realm) {
     int op = CEPH_SNAP_OP_SPLIT;
@@ -6636,7 +6636,7 @@ void Server::_logged_slave_rmdir(MDRequestRef& mdr, CDentry *dn, CDentry *strayd
       mdcache->do_realm_invalidate_and_update_notify(in, CEPH_SNAP_OP_SPLIT, false);
 
   // done.
-  mdr->slave_request = 0;
+  mdr->reset_slave_request();
   mdr->straydn = 0;
 
   if (!mdr->aborted) {
@@ -8084,7 +8084,7 @@ void Server::handle_slave_rename_prep(MDRequestRef& mdr)
     auto reply = MMDSSlaveRequest::create(mdr->reqid, mdr->attempt, MMDSSlaveRequest::OP_RENAMEPREPACK);
     reply->mark_interrupted();
     mds->send_message_mds(reply, mdr->slave_to_mds);
-    mdr->slave_request = 0;
+    mdr->reset_slave_request();
     return;
   }
 
@@ -8217,7 +8217,7 @@ void Server::handle_slave_rename_prep(MDRequestRef& mdr)
       auto reply = MMDSSlaveRequest::create(mdr->reqid, mdr->attempt, MMDSSlaveRequest::OP_RENAMEPREPACK);
       reply->witnesses.swap(srcdnrep);
       mds->send_message_mds(reply, mdr->slave_to_mds);
-      mdr->slave_request = 0;
+      mdr->reset_slave_request();
       return;  
     }
     dout(10) << " witness list sufficient: includes all srcdn replicas" << dendl;
@@ -8373,7 +8373,7 @@ void Server::_logged_slave_rename(MDRequestRef& mdr,
     mds->balancer->hit_inode(destdnl->get_inode(), META_POP_IWR);
 
   // done.
-  mdr->slave_request = 0;
+  mdr->reset_slave_request();
   mdr->straydn = 0;
 
   if (reply) {