Ceph » Linux kernel client

Ouch...the ceph readdir code seems to keep pointers to dentries in the ceph_readdir_cache_control, but doesn't hold references to them. Am I missing something on how that is expected to work? We probably need to ensure that we hold references to the child dentries in that code.

The idea is borrowed from ncpfs. When receiving reply from server, the readdir code fills dentry pointers in page cache for later readdir. The code knows a dentry pointers is valid when following checks pass: (1) no dentry was trimmed since filling the cache (For cephfs, the check is ceph_dir_is_complete_ordered(), the vfs callback for trimming dentry is ceph_d_prune()) (2) lockref_get_not_dead(dentry pointer) return non-null. Besides, the code needs to do both checks in RCU read lock.

I suspect the first check does not properly handle the d_splice_alias() case.

We clear the parent's complete bit whenever we call splice_dentry, AFAICT. The only exception is in the readdir code itself, and that's precisely the place that we want to set that bit, not clear it.

It does sound like there may be a missing ceph_dir_clear_complete call, I'm just not sure where it is. I will note that I don't see it being cleared in either the unlink or the create codepath (though it is cleared on rename).

I guess the trace handling is supposed to do that?

Jeff Layton wrote:

We clear the parent's complete bit whenever we call splice_dentry, AFAICT. The only exception is in the readdir code itself, and that's precisely the place that we want to set that bit, not clear it.

we call ceph_dir_clear_ordered() before splice_dentry(). But only for dentry's current parent. If the dentry was spliced from other directory, we don't call ceph_dir_clear_ordered() for the old directroy.

It does sound like there may be a missing ceph_dir_clear_complete call, I'm just not sure where it is. I will note that I don't see it being cleared in either the unlink or the create codepath (though it is cleared on rename).

I guess the trace handling is supposed to do that?

Yes, trace handling does the job (ceph_dir_clear_ordered() in ceph_fill_trace()). rename is not special either. The ceph_dir_clear_complete()s in ceph_rename() are for traceless reply. recent version mds does not send traceless reply.

Zheng Yan wrote:

we call ceph_dir_clear_ordered() before splice_dentry(). But only for dentry's current parent. If the dentry was spliced from other directory, we don't call ceph_dir_clear_ordered() for the old directroy.

For my own notes...there are 2 cases here fill_trace and ceph_readdir_prepopulate:

The main concern I think is in fill_trace. We use req->r_dentry and splice the inode we find in the trace into it. This case does call ceph_clear_dir_ordered prior to any call to splice_dentry. There's also this assertion at the top of the first conditional if block:

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

...but, I suppose it's possible that we could end up with a remotely renamed directory (the __d_unalias case in d_splice_alias). What happens to the complete bit in the original parent when that occurs? Probably nothing. Still...

I don't suppose you happened to have gotten a vmcore here? It would be interesting to look at the dentry here and verify that di was actually NULL, or whether the dentry itself was just toast.

One thing that looks suspicious to me is that we check __ceph_dir_is_complete_ordered in ceph_readdir, and that is what governs whether the cached dentry pointers are valid. What guarantees that it remains complete/ordered once the spinlock is dropped in there? Could the real fix be that we should be holding a reference to CEPH_CAP_FILE_SHARED on the directory when we call __dcache_readdir?

Jeff Layton wrote:

Another question too...why are we using CEPH_CAP_FILE_SHARED in this code. Shouldn't we require CEPH_CAP_FILE_CACHE? >Directories are different than files though, so perhaps that explains the discrepancy there?

We happen to use page cache to implement readdir cache, nothing to do with real file data cache. The reason that we use CEPH_CAP_FILE_SHARED here is that readdir cache is valid as long as client holds the CEPH_CAP_FILE_SHARED. The readdir cache become stale once client loses CEPH_CAP_FILE_SHARED. (If mds need to modify the directory, it revokes CEPH_CAP_FILE_SHARED first)

Jeff Layton wrote:

Zheng Yan wrote:

we call ceph_dir_clear_ordered() before splice_dentry(). But only for dentry's current parent. If the dentry was spliced from other directory, we don't call ceph_dir_clear_ordered() for the old directroy.

For my own notes...there are 2 cases here fill_trace and ceph_readdir_prepopulate:

The main concern I think is in fill_trace. We use req->r_dentry and splice the inode we find in the trace into it. This case does call ceph_clear_dir_ordered prior to any call to splice_dentry. There's also this assertion at the top of the first conditional if block:

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

...but, I suppose it's possible that we could end up with a remotely renamed directory (the __d_unalias case in d_splice_alias). What happens to the complete bit in the original parent when that occurs? Probably nothing. Still...

that what I suspects

I don't suppose you happened to have gotten a vmcore here? It would be interesting to look at the dentry here and verify that di was actually NULL, or whether the dentry itself was just toast.

no vmcore

One thing that looks suspicious to me is that we check __ceph_dir_is_complete_ordered in ceph_readdir, and that is what governs whether the cached dentry pointers are valid. What guarantees that it remains complete/ordered once the spinlock is dropped in there? Could the real fix be that we should be holding a reference to CEPH_CAP_FILE_SHARED on the directory when we call __dcache_readdir?

rcu_read_lock() and lockref_get_not_dead() guarantee that dentry pointer is valid even directory no longer complete/ordered. (rcu_read_lock() prevent freeing dentry from be reused. lockref_get_not_dead() makes sure the dentry is not being freed and get a reference)

Zheng Yan wrote:

no vmcore

Bummer. Do you have the text in the log from before the Oops line? It'd be good to know whether it was a GPF, NULL pointer exception or what.

If it was NULL pointer, then I'd be inclined to think that ceph_dentry returned NULL such that the dentry was valid but on its way to being torn down. If it's a GPF, then it sounds more like maybe we have a cached array of dentry pointers that point into memory that has been recycled for other purposes.

Either way, knowing what happened to cause the crash might help us rule out some potential avenues. If it happens again, and you can get a core, then so much the better.

rcu_read_lock() and lockref_get_not_dead() guarantee that dentry pointer is valid even directory no longer complete/ordered. (rcu_read_lock() prevent freeing dentry from be reused. lockref_get_not_dead() makes sure the dentry is not being freed and get a reference)

Right, so I think we have to consider how we would have gotten a bad pointer here. I think there are two likely possibilities:

1) we have a dentry that is not yet freed but somehow ended up with a ceph_dentry of NULL (race in lockref_get_not_dead handling?)

2) we have a dentry pointer in the cache that is being considered valid by the ceph readdir code, but is actually gone and has been recycled. lockref_get_not_dead just happened to work because of the way the memory was reused.

I think 2 is the most likely scenario, but it's hard to tell with just what we have so far here.

Jeff Layton wrote:

Zheng Yan wrote:

no vmcore

Bummer. Do you have the text in the log from before the Oops line? It'd be good to know whether it was a GPF, NULL pointer exception or what.

If it was NULL pointer, then I'd be inclined to think that ceph_dentry returned NULL such that the dentry was valid but on its way to being torn down. If it's a GPF, then it sounds more like maybe we have a cached array of dentry pointers that point into memory that has been recycled for other purposes.

Either way, knowing what happened to cause the crash might help us rule out some potential avenues. If it happens again, and you can get a core, then so much the better.

If I remember right, it's NULL pointer dereference. dentry is likely valid because the code has succeeded in locking d_lock.

rcu_read_lock() and lockref_get_not_dead() guarantee that dentry pointer is valid even directory no longer complete/ordered. (rcu_read_lock() prevent freeing dentry from be reused. lockref_get_not_dead() makes sure the dentry is not being freed and get a reference)

Right, so I think we have to consider how we would have gotten a bad pointer here. I think there are two likely possibilities:

1) we have a dentry that is not yet freed but somehow ended up with a ceph_dentry of NULL (race in lockref_get_not_dead handling?)

2) we have a dentry pointer in the cache that is being considered valid by the ceph readdir code, but is actually gone and has been recycled. lockref_get_not_dead just happened to work because of the way the memory was reused.

I think 2 is the most likely scenario, but it's hard to tell with just what we have so far here.

Agree. The oops happened in multimds setup. I guess CEPH_CAP_FILE_SHARED was not properly revoked.

Zheng Yan wrote:

Jeff Layton wrote:

Zheng Yan wrote:

no vmcore

Bummer. Do you have the text in the log from before the Oops line? It'd be good to know whether it was a GPF, NULL pointer exception or what.

If it was NULL pointer, then I'd be inclined to think that ceph_dentry returned NULL such that the dentry was valid but on its way to being torn down. If it's a GPF, then it sounds more like maybe we have a cached array of dentry pointers that point into memory that has been recycled for other purposes.

Either way, knowing what happened to cause the crash might help us rule out some potential avenues. If it happens again, and you can get a core, then so much the better.

If I remember right, it's NULL pointer dereference. dentry is likely valid because the code has succeeded in locking d_lock.

Maybe. It really depends on what was in the memory. Dentries come out of a slabcache, so it's possible it got recycled as another dentry that didn't set d_fsdata.

rcu_read_lock() and lockref_get_not_dead() guarantee that dentry pointer is valid even directory no longer complete/ordered. (rcu_read_lock() prevent freeing dentry from be reused. lockref_get_not_dead() makes sure the dentry is not being freed and get a reference)

Right, so I think we have to consider how we would have gotten a bad pointer here. I think there are two likely possibilities:

1) we have a dentry that is not yet freed but somehow ended up with a ceph_dentry of NULL (race in lockref_get_not_dead handling?)

2) we have a dentry pointer in the cache that is being considered valid by the ceph readdir code, but is actually gone and has been recycled. lockref_get_not_dead just happened to work because of the way the memory was reused.

I think 2 is the most likely scenario, but it's hard to tell with just what we have so far here.

Agree. The oops happened in multimds setup. I guess CEPH_CAP_FILE_SHARED was not properly revoked.

Yes, we also don't hold a reference to CEPH_CAP_FILE_SHARED in this code. I suppose that's the first thing to fix, and then we can see if the problem is still reproducible afterward.

So, my thinking was to call ceph_try_get_caps in ceph_readdir to grab CEPH_CAP_FILE_RD and CEPH_CAP_FILE_SHARED. ceph_try_get_caps has these BUG_ONs in there:

       BUG_ON(need & ~CEPH_CAP_FILE_RD);
       BUG_ON(want & ~(CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO));

Zheng, those were added pretty recently in 2b1ac852eb67a6e95595e576371d23519105559f. Why the strict rules on need and want there?

Jeff Layton wrote:

So, my thinking was to call ceph_try_get_caps in ceph_readdir to grab CEPH_CAP_FILE_RD and CEPH_CAP_FILE_SHARED. ceph_try_get_caps has these BUG_ONs in there:

[...]

Zheng, those were added pretty recently in 2b1ac852eb67a6e95595e576371d23519105559f. Why the strict rules on need and want there?

see __take_cap_refs(), it only supports these caps.

Jeff Layton wrote:

Yes, we also don't hold a reference to CEPH_CAP_FILE_SHARED in this code. I suppose that's the first thing to fix, and then we can see if the problem is still reproducible afterward.

I think we can just call __ceph_dir_clear_complete when client release CEPH_CAP_FILE_SHARED to mds