mds: cleanup dirty snap caps tracking

For individual client, track dirty snap caps as whole. This fixes an
infinite loop in Locker::file_update_finish and closes a race case.

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

diff --git a/src/mds/CInode.h b/src/mds/CInode.h
index f2b23a486453701ec72db3890877406edf561f0f..d866b7622b25a2e6a24e4b70a47cf5c28e8b07db 100644 (file)
--- a/src/mds/CInode.h
+++ b/src/mds/CInode.h
@@ -934,7 +934,7 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Counter<CIno
   // list item node for when we have unpropagated rstat data
   elist<CInode*>::item dirty_rstat_item;
 
-  mempool::mds_co::compact_map<int, mempool::mds_co::set<client_t> > client_snap_caps;     // [auth] [snap] dirty metadata we still need from the head
+  mempool::mds_co::set<client_t> client_snap_caps;
   mempool::mds_co::compact_map<snapid_t, mempool::mds_co::set<client_t> > client_need_snapflush;
 
   // LogSegment lists i (may) belong to

diff --git a/src/mds/Locker.cc b/src/mds/Locker.cc
index 910b048a007b74ae7df1da776b1adb738f0ec827..7515b6a76fc184218fbac5dbfd82299d67f7655a 100644 (file)
--- a/src/mds/Locker.cc
+++ b/src/mds/Locker.cc
@@ -1863,30 +1863,15 @@ void Locker::file_update_finish(CInode *in, MutationRef& mut, unsigned flags,
   } else if ((flags & UPDATE_SNAPFLUSH) && !in->client_snap_caps.empty()) {
     dout(10) << " client_snap_caps " << in->client_snap_caps << dendl;
     // check for snap writeback completion
-    bool gather = false;
-    auto p = in->client_snap_caps.begin();
-    while (p != in->client_snap_caps.end()) {
-      auto q = p->second.find(client);
-      if (q != p->second.end()) {
-       SimpleLock *lock = in->get_lock(p->first);
+    in->client_snap_caps.erase(client);
+    if (in->client_snap_caps.empty()) {
+      for (int i = 0; i < num_cinode_locks; i++) {
+       SimpleLock *lock = in->get_lock(cinode_lock_info[i].lock);
        ceph_assert(lock);
-       dout(10) << " completing client_snap_caps for " << ccap_string(p->first)
-                << " lock " << *lock << " on " << *in << dendl;
        lock->put_wrlock();
-
-       p->second.erase(q);
-       if (p->second.empty()) {
-         gather = true;
-         in->client_snap_caps.erase(p++);
-       } else
-         ++p;
-      }
-    }
-    if (gather) {
-      if (in->client_snap_caps.empty()) {
-       in->item_open_file.remove_myself();
-       in->item_caps.remove_myself();
       }
+      in->item_open_file.remove_myself();
+      in->item_caps.remove_myself();
       eval_cap_gather(in, &need_issue);
     }
   }

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index 722bb6d065c57565556c7b51b585c107e7da2403..039ce2e6253f592cd0360e3bcf5a3172504825b3 100644 (file)
--- a/src/mds/MDCache.cc
+++ b/src/mds/MDCache.cc
@@ -1471,17 +1471,16 @@ CInode *MDCache::cow_inode(CInode *in, snapid_t last)
     auto ret = head_in->split_need_snapflush(oldin, in);
     if (ret.first) {
       oldin->client_snap_caps = in->client_snap_caps;
-      for (const auto &p : oldin->client_snap_caps) {
-       SimpleLock *lock = oldin->get_lock(p.first);
-       ceph_assert(lock);
-       for (const auto &q : p.second) {
+      if (!oldin->client_snap_caps.empty()) {
+       for (int i = 0; i < num_cinode_locks; i++) {
+         SimpleLock *lock = oldin->get_lock(cinode_lock_info[i].lock);
+         ceph_assert(lock);
          if (lock->get_state() != LOCK_SNAP_SYNC) {
            ceph_assert(lock->is_stable());
            lock->set_state(LOCK_SNAP_SYNC);  // gathering
            oldin->auth_pin(lock);
          }
          lock->get_wrlock(true);
-         (void)q; /* unused */
        }
       }
     }
@@ -1491,22 +1490,21 @@ CInode *MDCache::cow_inode(CInode *in, snapid_t last)
       in->item_open_file.remove_myself();
       in->item_caps.remove_myself();
 
-      MDSContext::vec finished;
-      for (const auto &p : client_snap_caps) {
-       SimpleLock *lock = in->get_lock(p.first);
-       ceph_assert(lock);
-       ceph_assert(lock->get_state() == LOCK_SNAP_SYNC); // gathering
-       for (const auto &q : p.second) {
+      if (!client_snap_caps.empty()) {
+       MDSContext::vec finished;
+       for (int i = 0; i < num_cinode_locks; i++) {
+         SimpleLock *lock = in->get_lock(cinode_lock_info[i].lock);
+         ceph_assert(lock);
+         ceph_assert(lock->get_state() == LOCK_SNAP_SYNC); // gathering
          lock->put_wrlock();
-         (void)q; /* unused */
-       }
-       if (!lock->get_num_wrlocks()) {
-         lock->set_state(LOCK_SYNC);
-         lock->take_waiting(SimpleLock::WAIT_STABLE|SimpleLock::WAIT_RD, finished);
-         in->auth_unpin(lock);
+         if (!lock->get_num_wrlocks()) {
+           lock->set_state(LOCK_SYNC);
+           lock->take_waiting(SimpleLock::WAIT_STABLE|SimpleLock::WAIT_RD, finished);
+           in->auth_unpin(lock);
+         }
        }
+       mds->queue_waiters(finished);
       }
-      mds->queue_waiters(finished);
     }
     return oldin;
   }
@@ -1520,23 +1518,8 @@ CInode *MDCache::cow_inode(CInode *in, snapid_t last)
       int issued = cap->need_snapflush() ? CEPH_CAP_ANY_WR : cap->issued();
       if ((issued & CEPH_CAP_ANY_WR) &&
          cap->client_follows < last) {
-       // note in oldin
-       for (int i = 0; i < num_cinode_locks; i++) {
-         if (issued & cinode_lock_info[i].wr_caps) {
-           int lockid = cinode_lock_info[i].lock;
-           SimpleLock *lock = oldin->get_lock(lockid);
-           ceph_assert(lock);
-           if (lock->get_state() != LOCK_SNAP_SYNC) {
-             ceph_assert(lock->is_stable());
-             lock->set_state(LOCK_SNAP_SYNC);  // gathering
-             oldin->auth_pin(lock);
-           }
-           lock->get_wrlock(true);
-           oldin->client_snap_caps[lockid].insert(client);
-           dout(10) << " client." << client << " cap " << ccap_string(issued & cinode_lock_info[i].wr_caps)
-                    << " wrlock lock " << *lock << " on " << *oldin << dendl;
-         }
-       }
+       dout(10) << " client." << client << " cap " << ccap_string(issued) << dendl;
+       oldin->client_snap_caps.insert(client);
        cap->client_follows = last;
 
        // we need snapflushes for any intervening snaps
@@ -1550,6 +1533,19 @@ CInode *MDCache::cow_inode(CInode *in, snapid_t last)
        dout(10) << " ignoring client." << client << " cap follows " << cap->client_follows << dendl;
       }
     }
+
+    if (!oldin->client_snap_caps.empty()) {
+      for (int i = 0; i < num_cinode_locks; i++) {
+       SimpleLock *lock = oldin->get_lock(cinode_lock_info[i].lock);
+       ceph_assert(lock);
+       if (lock->get_state() != LOCK_SNAP_SYNC) {
+         ceph_assert(lock->is_stable());
+         lock->set_state(LOCK_SNAP_SYNC);  // gathering
+         oldin->auth_pin(lock);
+       }
+       lock->get_wrlock(true);
+      }
+    }
   }
   return oldin;
 }
@@ -5487,17 +5483,17 @@ void MDCache::rebuild_need_snapflush(CInode *head_in, SnapRealm *realm,
 
     dout(10) << " need snapflush from client." << client << " on " << *in << dendl;
 
-    /* TODO: we can check the reconnected/flushing caps to find
-     *       which locks need gathering */
-    for (int i = 0; i < num_cinode_locks; i++) {
-      int lockid = cinode_lock_info[i].lock;
-      SimpleLock *lock = in->get_lock(lockid);
-      ceph_assert(lock);
-      in->client_snap_caps[lockid].insert(client);
-      in->auth_pin(lock);
-      lock->set_state(LOCK_SNAP_SYNC);
-      lock->get_wrlock(true);
+    if (in->client_snap_caps.empty()) {
+      for (int i = 0; i < num_cinode_locks; i++) {
+       int lockid = cinode_lock_info[i].lock;
+       SimpleLock *lock = in->get_lock(lockid);
+       ceph_assert(lock);
+       in->auth_pin(lock);
+       lock->set_state(LOCK_SNAP_SYNC);
+       lock->get_wrlock(true);
+      }
     }
+    in->client_snap_caps.insert(client);
     mds->locker->mark_need_snapflush_inode(in);
   }
 }