client: add dedicated inode lock support for libcephfs #39797
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
|
jenkins retest this please |
|
jenkins test api |
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
lxbsz
force-pushed
the
inode_lock
branch
2 times, most recently
from
d2127d5
to
597ece4
Compare
|
jenkins test api |
dillaman
reviewed
Mar 8, 2021
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
3 tasks
The new helper will pass one lock, this will be used by the inode lock feature, where the threads will be waiting and locked by the inode lock instead of the client_lock as default. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
Prepares for inode_lock features, this is safe because the inode will be held by the InodeRef in walk_dentry_result. Signed-off-by: Xiubo Li <xiubli@redhat.com>
This is useful because after the client_lock's unlock/lock gap, the lru object maybe already removed from the lru list by another thread. Will use this to check it, if so do nothing later. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Make it be possible that let the users to determine whether should the object inserted in the lru list. Signed-off-by: Xiubo Li <xiubli@redhat.com>
To break the `client_lock`, when we need to unlock it and lock it later, during the gap we must make sure that the `Cap` won't be released. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Use the spin lock to protect the ll_snap_ref, it should be lighter than the mutex lock. Signed-off-by: Xiubo Li <xiubli@redhat.com>
To break the `client_lock`, when we need to unlock it and lock it later, during the gap we must make sure that the `Request` won't be released. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Switch to use RefCountedObject to help manage the refence. And at the same time to make sure when breaking the `client_lock` and during the unlock and lock gap the Fh won't be released. Signed-off-by: Xiubo Li <xiubli@redhat.com>
To make the code more readable and there is no need to care about the members order when initlizeing them. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Later will inherit the Object from RefCountedObject, in which the get()/put() methods will conflict. Signed-off-by: Xiubo Li <xiubli@redhat.com>
This is parparing for the oc_lock support to make sure during the unlock/lock gap, the Object won't be released. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Later will inherit the Object from RefCountedObject, in which the get()/put() methods will conflict. Signed-off-by: Xiubo Li <xiubli@redhat.com>
This is parparing for the oc_lock support to make sure during the unlock/lock gap, the BufferHeadRef won't be released. Signed-off-by: Xiubo Li <xiubli@redhat.com>
Move the outside lock to Objectset class and add a global oc_lock for the ObjectCacher class to protect its members. This will allow to support the inode_lock in libcephfs to break the big client_lock. For ObjectCacherWriteback class, make the m_lock a dedicated private lock. NOTE: this patch will build failed without the inode lock patch followed, because both of them are big patches and to make them easier to review I just split it into to two. Fixes: https://tracker.ceph.com/issues/46688 Signed-off-by: Xiubo Li <xiubli@redhat.com>
This patch will add one dedicated mutex lock for each Inode
object. For some members in Inode class, such as the ino, snapid
and faked_ino, there is no need to be protected by the inode_lock
because they are readonly once the inode object is initilized.
The Inode::inode_locks could be recursive, but the order must be:
parent->inode_lock.lock()
{
child->inode_lock.lock()
{
grandson->inode_lock.lock()
{
...
}
grandson->inode_lock.unlock()
}
child->inode_lock.unlock()
}
parent->inode_lock.unlock()
For the Dentry class, it will also be protected by the Inode it
belong to. And in the Inode::inode_lock scope it allows to hold
the client_lock when needed, but not vise versa.
For the Fh class, it will also be protected by the inode lock which
Inode it's attached to.
To avoid different Inode object get the same lock id in lockdep,
it will use the vino to build a unique lock name.
There has one exception for Inode::iget() and Inode::iput(), they
must put under the client_lock scope. That also means for the
InodeRef type variable must be assigned under the client_lock scope
too. This will avoid one race between releasing the Inode memory
and getting the nref.
For the cct->_conf, there is no need to be protected under the
client lock, which is not that critical. And with PR#40028 fixing
there won't have the use-after-free or crash isssue any longer.
Fixes: https://tracker.ceph.com/issues/46688
Signed-off-by: Xiubo Li <xiubli@redhat.com>
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
|
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! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch will add one dedicated mutex lock for each Inode
object. For some members in Inode class, such as the ino, snapid
and faked_ino, there is no need to be protected by the inode_lock
because they are readonly once the inode object is initilized.
The Inode::inode_locks could be recursive, but the order must be:
For the Dentry class, it will also be protected by the Inode it
belong to. And in the Inode::inode_lock scope it allows to hold
the client_lock when needed, but not vise versa.
To avoid different Inode object get the same lock id in lockdep,
it will use the vino to build a unique lock name.
There has one exception for Inode::iget() and Inode::iput(), they
must put under the client_lock scope. That also means for the
InodeRef type variable must be assigned under the client_lock scope
too. This will avoid one race between releasing the Inode memory
and getting the nref.
1, for the XXXRef related patches, they are mainly to hold the instances to make sure they won't be released during the client_lock's unlock/lock gap.
2, for some members in the Inode/Fh, etc, there is no need to protect them by the inode_lock or client_lock, because they won't change once initilized in the whole life time, this could help simplify the inode_lock patch.
3, the inode_lock will protect almost all the members in the Inode class, except the nref inherited from the RefCountedObject and it will be protected by the client_lock, this could make sure that during the client_lock and inode_lock's unlock/lock gap, the nref will be consistent to fix #40028.
4, the minor cleaning patches for the Inode/Fh, etc classes, just to simplify the code to make it easier to read and do not need to care too much about the members order when initilizing them.
5, for the osdc related changes, mainly added the oc_lock support to protect the members in the ObjectCacher class, and move the outside locks, like the inode_lock, to the ObjectSet class.
6, I have tried to break the big patches into small ones as possible as I can, but for the last two ones, they are still very big.
7, and also please note that the lockdep will affect the performance because it will try to construct/destrcut the Backtrace very frequently, the more inode locks the poorer performance will it be. This won't be a problem, in the build release the lockdep will always be disabled. Just use the lockdep for developing or test cases to help debug the bugs.
Fixes: https://tracker.ceph.com/issues/46688
Signed-off-by: Xiubo Li xiubli@redhat.com
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 apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox