squid: mds: command_quiesce_path: do not block the asok thread and return an adequate rc

Signed-off-by: Leonid Usov <leonid.usov@ibm.com>
(cherry picked from commit df546a4fba0d3851644ce1607340484409a3677d)
Fixes: https://tracker.ceph.com/issues/66258

diff --git a/qa/tasks/cephfs/test_quiesce.py b/qa/tasks/cephfs/test_quiesce.py
index b6ef8cde324bcbddcddf252326e983ee2934cbc1..c5b015d465a90812fbe54d0fda8821a6d9d7c171 100644 (file)
--- a/qa/tasks/cephfs/test_quiesce.py
+++ b/qa/tasks/cephfs/test_quiesce.py
@@ -470,7 +470,7 @@ class TestQuiesce(QuiesceTestCase):
         self.mount_a.run_shell_payload("ln -s ../.. subvol_quiesce")
         path = self.mount_a.cephfs_mntpt + "/subvol_quiesce"
 
-        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'])
+        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'], check_status=False)
         log.debug(f"{J}")
         self.assertEqual(J['op']['result'], -20) # ENOTDIR: the link is not a directory
 
@@ -484,7 +484,7 @@ class TestQuiesce(QuiesceTestCase):
         self.mount_a.run_shell_payload("ln -s ../../.. _nogroup")
         path = self.mount_a.cephfs_mntpt + "/_nogroup/" + self.QUIESCE_SUBVOLUME
 
-        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'])
+        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'], check_status=False)
         log.debug(f"{J}")
         self.assertEqual(J['op']['result'], -20) # ENOTDIR: path_traverse: the intermediate link is not a directory
 
@@ -498,7 +498,7 @@ class TestQuiesce(QuiesceTestCase):
         self.mount_a.run_shell_payload("mkdir dir")
         path = self.mount_a.cephfs_mntpt + "/dir"
 
-        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'])
+        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'], check_status=False)
         reqid = self._reqid_tostr(J['op']['reqid'])
         self._wait_for_quiesce_complete(reqid, path=path)
         self._verify_quiesce(root=path)
@@ -513,7 +513,7 @@ class TestQuiesce(QuiesceTestCase):
         self.mount_a.run_shell_payload("touch file")
         path = self.mount_a.cephfs_mntpt + "/file"
 
-        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'])
+        J = self.fs.rank_tell(["quiesce", "path", path, '--wait'], check_status=False)
         log.debug(f"{J}")
         self.assertEqual(J['op']['result'], -20) # ENOTDIR
 
@@ -525,9 +525,9 @@ class TestQuiesce(QuiesceTestCase):
 
         self._configure_subvolume()
 
-        op1 = self.fs.rank_tell(["quiesce", "path", self.subvolume])['op']
+        op1 = self.fs.rank_tell(["quiesce", "path", self.subvolume], check_status=False)['op']
         op1_reqid = self._reqid_tostr(op1['reqid'])
-        op2 = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'])['op']
+        op2 = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'], check_status=False)['op']
         op1 = self.fs.get_op(op1_reqid)['type_data'] # for possible dup result
         log.debug(f"op1 = {op1}")
         log.debug(f"op2 = {op2}")
@@ -545,7 +545,7 @@ class TestQuiesce(QuiesceTestCase):
         self.mount_a.run_shell_payload("touch file")
         self.mount_a.setfattr("file", "ceph.quiesce.block", "1")
 
-        J = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'])
+        J = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'], check_status=False)
         log.debug(f"{J}")
         self.assertEqual(J['op']['result'], 0)
         self.assertEqual(J['state']['inodes_blocked'], 1)
@@ -560,7 +560,7 @@ class TestQuiesce(QuiesceTestCase):
         self._configure_subvolume()
         self._client_background_workload()
 
-        J = self.fs.rank_tell(["quiesce", "path", self.subvolume])
+        J = self.fs.rank_tell(["quiesce", "path", self.subvolume], check_status=False)
         log.debug(f"{J}")
         reqid = self._reqid_tostr(J['op']['reqid'])
         self._wait_for_quiesce_complete(reqid)
@@ -582,7 +582,7 @@ class TestQuiesce(QuiesceTestCase):
         # drop cache
         self.fs.rank_tell(["cache", "drop"])
 
-        J = self.fs.rank_tell(["quiesce", "path", self.subvolume])
+        J = self.fs.rank_tell(["quiesce", "path", self.subvolume], check_status=False)
         log.debug(f"{J}")
         reqid = self._reqid_tostr(J['op']['reqid'])
         self._wait_for_quiesce_complete(reqid)
@@ -684,7 +684,7 @@ class TestQuiesceMultiRank(QuiesceTestCase):
         self._client_background_workload()
         self._wait_distributed_subtrees(2*2, rank="all", path=self.mntpnt)
 
-        op = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'], rank=0)['op']
+        op = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'], rank=0, check_status=False)['op']
         self.assertEqual(op['result'], -1) # EPERM
 
     @unittest.skip("https://tracker.ceph.com/issues/66152")
@@ -934,8 +934,8 @@ class TestQuiesceSplitAuth(QuiesceTestCase):
 
         sleep(2)
 
-        op0 = self.fs.rank_tell(["quiesce", "path", self.subvolume], rank=0)['op']
-        op1 = self.fs.rank_tell(["quiesce", "path", self.subvolume], rank=1)['op']
+        op0 = self.fs.rank_tell(["quiesce", "path", self.subvolume], rank=0, check_status=False)['op']
+        op1 = self.fs.rank_tell(["quiesce", "path", self.subvolume], rank=1, check_status=False)['op']
         reqid0 = self._reqid_tostr(op0['reqid'])
         reqid1 = self._reqid_tostr(op1['reqid'])
         op0 = self._wait_for_quiesce_complete(reqid0, rank=0, timeout=300)

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index 66a027e77c791c67933a1f763c238933c3e9d326..772a0e9fd589b61ff8e4e83ff63f06eb02a3631e 100644 (file)
--- a/src/mds/MDCache.cc
+++ b/src/mds/MDCache.cc
@@ -13777,6 +13777,7 @@ void MDCache::dispatch_quiesce_inode(const MDRequestRef& mdr)
   qs.inc_inodes_quiesced();
   auto* c = mdr->internal_op_finish;
   mdr->internal_op_finish = nullptr; // prevent ::request_kill recursion
+  mdr->result = 0;
   c->complete(0);
 
   /* do not respond/complete so locks are not lost, parent request will complete */
@@ -13876,6 +13877,7 @@ void MDCache::dispatch_quiesce_path(const MDRequestRef& mdr)
     dout(5) << __func__ << ": skipping recursive quiesce of path for non-auth inode" << dendl;
     mdr->mark_event("quiesce complete for non-auth tree");
   } else if (auto& qops = mdr->more()->quiesce_ops; qops.count(dirino) == 0) {
+    mdr->mark_event("quiescing root");
     MDRequestRef qimdr = request_start_internal(CEPH_MDS_OP_QUIESCE_INODE);
     qimdr->set_filepath(filepath(dirino));
     qimdr->internal_op_finish = new C_MDS_RetryRequest(this, mdr);
@@ -13895,12 +13897,12 @@ void MDCache::dispatch_quiesce_path(const MDRequestRef& mdr)
     mdr->mark_event("quiesce complete");
   }
 
+  mdr->result = 0;
   if (qfinisher) {
     auto* c = mdr->internal_op_finish;
     mdr->internal_op_finish = nullptr; // prevent ::request_kill recursion
     c->complete(0);
   }
-  mdr->result = 0;
 
   /* caller kills this op */
 }

diff --git a/src/mds/MDSRank.cc b/src/mds/MDSRank.cc
index 73e48889b584ed08dc7fac4f817d8d04c6cf9f2c..ebac99014d6894451bd79218fce3c39df2efe8f0 100644 (file)
--- a/src/mds/MDSRank.cc
+++ b/src/mds/MDSRank.cc
@@ -3019,7 +3019,8 @@ void MDSRankDispatcher::handle_asok_command(
     std::lock_guard l(mds_lock);
     mdcache->cache_status(f);
   } else if (command == "quiesce path") {
-    r = command_quiesce_path(f, cmdmap, *css);
+    command_quiesce_path(f, cmdmap, std::move(on_finish));
+    return;
   } else if (command == "lock path") {
     command_lock_path(f, cmdmap, std::move(on_finish));
     return;
@@ -3464,51 +3465,55 @@ void MDSRank::command_openfiles_ls(Formatter *f)
 
 class C_MDS_QuiescePathCommand : public MDCache::C_MDS_QuiescePath {
 public:
-  C_MDS_QuiescePathCommand(MDCache* cache, Context* fin) : C_MDS_QuiescePath(cache), finisher(fin) {}
+  C_MDS_QuiescePathCommand(MDCache* cache) : C_MDS_QuiescePath(cache) {}
   void finish(int rc) override {
-    if (finisher) {
-      finisher->complete(rc);
-      finisher = nullptr;
+    if (auto fin = std::move(finish_once)) {
+      fin(rc, *this);
     }
   }
-private:
-  Context* finisher = nullptr;
+  std::function<void(int, C_MDS_QuiescePathCommand const&)> finish_once;
 };
 
-int MDSRank::command_quiesce_path(Formatter* f, const cmdmap_t& cmdmap, std::ostream& ss)
+void MDSRank::command_quiesce_path(Formatter* f, const cmdmap_t& cmdmap, std::function<void(int, const std::string&, bufferlist&)> on_finish)
 {
   std::string path;
-  {
-    bool got = cmd_getval(cmdmap, "path", path);
-    if (!got) {
-      ss << "missing path";
-      return -CEPHFS_EINVAL;
-    }
+  if (!cmd_getval(cmdmap, "path", path)) {
+    bufferlist bl;
+    on_finish(-EINVAL, "missing path", bl);
+    return;
   }
 
-  bool wait = false;
-  cmd_getval(cmdmap, "wait", wait);
+  bool wait = cmd_getval_or<bool>(cmdmap, "wait", false);
 
   C_SaferCond cond;
-  auto* finisher = new C_MDS_QuiescePathCommand(mdcache, wait ? &cond : nullptr);
-  auto qs = finisher->qs;
-  MDRequestRef mdr;
-  f->open_object_section("quiesce");
-  {
-    std::lock_guard l(mds_lock);
-    mdr = mdcache->quiesce_path(filepath(path), finisher, f);
-    if (!wait) {
-      f->dump_object("op", *mdr);
-    }
-  }
-  if (wait) {
-    cond.wait();
-    std::lock_guard l(mds_lock);
-    f->dump_object("op", *mdr);
+  auto* quiesce_ctx = new C_MDS_QuiescePathCommand(mdcache);
+
+  quiesce_ctx->finish_once = [f, respond = std::move(on_finish)](int cephrc, C_MDS_QuiescePathCommand const& cmd) {
+    f->open_object_section("response");
+    f->dump_object("op", *cmd.mdr);
+    f->dump_object("state", *cmd.qs);
+    f->close_section();
+
+    bufferlist bl;
+    // need to do this manually, because the default asok
+    // on_finish handler doesn't flush the formatter for rc < 0
+    f->flush(bl);
+    auto rc = cephrc < 0 ? -ceph_to_hostos_errno(-cephrc) : cephrc;
+    respond(rc, "", bl);
+  };
+
+  std::lock_guard l(mds_lock);
+
+  auto mdr = mdcache->quiesce_path(filepath(path), quiesce_ctx, f);
+
+  // This is a little ugly, apologies.
+  // We should still be under the mds lock for this test to be valid.
+  // MDCache will delete the quiesce_ctx if it manages to complete syncrhonously,
+  // so we are testing the `mdr->internal_op_finish` to see if that has happend
+  if (!wait && mdr && mdr->internal_op_finish) {
+    ceph_assert(mdr->internal_op_finish == quiesce_ctx);
+    quiesce_ctx->finish(mdr->result.value_or(0));
   }
-  f->dump_object("state", *qs);
-  f->close_section();
-  return 0;
 }
 
 void MDSRank::command_lock_path(Formatter* f, const cmdmap_t& cmdmap, std::function<void(int, const std::string&, bufferlist&)> on_finish)

diff --git a/src/mds/MDSRank.h b/src/mds/MDSRank.h
index 128050ebe3493014ee88cbcb796e484dafdec65f..3dfda80ee690720cde3b0fef22fb42d6c5ca78bf 100644 (file)
--- a/src/mds/MDSRank.h
+++ b/src/mds/MDSRank.h
@@ -526,7 +526,7 @@ class MDSRank {
         std::ostream &ss);
     void command_openfiles_ls(Formatter *f);
     void command_dump_tree(const cmdmap_t &cmdmap, std::ostream &ss, Formatter *f);
-    int command_quiesce_path(Formatter *f, const cmdmap_t &cmdmap, std::ostream &ss);
+    void command_quiesce_path(Formatter *f, const cmdmap_t &cmdmap, std::function<void(int, const std::string&, bufferlist&)> on_finish);
     void command_lock_path(Formatter* f, const cmdmap_t& cmdmap, std::function<void(int, const std::string&, bufferlist&)> on_finish);
     void command_dump_inode(Formatter *f, const cmdmap_t &cmdmap, std::ostream &ss);
     void command_dump_dir(Formatter *f, const cmdmap_t &cmdmap, std::ostream &ss);