Ceph » CephFS

i initially thought something like this would work

diff --git a/src/mds/Locker.cc b/src/mds/Locker.cc
index 3c7c190..04428c9 100644
--- a/src/mds/Locker.cc
+++ b/src/mds/Locker.cc
@ -758,6 +758,10 @ void Locker::eval_scatter_gathers(CInode *in)

void Locker::eval(SimpleLock *lock, bool *need_issue)
{
+  bool my_need_issue = false;
+  if (!need_issue)
+    need_issue = &my_need_issue;

   switch (lock->get_type()) {
   case CEPH_LOCK_IFILE:
     return file_eval((ScatterLock*)lock, need_issue);
@ -767,6 +771,9 @ void Locker::eval(SimpleLock lock, bool *need_issue)
   default:
     return simple_eval(lock, need_issue);
   }

+  if (my_need_issue && lock->get_type() != CEPH_LOCK_DN)
+    issue_caps((CInode)lock->get_parent());
 }

but i think that is going to cause lots of non-optimal lock thrashing. i suspect a better approach would be to make the eval() *need_issue argument non-optional and passed down via drop_locks, so that callers like file_update_finish have more control over when things happen. i'll take a closer look tomorrow...

I haven't test it for a while. I'll give it a try after the holidays.

Having looked at this some more, I'm pretty sure it's a different issue than Sage though.

The eval family of functions is only supposed to issue_caps if they are removing caps, not if it's now possible for somebody to take more (after all, these functions only look at the lock -- they don't know anything about who wants caps). As best I can tell, they do this correctly -- they pass around a bool *need_issue, and when appropriate¹ they either set it to true, or if the pointer is NULL they call need_issue themselves.
It is the responsibility of higher-level functions (like file_update_finish) to issue caps to clients that want more caps. It does attempt to do this:

    if (cap && (cap->wanted() & ~cap->pending())) {
      issue_caps(in, cap);
    }

However, in the partial log we've received, file_update_caps gets called after check_inode_max_size (called under handle_client_cap_release) and check_max_inode_size doesn't set the cap variable.
So that's why in this case the MDS isn't issuing caps again. I'll have to look around a bit more to figure out the proper fix, though.

[1]: On a cursory inspection -- maybe they've missed a case, but either way I don't think it could cause the bug we're seeing here.

The real question here is why the earlier callers of issue_caps think that the client (a loner) can't have the Fr cap which it is asking for. It shows up right away in the requested caps but isn't part of the allowed.

Optimizations may involve calculating caps in other places, but this is the actual bug.

Looked at this some more yesterday. Turns out the problem seems to be that the client closes the inode, and all caps on it are dropped. Then the MDS starts check_max_inode_size (and should it do this when an inode closes properly?). The client reopens the inode, but most of the caps are in sync state and the MDS has the write cap (from a force_wrlock, which might impact things on its own?) -- this prevents the inode from getting the file access caps it wants during the initial open.
When check_inode_max_size completes, the MDS drops the caps it's grabbed on to. In the log fragment we have, the client is then never issued any more caps. I'm not sure if this is because it would never be issued more caps (meaning the MDS needs to do something differently), or if it's because there was some other bug preventing it from being issued caps and the log we have is just incomplete.

I'm going to try and reproduce this bug, I think, but more information on the versions being and how to reproduce would be appreciated!

I just tested it with the latest ceph stable branch (0.29+) and kclient master branch (backported for 2.6.32 by me). Tested it 3 times, and the last one hangs.

As before, "ls -l /proc/<blogbench pid>/fd/" will let blogbench finish. It seems to me that if the client sent the cap request to the MDS again, the MDS would be able to issue Fr to the client without problems. Thus, there should be no other bug preventing Fr from being issued. It was just that MDS missed the chance of issueing Fr on file_update_finish, and then there are no other "trigger points" to issue it later.

After applying the following diff, blogbench can finish successfully every time:

diff --git a/src/mds/Locker.cc b/src/mds/Locker.cc
index 3c7c190..36da98e 100644
--- a/src/mds/Locker.cc
+++ b/src/mds/Locker.cc
@@ -1242,7 +1242,7 @@ void Locker::file_update_finish(CInode *in, Mutation *mut, bool share, client_t
    if (gather)
      eval_cap_gather(in);
  } else {
-    if (cap && (cap->wanted() & ~cap->pending())) {
+    if (!cap || (cap && (cap->wanted() & ~cap->pending()))) {
      issue_caps(in, cap);
    }

This may not be the right fix, just backing up my thoughts.

On the other side, I am also wondering if the bug is caused by my backport since no others than me are easy to reproduce it...................

The mds_issue_caps branch has my current work on cleaning the issue_caps stuff up. I'm pretty happy with what the MDS code looks like at the moment. My blogbench runs have turned up a couple of bugs in the kclient (pushed to master), but so far it's holding up...

It is working great. I haven't see this problem after applying the patches.