Merge PR ceph#57332 into main

* refs/pull/57332/head:
	mds/quiesce: drop remote authpins before waiting for the quiesce lock
	qa/cephfs/test_quiesce: test proper handling of remote authpins
	mds: don't clear `AUTHPIN_FROZEN` until `FROZEN` in rename_prep
	mds: enhance the `lock path` asok command
	mds/quiesce: overdrive fragmenting that's still freezing
	revert: mds: provide a mechanism to authpin while freezing
	qa/cephfs/test_quiesce: enhance the fragmentation test

Reviewed-by: Patrick Donnelly <[email protected]>

Author: batrick

qa/tasks/cephfs/test_quiesce.py

@@ -331,6 +331,14 @@ def _verify_quiesce_wrapped(self, rank, status, root, root_inode, ops, cache, sp
         # check request/cap count is stopped
         # count inodes under /usr and count subops!
 
+    def quiesce_and_verify(self, path, timeout=120):
+        J = self.fs.rank_tell("quiesce", "path", path)
+        log.debug(f"{J}")
+        reqid = self._reqid_tostr(J['op']['reqid'])
+        self._wait_for_quiesce_complete(reqid, timeout=timeout)
+        self._verify_quiesce(root=path)
+        return reqid
+
 class TestQuiesce(QuiesceTestCase):
     """
     Single rank functional tests.
@@ -598,21 +606,39 @@ def test_quiesce_dir_fragment(self):
         That quiesce completes with fragmentation in the background.
         """
 
-        self.config_set('mds', 'mds_bal_split_size', '10')
-        self.config_set('mds', 'mds_bal_merge_size', '1') # do not merge
-        self.config_set('mds', 'mds_bal_split_bits', '1')
-        self._configure_subvolume()
-        self._client_background_workload()
-
-        # time for the workload to get busy
-        time.sleep(5)
+        # the config should cause continuous merge-split wars
+        self.config_set('mds', 'mds_bal_split_size', '1') # split anything larger than one item ....
+        self.config_set('mds', 'mds_bal_merge_size', '2') # and then merge if only one item ]:-}
+        self.config_set('mds', 'mds_bal_split_bits', '2')
 
-        J = self.fs.rank_tell("quiesce", "path", self.subvolume)
-        log.debug(f"{J}")
-        reqid = self._reqid_tostr(J['op']['reqid'])
-        self._wait_for_quiesce_complete(reqid)
-        self._verify_quiesce(root=self.subvolume)
+        self._configure_subvolume()
 
+        self.mount_a.run_shell_payload("mkdir -p root/sub1")
+        self.mount_a.write_file("root/sub1/file1", "I'm file 1")
+        self.mount_a.run_shell_payload("mkdir -p root/sub2")
+        self.mount_a.write_file("root/sub2/file2", "I'm file 2")
+        
+        sleep_for = 30
+        log.info(f"Sleeping {sleep_for} seconds to warm up the balancer")
+        time.sleep(sleep_for)
+
+        for _ in range(30):
+            sub1 = f"{self.subvolume}/root/sub1"
+            log.debug(f"Quiescing {sub1}")
+            # with one of the subdirs quiesced, the freezing
+            # of the parent dir (root) can't complete
+            op1 = self.quiesce_and_verify(sub1, timeout=15)
+
+            sub2 = f"{self.subvolume}/root/sub2"
+            log.debug(f"{sub1} quiesced: {op1}. Quiescing {sub2}")
+            # despite the parent dir freezing, we should be able
+            # to quiesce the other subvolume
+            op2 = self.quiesce_and_verify(sub2, timeout=15)
+
+            log.debug(f"{sub2} quiesced: {op2}. Killing the ops.")
+            self.fs.kill_op(op1)
+            self.fs.kill_op(op2)
+            time.sleep(5)
 
 class TestQuiesceMultiRank(QuiesceTestCase):
     """
@@ -661,6 +687,50 @@ def test_quiesce_path_splitauth(self):
         op = self.fs.rank_tell(["quiesce", "path", self.subvolume, '--wait'], rank=0)['op']
         self.assertEqual(op['result'], -1) # EPERM
 
+    @unittest.skip("https://tracker.ceph.com/issues/66152")
+    def test_quiesce_drops_remote_authpins_on_failure(self):
+        """
+        That remote authpins are dropped when the request fails to acquire the quiesce lock
+
+        When the remote authpin is freezing, not dropping it is likely to deadlock a distributed quiesce
+        """
+        self._configure_subvolume()
+
+        # create two dirs for pinning
+        self.mount_a.run_shell_payload("mkdir -p pin0 pin1")
+        # enable export by populating the directories
+        self.mount_a.run_shell_payload("touch pin0/export_dummy pin1/export_dummy")
+        # pin the files to different ranks
+        self.mount_a.setfattr("pin0", "ceph.dir.pin", "0")
+        self.mount_a.setfattr("pin1", "ceph.dir.pin", "1")
+
+        # prepare the patient at rank 0
+        self.mount_a.write_file("pin0/thefile", "I'm ready, doc")
+
+        # wait for the export to settle
+        self._wait_subtrees([(f"{self.mntpnt}/pin0", 0), (f"{self.mntpnt}/pin1", 1)])
+
+        # Take the quiesce lock on the replica of the src file.
+        # This is needed to cause the next command to block
+        cmd = self.fs.run_ceph_cmd(f"tell mds.{self.fs.name}:1 lock path {self.mntpnt}/pin0/thefile quiesce:x --await")
+        self.assertEqual(cmd.exitstatus, 0)
+
+        # Simulate a rename by remote-auth-pin-freezing the file.
+        # Also try to take the quiesce lock to cause the MDR
+        # to block on quiesce with the remote authpin granted
+        # Don't --await this time because we expect this to block
+        cmd = self.fs.run_ceph_cmd(f"tell mds.{self.fs.name}:1 lock path {self.mntpnt}/pin0/thefile quiesce:w --ap-freeze")
+        self.assertEqual(cmd.exitstatus, 0)
+
+        # At this point, if everything works well, we should be able to
+        # autpin and quiesce the file on the auth side.
+        # If the op above fails to release remote authpins, then the inode
+        # will still be authpin frozen, and that will disallow auth pinning the file
+        # We are using a combination of --ap-dont-block and --await
+        # to detect whether the file is authpinnable
+        cmd = self.fs.run_ceph_cmd(f"tell mds.{self.fs.name}:0 lock path {self.mntpnt}/pin0/thefile quiesce:x --ap-dont-block --await")
+        self.assertEqual(cmd.exitstatus, 0)
+
     def test_quiesce_authpin_wait(self):
         """
         That a quiesce_inode op with outstanding remote authpin requests can be killed.
@@ -804,7 +874,7 @@ def test_quiesce_block_replicated(self):
         op = self.fs.rank_tell("lock", "path", self.mntpnt+"/dir1/dir2", "policy:r", rank=1)
         p = self.mount_a.setfattr("dir1/dir2", "ceph.quiesce.block", "1", wait=False)
         sleep(2) # for req to block waiting for xlock on policylock
-        reqid = self._reqid_tostr(op['op']['reqid'])
+        reqid = self._reqid_tostr(op['reqid'])
         self.fs.kill_op(reqid, rank=1)
         p.wait()
 

qa/tasks/vstart_runner.py

@@ -1375,7 +1375,7 @@ def exec_test():
         elif f == '--run-all-tests':
             opt_exit_on_test_failure = False
         elif f == '--debug':
-            log.setLevel(logging.DEBUG)
+            logging.root.setLevel(logging.DEBUG)
         elif f == '--config-mode':
             mode = Mode.config
         else:

src/common/TrackedOp.h

@@ -204,26 +204,27 @@ class OpTracker {
   }
   ~OpTracker();
 
-  template <typename T, typename U>
-  typename T::Ref create_request(U params)
+  // NB: P is ref-like, i.e. `params` should be dereferenced for members
+  template <typename R, typename P>
+  typename R::Ref create_request(P params)
   {
-    constexpr bool has_is_continuous = requires(U u) {
-      { u->is_continuous() } -> std::same_as<bool>;
+    constexpr bool enable_mark_continuous = requires(typename R::Ref r, P p) {
+      { p->is_continuous() } -> std::same_as<bool>;
+      r->mark_continuous();
     };
-    typename T::Ref retval(new T(params, this));
+    typename R::Ref retval(new R(params, this));
     retval->tracking_start();
     if (is_tracking()) {
       retval->mark_event("header_read", params->get_recv_stamp());
       retval->mark_event("throttled", params->get_throttle_stamp());
       retval->mark_event("all_read", params->get_recv_complete_stamp());
       retval->mark_event("dispatched", params->get_dispatch_stamp());
     }
-    if constexpr (has_is_continuous) {
+    if constexpr (enable_mark_continuous) {
       if (params->is_continuous()) {
         retval->mark_continuous();
       }
     }
-
     return retval;
   }
 };

src/mds/CDentry.cc

@@ -368,10 +368,10 @@ int CDentry::get_num_dir_auth_pins() const
   return auth_pins;
 }
 
-bool CDentry::can_auth_pin(int *err_ret, bool bypassfreezing) const
+bool CDentry::can_auth_pin(int *err_ret) const
 {
   ceph_assert(dir);
-  return dir->can_auth_pin(err_ret, bypassfreezing);
+  return dir->can_auth_pin(err_ret);
 }
 
 void CDentry::auth_pin(void *by)

src/mds/CDentry.h

@@ -222,7 +222,7 @@ class CDentry : public MDSCacheObject, public LRUObject, public Counter<CDentry>
   void _put() override;
 
   // auth pins
-  bool can_auth_pin(int *err_ret=nullptr, bool bypassfreezing=false) const override;
+  bool can_auth_pin(int *err_ret=nullptr) const override;
   void auth_pin(void *by) override;
   void auth_unpin(void *by) override;
   void adjust_nested_auth_pins(int diradj, void *by);

src/mds/CDir.cc

@@ -3450,23 +3450,16 @@ void CDir::adjust_freeze_after_rename(CDir *dir)
   mdcache->mds->queue_waiters(unfreeze_waiters);
 }
 
