mds: fix check of underwater dentries

Underwater dentry is dentry that is dirty in our cache from journal
replay, but had already been flushed to disk before the mds failed.
To decide if an dentry is underwater, original code compares dirty
dentry's version to on-disk dirfrag's version. This method is racy
because CDir::log_mark_dirty() can increase dirfrag's version without
adding log event. After mds failover, version of dirfrag from journal
replay can be less than on-disk dirfrag's version. So newly dirtied
dentry can be equal to or less than the on-disk dirfrag's version.

The race can cause incorrect fragstat/rstat

Fixes: http://tracker.ceph.com/issues/23032
Signed-off-by: Yan, Zheng <zyan@redhat.com>
(cherry picked from commit 9d271696b7735ab5b7384cdd386d6ac3eaafe437)

Conflicts:
	src/mds/CDir.cc - conditional being replaced by "if (!dn)" is different
                          in luminous

diff --git a/src/mds/CDir.cc b/src/mds/CDir.cc
index 1157e07dfe1e1b5fdd6aff0c112494310d13c469..7de39a0850b7526077665ef83a14ee0d3e56fb19 100644 (file)
--- a/src/mds/CDir.cc
+++ b/src/mds/CDir.cc
@@ -1659,8 +1659,7 @@ CDentry *CDir::_load_dentry(
     bufferlist &bl,
     const int pos,
     const std::set<snapid_t> *snaps,
-    bool *force_dirty,
-    list<CInode*> *undef_inodes)
+    bool *force_dirty)
 {
   bufferlist::iterator q = bl.begin();
 
@@ -1712,10 +1711,16 @@ CDentry *CDir::_load_dentry(
     }
 
     if (dn) {
-      if (dn->get_linkage()->get_inode() == 0) {
-        dout(12) << "_fetched  had NEG dentry " << *dn << dendl;
-      } else {
-        dout(12) << "_fetched  had dentry " << *dn << dendl;
+      CDentry::linkage_t *dnl = dn->get_linkage();
+      dout(12) << "_fetched had " << (dnl->is_null() ? "NEG" : "") << " dentry " << *dn << dendl;
+      if (committed_version == 0 &&
+         dnl->is_remote() &&
+         dn->is_dirty() &&
+         ino == dnl->get_remote_ino() &&
+         d_type == dnl->get_remote_d_type()) {
+       // see comment below
+       dout(10) << "_fetched  had underwater dentry " << *dn << ", marking clean" << dendl;
+       dn->mark_clean();
       }
     } else {
       // (remote) link
@@ -1748,15 +1753,35 @@ CDentry *CDir::_load_dentry(
 
     bool undef_inode = false;
     if (dn) {
-      CInode *in = dn->get_linkage()->get_inode();
-      if (in) {
-        dout(12) << "_fetched  had dentry " << *dn << dendl;
-        if (in->state_test(CInode::STATE_REJOINUNDEF)) {
-          undef_inodes->push_back(in);
-          undef_inode = true;
-        }
-      } else
-        dout(12) << "_fetched  had NEG dentry " << *dn << dendl;
+      CDentry::linkage_t *dnl = dn->get_linkage();
+      dout(12) << "_fetched had " << (dnl->is_null() ? "NEG" : "") << " dentry " << *dn << dendl;
+
+      if (dnl->is_primary()) {
+       CInode *in = dnl->get_inode();
+       if (in->state_test(CInode::STATE_REJOINUNDEF)) {
+         undef_inode = true;
+       } else if (committed_version == 0 &&
+                  dn->is_dirty() &&
+                  inode_data.inode.ino == in->ino() &&
+                  inode_data.inode.version == in->get_version()) {
+         /* clean underwater item?
+          * Underwater item is something that is dirty in our cache from
+          * journal replay, but was previously flushed to disk before the
+          * mds failed.
+          *
+          * We only do this is committed_version == 0. that implies either
+          * - this is a fetch after from a clean/empty CDir is created
+          *   (and has no effect, since the dn won't exist); or
+          * - this is a fetch after _recovery_, which is what we're worried
+          *   about.  Items that are marked dirty from the journal should be
+          *   marked clean if they appear on disk.
+          */
+         dout(10) << "_fetched  had underwater dentry " << *dn << ", marking clean" << dendl;
+         dn->mark_clean();
+         dout(10) << "_fetched  had underwater inode " << *dnl->get_inode() << ", marking clean" << dendl;
+         in->mark_clean();
+       }
+      }
     }
 
     if (!dn || undef_inode) {
@@ -1918,7 +1943,7 @@ void CDir::_omap_fetched(bufferlist& hdrbl, map<string, bufferlist>& omap,
     try {
       dn = _load_dentry(
             p->first, dname, last, p->second, pos, snaps,
-            &force_dirty, &undef_inodes);
+            &force_dirty);
     } catch (const buffer::error &err) {
       cache->mds->clog->warn() << "Corrupt dentry '" << dname << "' in "
                                   "dir frag " << dirfrag() << ": "
@@ -1937,35 +1962,16 @@ void CDir::_omap_fetched(bufferlist& hdrbl, map<string, bufferlist>& omap,
       continue;
     }
 
-    if (dn && (wanted_items.count(mempool::mds_co::string(boost::string_view(dname))) > 0 || !complete)) {
-      dout(10) << " touching wanted dn " << *dn << dendl;
-      inode->mdcache->touch_dentry(dn);
-    }
+    if (!dn)
+      continue;
 
-    /** clean underwater item?
-     * Underwater item is something that is dirty in our cache from
-     * journal replay, but was previously flushed to disk before the
-     * mds failed.
-     *
-     * We only do this is committed_version == 0. that implies either
-     * - this is a fetch after from a clean/empty CDir is created
-     *   (and has no effect, since the dn won't exist); or
-     * - this is a fetch after _recovery_, which is what we're worried 
-     *   about.  Items that are marked dirty from the journal should be
-     *   marked clean if they appear on disk.
-     */
-    if (committed_version == 0 &&     
-       dn &&
-       dn->get_version() <= got_fnode.version &&
-       dn->is_dirty()) {
-      dout(10) << "_fetched  had underwater dentry " << *dn << ", marking clean" << dendl;
-      dn->mark_clean();
+    CDentry::linkage_t *dnl = dn->get_linkage();
+    if (dnl->is_primary() && dnl->get_inode()->state_test(CInode::STATE_REJOINUNDEF))
+      undef_inodes.push_back(dnl->get_inode());
 
-      if (dn->get_linkage()->is_primary()) {
-       assert(dn->get_linkage()->get_inode()->get_version() <= got_fnode.version);
-       dout(10) << "_fetched  had underwater inode " << *dn->get_linkage()->get_inode() << ", marking clean" << dendl;
-       dn->get_linkage()->get_inode()->mark_clean();
-      }
+    if (!complete || wanted_items.count(mempool::mds_co::string(boost::string_view(dname))) > 0) {
+      dout(10) << " touching wanted dn " << *dn << dendl;
+      inode->mdcache->touch_dentry(dn);
     }
   }
 

diff --git a/src/mds/CDir.h b/src/mds/CDir.h
index 8de1867329df9aae00a736fc667b3ee69a241e2f..3e3208633852bd41220dbb2369ef3f781c4a2c6f 100644 (file)
--- a/src/mds/CDir.h
+++ b/src/mds/CDir.h
@@ -605,8 +605,7 @@ protected:
       bufferlist &bl,
       int pos,
       const std::set<snapid_t> *snaps,
-      bool *force_dirty,
-      std::list<CInode*> *undef_inodes);
+      bool *force_dirty);
 
   /**
    * Mark this fragment as BADFRAG (common part of go_bad and go_bad_dentry)