Merge pull request #44335 from SMIL-Infra/export-unlinked-dir

mds: fix crash when exporting unlinked dir

Reviewed-by: Venky Shankar <vshankar@redhat.com>
Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
Reviewed-by: Xiubo Li <xiubli@redhat.com>

qa/tasks/cephfs/mount.py

@@ -851,6 +851,31 @@ def open_background(self, basename="background_file", write=True):
 
         return rproc
 
+    def open_dir_background(self, basename):
+        """
+        Create and hold a capability to a directory.
+        """
+        assert(self.is_mounted())
+
+        path = os.path.join(self.hostfs_mntpt, basename)
+
+        pyscript = dedent("""
+            import time
+            import os
+
+            os.mkdir("{path}")
+            fd = os.open("{path}", os.O_RDONLY)
+            while True:
+                time.sleep(1)
+            """).format(path=path)
+
+        rproc = self._run_python(pyscript)
+        self.background_procs.append(rproc)
+
+        self.wait_for_visible(basename)
+
+        return rproc
+
     def wait_for_dir_empty(self, dirname, timeout=30):
         dirpath = os.path.join(self.hostfs_mntpt, dirname)
         with safe_while(sleep=5, tries=(timeout//5)) as proceed:

qa/tasks/cephfs/test_strays.py

@@ -602,7 +602,6 @@ def _force_migrate(self, path, rank=1):
         """
         :param to_id: MDS id to move it to
         :param path: Filesystem path (string) to move
-        :param watch_ino: Inode number to look for at destination to confirm move
         :return: None
         """
         self.mount_a.run_shell(["setfattr", "-n", "ceph.dir.pin", "-v", str(rank), path])
@@ -700,6 +699,46 @@ def test_migration_on_shutdown(self):
         # See that the stray counter on rank 0 has incremented
         self.assertEqual(self.get_mdc_stat("strays_created", rank_0_id), 1)
 
+    def test_migrate_unlinked_dir(self):
+        """
+        Reproduce https://tracker.ceph.com/issues/53597
+        """
+        rank_0_id, rank_1_id = self._setup_two_ranks()
+
+        self.mount_a.run_shell_payload("""
+mkdir pin
+touch pin/placeholder
+""")
+
+        self._force_migrate("pin")
+
+        # Hold the dir open so it cannot be purged
+        p = self.mount_a.open_dir_background("pin/to-be-unlinked")
+
+        # Unlink the dentry
+        self.mount_a.run_shell(["rmdir", "pin/to-be-unlinked"])
+
+        # Wait to see the stray count increment
+        self.wait_until_equal(
+            lambda: self.get_mdc_stat("num_strays", mds_id=rank_1_id),
+            expect_val=1, timeout=60, reject_fn=lambda x: x > 1)
+        # but not purged
+        self.assertEqual(self.get_mdc_stat("strays_created", mds_id=rank_1_id), 1)
+        self.assertEqual(self.get_mdc_stat("strays_enqueued", mds_id=rank_1_id), 0)
+
+        # Test loading unlinked dir into cache
+        self.fs.mds_asok(['flush', 'journal'], rank_1_id)
+        self.fs.mds_asok(['cache', 'drop'], rank_1_id)
+
+        # Shut down rank 1
+        self.fs.set_max_mds(1)
+        self.fs.wait_for_daemons(timeout=120)
+        # Now the stray should be migrated to rank 0
+        # self.assertEqual(self.get_mdc_stat("strays_created", mds_id=rank_0_id), 1)
+        # https://github.com/ceph/ceph/pull/44335#issuecomment-1125940158
+
+        self.mount_a.kill_background(p)
+
     def assert_backtrace(self, ino, expected_path):
         """
         Assert that the backtrace in the data pool for an inode matches

src/mds/CDir.cc

@@ -1475,6 +1475,20 @@ void CDir::mark_new(LogSegment *ls)
   mdcache->mds->queue_waiters(waiters);
 }
 
+void CDir::set_fresh_fnode(fnode_const_ptr&& ptr) {
+  ceph_assert(inode->is_auth());
+  ceph_assert(!is_projected());
+  ceph_assert(!state_test(STATE_COMMITTING));
+  reset_fnode(std::move(ptr));
+  projected_version = committing_version = committed_version = get_version();
+
+  if (state_test(STATE_REJOINUNDEF)) {
+    ceph_assert(mdcache->mds->is_rejoin());
+    state_clear(STATE_REJOINUNDEF);
+    mdcache->opened_undef_dirfrag(this);
+  }
+}
+
 void CDir::mark_clean()
 {
   dout(10) << __func__ << " " << *this << " version " << get_version() << dendl;
@@ -1547,16 +1561,9 @@ void CDir::fetch(std::string_view dname, snapid_t last,
       pdir && pdir->inode->is_stray() && !inode->snaprealm) {
     dout(7) << "fetch dirfrag for unlinked directory, mark complete" << dendl;
     if (get_version() == 0) {
-      ceph_assert(inode->is_auth());
       auto _fnode = allocate_fnode();
       _fnode->version = 1;
-      reset_fnode(std::move(_fnode));
-
-      if (state_test(STATE_REJOINUNDEF)) {
-	ceph_assert(mdcache->mds->is_rejoin());
-	state_clear(STATE_REJOINUNDEF);
-	mdcache->opened_undef_dirfrag(this);
-      }
+      set_fresh_fnode(std::move(_fnode));
     }
     mark_complete();
 
@@ -2043,17 +2050,7 @@ void CDir::_omap_fetched(bufferlist& hdrbl, map<string, bufferlist>& omap,
   // take the loaded fnode?
   // only if we are a fresh CDir* with no prior state.
   if (get_version() == 0) {
-    ceph_assert(!is_projected());
-    ceph_assert(!state_test(STATE_COMMITTING));
-    auto _fnode = allocate_fnode(got_fnode);
-    reset_fnode(std::move(_fnode));
-    projected_version = committing_version = committed_version = get_version();
-
-    if (state_test(STATE_REJOINUNDEF)) {
-      ceph_assert(mdcache->mds->is_rejoin());
-      state_clear(STATE_REJOINUNDEF);
-      mdcache->opened_undef_dirfrag(this);
-    }
+    set_fresh_fnode(allocate_fnode(got_fnode));
   }
 
   list<CInode*> undef_inodes;

src/mds/CDir.h

@@ -247,6 +247,7 @@ class CDir : public MDSCacheObject, public Counter<CDir> {
   void reset_fnode(fnode_const_ptr&& ptr) {
     fnode = std::move(ptr);
   }
+  void set_fresh_fnode(fnode_const_ptr&& ptr);
 
   const fnode_const_ptr& get_fnode() const {
     return fnode;