client: avoid second lock on client_lock #21554
Conversation
|
this is wrong. ll_get_inodeno() unlock client_lock when it returns |
|
@ukernel I guess you meant that ll_get_inodeno DOES not unlock when it returns? You are right. It would be better to take lock in ll_get_stripe_osd and not in ll_get_inodeno as it will never release the lock. |
src/client/Client.cc
Outdated
| @@ -12422,8 +12422,6 @@ int Client::ll_file_layout(Fh *fh, file_layout_t *layout) | |||
| int Client::ll_get_stripe_osd(Inode *in, uint64_t blockno, | |||
| file_layout_t* layout) | |||
| { | |||
| Mutex::Locker lock(client_lock); | |||
|
|
|||
| inodeno_t ino = ll_get_inodeno(in); | |||
There was a problem hiding this comment.
I think this can get directly from in. in->ino.
There was a problem hiding this comment.
In the case, no one is calling this function ll_get_inodeno ( and consecutively) _get_inodeno except ll_get_stripe_osd. May be we can remove these functions?
There was a problem hiding this comment.
Client::_ll_forget() is calling _get_inodeno()
There was a problem hiding this comment.
@joscollin in _ll_forget as well we can have inodeno_t ino = in->ino. But I am not aware of design behind using _get_inodeno().
Besides that, I see in some the functions in Client.cc, we have taken client_lock but don't unlock it before leaving the function. For example, ll_get_stripe_osd, ll_readlink, ll_get_snapid. This seems like error. If that's case I can submit a another patch.
There was a problem hiding this comment.
See how Mutex::Locker is defined and where in memory those objects are created.
supriti
changed the title
|
@joscollin thanks for review and your answers. I have updated PR addressing your comments. |
batrick
changed the title
joscollin
changed the title
|
retest this please |
|
Updated the commit message with tracker issue. |
There was a problem hiding this comment.
This is a good read: https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#tag-the-commit
|
Please fix the commit message to be:
The |
Avoid a second nested lock on client lock. As a result, drop unused function ll_get_inodeno(). Fixes: https://tracker.ceph.com/issues/23815 Signed-off-by: Supriti Singh <supriti.singh@suse.com>
|
@batrick sorry I was not aware about it. Updated the comment now. |
|
retest this please |
|
retest this please |
| // low-level interface v2 | ||
| inodeno_t ll_get_inodeno(Inode *in) { | ||
| Mutex::Locker lock(client_lock); | ||
| return _get_inodeno(in); |
There was a problem hiding this comment.
Might not hurt to get rid of the _get_inodeno wrapper too, while you're in there. Seems rather silly to have a private method for fetching a single value out of the class.
* refs/pull/21554/head: client: avoid second lock on client_lock Reviewed-by: Jos Collin <jcollin@redhat.com> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> Reviewed-by: Jeff Layton <jlayton@redhat.com>
Drop _get_inodeno() as per the comment in ceph#21554. Signed-off-by: Jos Collin <jcollin@redhat.com>
Avoid a second nested lock on client lock. As a result, drop unused function
ll_get_inodeno().Fixes: http://tracker.ceph.com/issues/23815
Signed-off-by: Supriti Singh supriti.singh@suse.com