-bool CDir::can_auth_pin(int *err_ret, bool bypassfreezing) const
+bool CDir::can_auth_pin(int *err_ret) const
 {
   int err;
   if (!is_auth()) {
     err = ERR_NOT_AUTH;
-  } else if (is_freezing_dir()) {
-    if (bypassfreezing) {
-      dout(20) << "allowing authpin with freezing" << dendl;
-      err = 0;
-    } else {
-      err = ERR_FRAGMENTING_DIR;
-    }
-  } else if (is_frozen_dir()) {
+  } else if (is_freezing_dir() || is_frozen_dir()) {
     err = ERR_FRAGMENTING_DIR;
   } else {
     auto p = is_freezing_or_frozen_tree();
-    if (p.first && !bypassfreezing) {
+    if (p.first) {
       err = ERR_EXPORTING_TREE;
     } else if (p.second) {
       err = ERR_EXPORTING_TREE;

src/mds/CDir.h

@@ -527,7 +527,7 @@ class CDir : public MDSCacheObject, public Counter<CDir> {
   void abort_import();
 
   // -- auth pins --
-  bool can_auth_pin(int *err_ret=nullptr, bool bypassfreezing=false) const override;
+  bool can_auth_pin(int *err_ret=nullptr) const override;
   int get_auth_pins() const { return auth_pins; }
   int get_dir_auth_pins() const { return dir_auth_pins; }
   void auth_pin(void *who) override;

src/mds/CInode.cc

@@ -2970,22 +2970,15 @@ void CInode::clear_ambiguous_auth()
 }
 
 // auth_pins
-bool CInode::can_auth_pin(int *err_ret, bool bypassfreezing) const {
+bool CInode::can_auth_pin(int *err_ret) const {
   int err;
   if (!is_auth()) {
     err = ERR_NOT_AUTH;
-  } else if (is_freezing_inode()) {
-    if (bypassfreezing) {
-      dout(20) << "allowing authpin with freezing" << dendl;
-      err = 0;
-    } else {
-      err = ERR_EXPORTING_INODE;
-    }
-  } else if (is_frozen_inode() || is_frozen_auth_pin()) {
+  } else if (is_freezing_inode() || is_frozen_inode() || is_frozen_auth_pin()) {
     err = ERR_EXPORTING_INODE;
   } else {
     if (parent)
-      return parent->can_auth_pin(err_ret, bypassfreezing);
+      return parent->can_auth_pin(err_ret);
     err = 0;
   }
   if (err && err_ret)
@@ -5605,4 +5598,23 @@ void CInode::get_subtree_dirfrags(std::vector<CDir*>& v) const
   }
 }
 
+bool CInode::is_quiesced() const { 
+  if (!quiescelock.is_xlocked()) {
+    return false;
+  }
+  // check that it's the quiesce op that's holding the lock
+  auto mut = quiescelock.get_xlock_by();
+  ceph_assert(mut); /* that would be weird */
+  auto* mdr = dynamic_cast<MDRequestImpl*>(mut.get());
+  ceph_assert(mdr); /* also would be weird */
+  return mdr->internal_op == CEPH_MDS_OP_QUIESCE_INODE;
+}
+
+bool CInode::will_block_for_quiesce(const MDRequestRef& mdr) {
+  if (mdr && mdr->is_wrlocked(&quiescelock)) {
+    return false;
+  }
+  return !quiescelock.can_wrlock();
+}
+
 MEMPOOL_DEFINE_OBJECT_FACTORY(CInode, co_inode, mds_co);

src/mds/CInode.h

@@ -662,7 +662,8 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Counter<CIno
   bool is_file() const    { return get_inode()->is_file(); }
   bool is_symlink() const { return get_inode()->is_symlink(); }
   bool is_dir() const     { return get_inode()->is_dir(); }
-  bool is_quiesced() const { return quiescelock.is_xlocked(); }
+  bool is_quiesced() const;
+  bool will_block_for_quiesce(const MDRequestRef& mdr = MDRequestRef {});
 
   bool is_head() const { return last == CEPH_NOSNAP; }
 
@@ -930,7 +931,7 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Counter<CIno
   mds_authority_t authority() const override;
 
   // -- auth pins --
-  bool can_auth_pin(int *err_ret=nullptr, bool bypassfreezing=false) const override;
+  bool can_auth_pin(int *err_ret=nullptr) const override;
   void auth_pin(void *by) override;
   void auth_unpin(void *by) override;