mds: set the correct WRLOCK flag always in wrlock_force() #57085
Conversation
| @@ -1832,7 +1832,8 @@ void Locker::wrlock_force(SimpleLock *lock, MutationRef& mut) | |||
| dout(7) << "wrlock_force on " << *lock | |||
| << " on " << *lock->get_parent() << dendl; | |||
| lock->get_wrlock(true); | |||
| mut->emplace_lock(lock, MutationImpl::LockOp::WRLOCK); | |||
| auto it = mut->emplace_lock(lock, MutationImpl::LockOp::WRLOCK); | |||
| it->flags |= MutationImpl::LockOp::WRLOCK; // may already remote_wrlocked | |||
There was a problem hiding this comment.
I was looking at Server::rdlock_two_paths_xlock_destdn and found it strange that this method is grabbing a remote_wrlock at all on an inode the MDS may be auth for. It's checking if the directory auth is different (as in the case in the bug) but then grabs a remote_wrlock on the inode's nestlock -- but it is auth for the inode. The check seems odd but maybe that's just how it's supposed to work; it's been that way for a while. I don't get it though.
There was a problem hiding this comment.
The directory is fragmented and the fragmented CDirs' auths could be different with the CInode's. In Server::rdlock_two_paths_xlock_destdn() it will try to grab remote_wrlock for the CDir's in the remote auth MDS. While the remote MDS isn't the auth of the CInode.
While the local wrlock is for the CInode's auth in local MDS.
There was a problem hiding this comment.
The directory is fragmented and the fragmented
CDirs'auths could be different with theCInode's.
Correct.
In
Server::rdlock_two_paths_xlock_destdn()it will try to grabremote_wrlockfor theCDir'sin the remote authMDS. While the remoteMDSisn't the auth of theCInode.
That's the the thing though: a CDir has no locks. Those "remote wrlocks" are actually for the inode which the MDS may be auth for. So why would it request a remote_wrlock on a lock with a parent (CInode) that it is auth for?
There was a problem hiding this comment.
Hmm, this is beyond my knowledge about this. What I get from the code before is that the remote_wrlock will always in the CInode's auth. I need to check the history of the CDir case to get why.
There was a problem hiding this comment.
This was first introduced by:
commit d72bdab7d7c74148cbb119a0e7c1eb14a0432d46
Author: Sage Weil <sweil@redhat.com>
Date: Fri Jul 8 09:32:04 2011 -0700
mds: take a remote_wrlock on srcdir for cross-mds rename
This ensures that we hold a wrlock on the srcdn auth when the slave
makes it's changes to the src directory, and prevents us from corrupting
the scatterlock state.
Signed-off-by: Sage Weil <sage.weil@dreamhost.com>
And by going through the code the wrlock is not like the xlock, which need to do it in the CInode's auth always. While the wrlock is based on the CDir's auths instead.
There was a problem hiding this comment.
Very good. Please update the code with a comment explanation Xiubo.
|
jenkins retest this please |
The wrlock is not like the xlock, which needs to be acquired in the CInode's auth always, and it is based on the CDir's auths instead. When a remote_wrlock is acquired and the local MDS will add a lock item and marks it as REMOTE_WRLOCK. And later when the local MDS try to force wrlock in the emplace_lock() will just return the existing lock item without updating the WRLOCK flag. So when cleaning the requests later it will just release the remote locks and then removes lock items directly, which will miss releasing the local wrlock reference. Fixes: https://tracker.ceph.com/issues/65630 Signed-off-by: Xiubo Li <xiubli@redhat.com>
The wrlock is not like the xlock, which needs to be acquired in
the CInode's auth always, and it is based on the CDir's auths instead.
When a remote_wrlock is acquired and the local MDS will add a lock
item and marks it as REMOTE_WRLOCK. And later when the local MDS try
to force wrlock in the emplace_lock() will just return the existing
lock item without updating the WRLOCK flag. So when cleaning the
requests later it will just release the remote locks and then removes
lock items directly, which will miss releasing the local wrlock
reference.
Fixes: https://tracker.ceph.com/issues/65630
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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.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 windowsjenkins test rook e2e