Ceph » Linux kernel client

[root@us-imm-cephmon1 ~]# ceph -s
    cluster fc43f491-9693-48d3-91be-d5bb2b7a085e
     health HEALTH_WARN
            nodeep-scrub,sortbitwise flag(s) set
     monmap e4: 3 mons at {us-imm-cephmon1=[2a02:4780:bad:c0de::d]:6789/0,us-imm-cephmon2=[2a02:4780:bad:c0de::17]:6789/0,us-imm-cephmon3=[2a02:4780:bad:c0de::18]:6789/0}
            election epoch 140, quorum 0,1,2 us-imm-cephmon1,us-imm-cephmon2,us-imm-cephmon3
      fsmap e154905: 1/1/1 up {0=v6.us-imm-cephmon1=up:active}, 2 up:standby
     osdmap e4228: 27 osds: 27 up, 27 in
            flags nodeep-scrub,sortbitwise
      pgmap v4950900: 768 pgs, 3 pools, 2181 GB data, 114 Mobjects
            5874 GB used, 127 TB / 133 TB avail
                 759 active+clean
                   9 active+clean+scrubbing
  client io 2025 kB/s rd, 3252 kB/s wr, 191 op/s rd, 832 op/s wr

what workloads trigger this bug?

could you please check if this issue exists in 4.5 kernel

fs/ceph/inode.c:1272 is BUG_ON(d_inode(dn->d_parent) != dir); For cephfs case, I think only thing that can change d_parent is d_splice_alias(). Maybe d_splice_alias() modifies lookup request's r_dentry while waiting for reply

Jeff, could you take a look

Sure...

First, does this kernel have any patches beyond what went into v4.8.4 ? There are some other BUG_ONs in that area so anything that adds or removes lines before that point in the file could affect which BUG_ON actually got tripped.

Also, what sort of workload is this -- are there multiple clients running and thrashing the filesystem concurrently? Single client?

Assuming that it is a stock v4.8.4 kernel:

                struct inode *dir = req->r_locked_dir;
                struct dentry *dn = req->r_dentry;
                bool have_dir_cap, have_lease;

                BUG_ON(!dn);
                BUG_ON(!dir);
                BUG_ON(d_inode(dn->d_parent) != dir);

So basically that just means that we got a different dentry/parent pair in the reply traces than we expected?

While that seems like it's non-ideal, BUG'ing here seems like the wrong way to handle it. Just because the vfs holds the i_rwsem for write, that doesn't prohibit other clients (for instance) from doing namespace changes, right? Also, we could end up with a broken or malicious MDS here, and I don't think we want to crash the client for that. We should probably change those BUG_ONs to handle this situation more gracefully. In NFS, we'd probably just invalidate the relevant cache objects here under the assumption that something changed on the server. I'm not sure what we'd need to do in Ceph.

Now all of that said, a big VFS layer change in v4.8 was the change of the i_mutex to i_rwsem to enable parallel lookups. splice_dentry does use d_splice_alias, and we don't necessarily hold a write lock on the i_rwsem in those codepaths anymore even though the comments say that we must hold i_mutex for that to be safe.

One theory: before v4.8 we'd hold the mutex over a lookup, so even if something changed on the server, only one task could call d_splice_alias at a time. Now though, we can potentially have multiple tasks calling it, processing traces that involve the same directory in parallel. So Zheng may be on to something here...one way to try and confirm that would be to have the lookup and readdir codepaths upgrade the i_rwsem to a rw lock.

It'll slow things down but may let us know whether we're on the right track here.

There are 6 nodes that have mounted the same cephfs directory and 4 of them doing active R/W requests on same paths (load balanced), so your theory makes sense in this case.

Kernel is stock, no patches.

Just tried to upgrade kernel to v4.8.11 - nothing's changed, 3 out of 3 nodes had kernel panics in first hour (kernel: kernel BUG at fs/ceph/inode.c:1272!).

Currently downgrading kernel to v4.5.4.

Ok, thanks. I see that you had written that in the original description too. If you have the time, then it might be good to test out the latest v4.9-rc kernel as well, but I suspect you'll hit the same problem there.

I got Al Viro to take a peek:

<viro> but there's a problem with ->d_revalidate()
<viro> it had always been callable without parent locked
<viro> before RCU work, even
<viro> if we find it on plain dcache lookup, we call ->d_revalidate()
<viro> ceph_d_revalidate() does
<viro>                         req->r_locked_dir = dir;
<viro> and it's not guaranteed to be locked at all
<viro> never had been
<viro> worse, the same method _can_ be called with parent locked by caller
<viro> so it can't go and lock the sucker itself
<viro> X-broken-by: commit 200fd27c8fa2ba8bb4529033967b69a7cbfa2c2e
<viro> Author: Yan, Zheng <zyan@redhat.com>
<viro> Date:   Thu Mar 17 14:41:59 2016 +0800
<viro>     ceph: use lookup request to revalidate dentry

We should take a hard look at that. That may or may not be the culprit here, but it's at least a problem if not the problem.

It may be sufficient to simply to set r_locked_dir to NULL in d_revalidate. I don't think we specifically care about the parent dentry for d_revalidate, do we?

I'll plan to test out a patch with that and see whether it breaks anything.

From ceph_d_revalidate:

                req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
                if (!IS_ERR(req)) {
                        req->r_dentry = dget(dentry);
                        req->r_num_caps = 2;

                        mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
                        if (ceph_security_xattr_wanted(dir))
                                mask |= CEPH_CAP_XATTR_SHARED;
                        req->r_args.getattr.mask = mask;

                        req->r_locked_dir = dir;
                        err = ceph_mdsc_do_request(mdsc, NULL, req);
                        if (err == 0 || err == -ENOENT) {
                                if (dentry == req->r_dentry) {
                                        valid = !d_unhashed(dentry);
                                } else {
                                        d_invalidate(req->r_dentry);
                                        err = -EAGAIN;
                                }
                        }
                        ceph_mdsc_put_request(req);
                        dout("d_revalidate %p lookup result=%d\n",
                             dentry, err);
                }

The result handling after ceph_mdsc_do_request looks fishy. We do the same thing if we get err 0 as we do if we get err -ENOENT, regardless of whether the original dentry was negative or not?

OTOH, I'm still trying to understand the logic in ceph_fill_trace, so that may be correct depending on what sort of dcache shenanigans it does.

Jeff Layton wrote:

From ceph_d_revalidate:

[...]

The result handling after ceph_mdsc_do_request looks fishy. We do the same thing if we get err 0 as we do if we get err -ENOENT, regardless of whether the original dentry was negative or not?

OTOH, I'm still trying to understand the logic in ceph_fill_trace, so that may be correct depending on what sort of dcache shenanigans it does.

The reason I wrote this code is that the dentry was spliced, no matter original dentry was negative or not, request->r_dentry should be valid.

Jeff Layton wrote:

Ok, thanks. I see that you had written that in the original description too. If you have the time, then it might be good to test out the latest v4.9-rc kernel as well, but I suspect you'll hit the same problem there.

I got Al Viro to take a peek:

[...]

We should take a hard look at that. That may or may not be the culprit here, but it's at least a problem if not the problem.

It may be sufficient to simply to set r_locked_dir to NULL in d_revalidate. I don't think we specifically care about the parent dentry for d_revalidate, do we?

I'll plan to test out a patch with that and see whether it breaks anything.

I'm little worry about that d_revalidate does sleep lookup while parent is not locked. I think the dentry can get spliced while waiting for the reply. I think the better solution is make vfs tell us if parent is locked. handle the unlocked case as rcu case.

Zheng Yan wrote:

I'm little worry about that d_revalidate does sleep lookup while parent is not locked. I think the dentry can get spliced while waiting for the reply. I think the better solution is make vfs tell us if parent is locked. handle the unlocked case as rcu case.

