mds: retry rdlock_path_pin_ref if needs to fwd request to auth #52453
Conversation
src/mds/Server.cc
Outdated
| CInode *cur = rdlock_path_pin_ref(mdr, need_auth); | ||
| if (!cur) | ||
| return; | ||
|
|
||
| if (cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) { | ||
| if ((cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) && !need_auth) { |
There was a problem hiding this comment.
So, the request should have been forwarded if it was not the auth mds and then we won't reach here. This looks fine.
There was a problem hiding this comment.
Ok, but thinking more on this - it requires multiple MDSs to run into the deadlock? If yes, I'm not sure how this was hit in the setup where the deadlock was seen.
There was a problem hiding this comment.
I don't think this can be it because the inode dump does not show it as frozen or exporting caps.
There was a problem hiding this comment.
Ok, but thinking more on this - it requires multiple MDSs to run into the deadlock? If yes, I'm not sure how this was hit in the setup where the deadlock was seen.
There were 2 active MDSs.
There was a problem hiding this comment.
Ok, but thinking more on this - it requires multiple MDSs to run into the deadlock? If yes, I'm not sure how this was hit in the setup where the deadlock was seen.
There were 2 active MDSs.
Yeh, but the inode wasn't frozen, so it doesn't look to be this call path for the deadlock.
There was a problem hiding this comment.
I think you found a genuine issue here Xiubo. Can you restructure this to break out !need_auth into a nested if which will assert if need_auth == true (as, in that case, the request should have been forwarded).
There was a problem hiding this comment.
Sure.
There have two case:
1, _openc() --> _open(): the first call of rdlock_path_pin_ref() will do nothing even the need_auth == true, because in _openc() the rdlock_path_xlock_dentry() will set the MutationImpl::PATH_LOCKED flag.
So we should retry the rdlock_path_pin_ref() anyway if need_auth == true with:
- if (cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) {
- ceph_assert(!need_auth);
+ if (need_auth || cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) {
mdr->locking_state &= ~(MutationImpl::PATH_LOCKED | MutationImpl::ALL_LOCKED);
CInode *cur = rdlock_path_pin_ref(mdr, true);
if (!cur)
2, _open() directly: the first call of rdlock_path_pin_ref() will forward to auth directly if need_auth == true, so it shouldn't be here.
There was a problem hiding this comment.
So we should retry the rdlock_path_pin_ref() anyway if need_auth == true with:
That will always rerun rdlock_path_pin_ref twice if need_auth == true though, yes? I think we should try to avoid that except in the case of openc -> open. I wonder why not mdr->locking_state &= ~(MutationImpl::PATH_LOCKED | MutationImpl::ALL_LOCKED); before having openc call open.
There was a problem hiding this comment.
So we should retry the rdlock_path_pin_ref() anyway if need_auth == true with:
That will always rerun
rdlock_path_pin_reftwice ifneed_auth == truethough, yes? I think we should try to avoid that except in the case ofopenc -> open. I wonder why notmdr->locking_state &= ~(MutationImpl::PATH_LOCKED | MutationImpl::ALL_LOCKED);before havingopenccallopen.
No, not always. Mostly the first one will do nothing in case of _openc() --> _open() and no need to do anything. Because the clients will always try to send the WRITE request to auth MDS and the _openc() has already done what we need here.
Just in case the open requests need be handled in auth MDS but the current MDS is not auth or the inode is under exporting will we need to retry it.
|
@lxbsz can you share your reproducer you mentioned in slack? Even if it's an ad-hoc script. |
Locally I didn't reproduce it, but just added one test code try to acquire the This is what I had hit when I was coding the unlink related fixing last month. |
|
I was able to reproduce the problem. I'll have more details Monday. |
Cool @batrick. |
|
jenkins retest this please |
|
src/mds/MDCache.cc
Outdated
| lov.add_xlock(&dn->lock); | ||
| } else { | ||
| lov.add_rdlock(&dn->lock); | ||
| } |
There was a problem hiding this comment.
Since cf5fdcd, openc will not xlock an existing non-null dentry if O_EXCL is not set. Instead it will acquire a rdlock:
cf5fdcd#diff-277dc6e796ccecb6aa14c9357f7a86898d6ddcf4113c110a4e00e0a10f6fefa7R4476
So I don't think this commit will do anything?
There was a problem hiding this comment.
I think you are right.
Because only when dnl->is_null() || !want_inode will true will the _openc() xlock the dentry. While if dnl->is_null() is true it should go to _open(). And the rdlock_path_xlock_dentry() in _openc() will always make want_inode to true. So the _openc() won't xlock the dentry.
src/mds/Server.cc
Outdated
| CInode *cur = rdlock_path_pin_ref(mdr, need_auth); | ||
| if (!cur) | ||
| return; | ||
|
|
||
| if (cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) { | ||
| if ((cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) && !need_auth) { |
There was a problem hiding this comment.
I think you found a genuine issue here Xiubo. Can you restructure this to break out !need_auth into a nested if which will assert if need_auth == true (as, in that case, the request should have been forwarded).
lxbsz
changed the title
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
src/mds/Server.cc
Outdated
| @@ -4377,13 +4377,14 @@ void Server::handle_client_open(MDRequestRef& mdr) | |||
| respond_to_request(mdr, -CEPHFS_EROFS); | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
please remove unnecessary whitespace changes
src/mds/Server.cc
Outdated
| if (cur->is_frozen() || cur->state_test(CInode::STATE_EXPORTINGCAPS)) { | ||
| ceph_assert(!need_auth); | ||
| // In case the _open() is called from _openc() the previous rdlock_path_pin_ref() | ||
| // will fail to forward the request to auth MDS, we need to retry it here. |
There was a problem hiding this comment.
Can you expand in the comment here why this particular flow will fail to forward the request to auth?
I feel we could write a test for this:
- make a config for the client to always choose a particular rank by default
- make the necessary file, pin the directory its in to rank 1
- set the config to have the client always choose rank 0 by default
- write a python script to open the file
O_CREATE|O_TRUNC|O_WRONLY.
yes?
There was a problem hiding this comment.
Certainly, I will expand the comment and try to add a test case for it. Thanks @batrick
In openc() the 'PATH_LOCKED' will be set and if the file exists then it will call open() dirrectly, but when the create requests need be handled in auth MDS but the current MDS is not auth because of the inode is under exporting and the auth is changed, we need to retry rdlock_path_pin_ref() and forward the requests to the auth. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
There have some tests need multiple mds, such when pining directories. Signed-off-by: Xiubo Li <xiubli@redhat.com>
|
@batrick Added a test case for it. Please take a look. Thanks! [Edit] When testing this I just added the |
In case the _open() is called from _openc() the first
rdlock_path_pin_ref() will do nothing because the _openc() will
set the MutationImpl::PATH_LOCKED flag. And then we need to retry
it.
Signed-off-by: Xiubo Li xiubli@redhat.com
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows