Ceph » CephFS

Reasonable assumption about this crash is either the inode was deleted (in which case the Cap should have been deleted too) or the inode is a nullptr. The latter should never happen AFAICT as the inode is set only when the cap is created.

I couldn't find a way for the inode/cap to be deleted while the cap remains in the session cap list. Ideas?

commit 4dda1b6ead6b1f04f996403881f61e9b7d94dba0
Author: Patrick Donnelly <pdonnell@redhat.com>
Date:   Sun Nov 19 15:08:18 2017 -0800

    client: anchor Inode while trimming caps

    This prevents the Inode from being deleted until after cap trimming is
    finished. In particular, this prevents remove_all_caps from being called which
    screws up the traversal of caps in trim_caps.

    Fixes: http://tracker.ceph.com/issues/22157

    Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
    (cherry picked from commit 1439337e949c9fcb7d15eb38c22d19eb57d3d0f2)

I think above commit isn't quite right. how about patch below

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 5bbe449974..2885a87e48 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -4113,14 +4119,30 @@ void Client::trim_caps(MetaSession *s, uint64_t max)

   uint64_t trimmed = 0;
   auto p = s->caps.begin();
-  std::set<InodeRef> anchor; /* prevent put_inode from deleting all caps during traversal */
-  while ((caps_size - trimmed) > max && !p.end()) {
-    Cap *cap = *p;
-    InodeRef in(cap->inode);
+
+  InodeRef next_in;
+  Cap *next_cap;
+  if (p.end()) {
+    next_cap = nullptr;
+  } else {
+    next_cap = *p;
+    next_in = next_cap->inode;
+  }
+
+  while (next_cap && (caps_size - trimmed) > max) {
+    Cap *cap = next_cap;
+    InodeRef in;
+    in.swap(next_in);

     // Increment p early because it will be invalidated if cap
     // is deleted inside remove_cap
     ++p;
+    if (p.end()) {
+      next_cap = nullptr;
+    } else {
+      next_cap = *p;
+      next_in = next_cap->inode;
+    }

     if (in->caps.size() > 1 && cap != in->auth_cap) {
       int mine = cap->issued | cap->implemented;
@@ -4129,7 +4151,6 @@ void Client::trim_caps(MetaSession *s, uint64_t max)
       if (!(get_caps_used(in.get()) & ~oissued & mine)) {
     ldout(cct, 20) << " removing unused, unneeded non-auth cap on " << *in << dendl;
     cap = (remove_cap(cap, true), nullptr);
-        /* N.B. no need to push onto anchor, as we are only removing one cap */
     trimmed++;
       }
     } else {
@@ -4148,8 +4169,6 @@ void Client::trim_caps(MetaSession *s, uint64_t max)
         // the end of this function.
         _schedule_invalidate_dentry_callback(dn, true);
       }
-          ldout(cct, 20) << " anchoring inode: " << in->ino << dendl;
-          anchor.insert(in);
       trim_dentry(dn);
         } else {
           ldout(cct, 20) << "  not expirable: " << dn->name << dendl;
@@ -4162,8 +4181,6 @@ void Client::trim_caps(MetaSession *s, uint64_t max)
       }
     }
   }
-  ldout(cct, 20) << " clearing anchored inodes" << dendl;
-  anchor.clear();

   caps_size = s->caps.size();
   if (caps_size > (size_t)max)

Zheng Yan wrote:

[...]

I think above commit isn't quite right. how about patch below

[...]

I'm not seeing how that helps the situation. The issue my patch was trying to solve is preventing the next cap (what p points to after ++p) from being invalidated due to trim_dentry -> Client::unlink -> Dentry::put -> Inode::put -> intrusive_ptr_release -> Client::put_inode -> Client::remove_all_caps. This should be prevented by anchoring the Inode references as in the original PR?

We need a test case to definitively show the issue. I'm not willing to push any new fixes for this without a reproducer.

The problem is that anchor only pins current inode. Client::unlink() still may drop reference of its parent inode.

Zheng Yan wrote:

The problem is that anchor only pins current inode. Client::unlink() still may drop reference of its parent inode.

Okay that makes sense I think. We still need a test that demonstrates the problem.