I don't think that will work. The VFS only locks the parent's rwsem if it's going to do a "real" lookup. lookup_fast won't take it (since it doesn't need it), and that's the main caller of d_revalidate. We could somehow communicate to the VFS that we require the parent to be locked (that was one of Al's suggestions), but I'm not sure I like that.

Alternately, do we really need to set r_locked_dir at all? What we really want to know here is whether the dentry still points to the same inode (or is still negative). We don't really need the parent's info to satisfy d_revalidate.

The main problem there (AFAICT) is that I don't see how to communicate those results back from ceph_fill_trace when r_locked_dir isn't set. We rely that function manipulating the dcache for that now, and that requires the parent's mutex.

I think the thing we want to do here is to not set r_locked_dir, and pass a reference to the inode that was the result of the lookup back to the d_revalidate function. Then we can do something like this in ceph_d_revalidate:

if (d_inode(dentry) == inode)
        valid = 1;

...and that sidesteps the whole problem.

The question is how to pass an inode reference back to it there. There is a req->r_inode field that we could try to use for that purpose, but other codepaths use that field as an argument not as a result container.

What's the right way to do that? Alternately, we could pass back just enough info to ID the inode (e.g. i_ino and i_generation).

Jeff Layton wrote:

Zheng Yan wrote:

I'm little worry about that d_revalidate does sleep lookup while parent is not locked. I think the dentry can get spliced while waiting for the reply. I think the better solution is make vfs tell us if parent is locked. handle the unlocked case as rcu case.

I don't think that will work. The VFS only locks the parent's rwsem if it's going to do a "real" lookup. lookup_fast won't take it (since it doesn't need it), and that's the main caller of d_revalidate. We could somehow communicate to the VFS that we require the parent to be locked (that was one of Al's suggestions), but I'm not sure I like that.

My point is let vfs tell us if the parent is locked. If it's not, we skip the step that sending a lookup request to mds. In most cases, our d_revalidate can still use lease or CEPH_CAP_FILE_SHARED to validate the dentry.

Sending a lookup request to mds is slow. I think it’s meaningless for these fast lookup cases. It's better to let VFS fallback to the slow lookup.

Alternately, do we really need to set r_locked_dir at all? What we really want to know here is whether the dentry still points to the same inode (or is still negative). We don't really need the parent's info to satisfy d_revalidate.

I think we can do this (maybe need minor modification to the code). But I don't understand how does this suppose to work in following situation.

- send lookup request to mds
- the dentry get spliced, it's parent changed
- get lookup reply.

let's assume the replied inode is equal to dentry inode. Is the dentry valid ?

The main problem there (AFAICT) is that I don't see how to communicate those results back from ceph_fill_trace when r_locked_dir isn't set. We rely that function manipulating the dcache for that now, and that requires the parent's mutex.

Jeff Layton wrote:

I think the thing we want to do here is to not set r_locked_dir, and pass a reference to the inode that was the result of the lookup back to the d_revalidate function. Then we can do something like this in ceph_d_revalidate:

[...]

...and that sidesteps the whole problem.

The question is how to pass an inode reference back to it there. There is a req->r_inode field that we could try to use for that purpose, but other codepaths use that field as an argument not as a result container.

I think we can use req->r_target_inode

What's the right way to do that? Alternately, we could pass back just enough info to ID the inode (e.g. i_ino and i_generation).

Zheng Yan wrote:

Jeff Layton wrote:

Zheng Yan wrote:

I'm little worry about that d_revalidate does sleep lookup while parent is not locked. I think the dentry can get spliced while waiting for the reply. I think the better solution is make vfs tell us if parent is locked. handle the unlocked case as rcu case.

I don't think that will work. The VFS only locks the parent's rwsem if it's going to do a "real" lookup. lookup_fast won't take it (since it doesn't need it), and that's the main caller of d_revalidate. We could somehow communicate to the VFS that we require the parent to be locked (that was one of Al's suggestions), but I'm not sure I like that.

My point is let vfs tell us if the parent is locked. If it's not, we skip the step that sending a lookup request to mds. In most cases, our d_revalidate can still use lease or CEPH_CAP_FILE_SHARED to validate the dentry.

Sending a lookup request to mds is slow. I think it’s meaningless for these fast lookup cases. It's better to let VFS fallback to the slow lookup.

Yes, though the whole reason for doing a lookup here (AIUI) is to avoid having the dentry be d_invalidated. Honestly, I hate the d_revalidate API. Maybe we should point out this difficulty in the email thread.

Alternately, do we really need to set r_locked_dir at all? What we really want to know here is whether the dentry still points to the same inode (or is still negative). We don't really need the parent's info to satisfy d_revalidate.

I think we can do this (maybe need minor modification to the code). But I don't understand how does this suppose to work in following situation.

- send lookup request to mds
- the dentry get spliced, it's parent changed
- get lookup reply.

let's assume the replied inode is equal to dentry inode. Is the dentry valid ?

Yes -- That's the way it works with NFS, for instance. All the filesystem needs to answer is "Does this dentry still point at this inode?". Handling changes to the dcache hierarchy is the job of the vfs layer.

Hmm...maybe we should update vfs.txt to make that clear.

got it. the whole point is avoid d_invalidated

Jeff Layton wrote:

I think we can do this (maybe need minor modification to the code). But I don't understand how does this suppose to work in following situation.

- send lookup request to mds
- the dentry get spliced, it's parent changed
- get lookup reply.

let's assume the replied inode is equal to dentry inode. Is the dentry valid ?

Yes -- That's the way it works with NFS, for instance. All the filesystem needs to answer is "Does this dentry still point at this inode?". Handling changes to the dcache hierarchy is the job of the vfs layer.

Hmm...maybe we should update vfs.txt to make that clear.

So that said...there is a difference between NFS and Ceph here, and that is that an NFS lookup involves a filehandle and a component name, whereas Ceph builds a full path and requests that. Thus, NFS more closely mirrors how the dcache actually works. With Ceph, you could get intermediate components renamed after you build a path but before you send it to the server, which could render the results of d_revalidate questionable.

I don't see a way to fix that given the current Ceph protocol though, given its reliance on (ick) pathnames. Note that CIFS has the same problem.

Did some basic testing (with a doctored kernel that always revalidates dentries) and this seems to do the right thing. That said, I am seeing some hangs in testing with the latest ceph/testing branch with synchronous calls seeming to get stuck waiting on replies. I don't think this patch is the cause but it would be good to rule that out.

Jeff Layton wrote:

Actually, I think we want this patch. Now that we're not manipulating the dcache with this call, we must handle the different outcomes from the lookup differently.

I lightly tested this patch on top of ceph/testing running connectathon and it seemed to do OK. I'm not sure that that really exercises this codepath well though.

I also hacked ceph_d_revalidate to always do an on-the-wire revalidation, but then the test kept hanging on an rmdir (completely unrelated codepath). Possibly I broke something there, but it may be that the increased MDS lookup load is uncovering a different bug.

Note too that we need to remove the WARN_ON_ONCE in ceph_fill_trace, as this seems to trip it up. I don't think we need a WARN_ON just because MDS sent us an unsolicited is_dentry trace.

If we only want inode, it's better to use getattr to validate dentry (getattr is basically the same as lookup, except mds does send dentry trace for getattr).

There are some places that access req->r_dentry->d_parent. Is it safe to do that when parent is not locked? maybe we need to use dget_parent in these places

Zheng Yan wrote:

If we only want inode, it's better to use getattr to validate dentry (getattr is basically the same as lookup, except mds does send dentry trace for getattr).

Interesting, I had always figured that getattr would be done by inode number and not path. Note that we do need to verify that the dentry name points to the inode so the mds does need to do a name to inode lookup of some sort.

There are some places that access req->r_dentry->d_parent. Is it safe to do that when parent is not locked? maybe we need to use dget_parent in these places

Generally no, that's not safe. The only place that looks suspicious right offhand is in __choose_mds. dget_parent would probably work there, but we might also be able to rcu there instead since it doesn't look like any of the functions in the relevant code will block.

Jeff Layton wrote:

Zheng Yan wrote:

If we only want inode, it's better to use getattr to validate dentry (getattr is basically the same as lookup, except mds does send dentry trace for getattr).

Interesting, I had always figured that getattr would be done by inode number and not path. Note that we do need to verify that the dentry name points to the inode so the mds does need to do a name to inode lookup of some sort.

sorry, I mean "getattr is basically the same as lookup, except mds does NOT send dentry trace for getattr". getattr can do name lookup, it's ideal for this case.

Tested this patch (https://github.com/ceph/ceph-client/commit/06ae6a76e4570cba948084f6a1c0d206485d1706) on two servers with kernel v4.8.12 yesterday for 6 hours straight and there were no kernel panics. Then another problem came up - after some time all IO started to get very slow (we had this problem earlier, but it wouldn't have time to fully show up before kernel panic). mdsc showed a lot of (thousands) requests processing and steadily growing (osdc showed just a few requests and they were processed fast). Requests were not hanged, they were being processed, but very slowly. Because of that we could not continue and returned to kernel v4.5, which handles way more IO in our case.

Thanks for testing it. The slowdown sounds like a different problem entirely and I'd suggest opening a separate bug for that.

The patch for this bug has been merged into the testing branch, and I think we want it to go into stable kernels as well. I'll leave this open for now until it has been merged into master.