New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: simplify remove_cap interface #12161

Merged
merged 2 commits into from Jan 9, 2017

Conversation

Projects
None yet
4 participants
@jcsp
Contributor

jcsp commented Nov 23, 2016

All this complexity only existed to handle the
case where trim wanted to iterate over an xlist
without getting disturbed by a deletion. We
can simply increment the iterator before
maybe-deleting the Cap.

Signed-off-by: John Spray john.spray@redhat.com

@jcsp

This comment has been minimized.

Contributor

jcsp commented Nov 23, 2016

This is a followup to #12145

@ukernel

This comment has been minimized.

Member

ukernel commented Nov 23, 2016

this code is from kernel client (which is thread safe). If we want to mulththread the client in the future. It's better to keep this code

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented Nov 23, 2016

This looks good to me code-wise, but can we structure it as reverts of the commits in question?

Alternatively we can do something else to satisfy the clang static analyzer, although I don't think we need to bother with that until we actually have a multithreaded client...

@jcsp

This comment has been minimized.

Contributor

jcsp commented Nov 23, 2016

@ukernel I don't think the existing code gave us any real thread safety, setting the s_cap_iterator to protect against deletion would race with any calls that might have deleted it

@batrick

This comment has been minimized.

Member

batrick commented Nov 23, 2016

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

@jcsp

This comment has been minimized.

Contributor

jcsp commented Nov 29, 2016

This has been updated to do the revert first

John Spray added some commits Nov 29, 2016

John Spray
Revert "client: trim_caps() do not dereference cap if it's removed"
We will simplify this another way.

This reverts commit e9fbe39.

Signed-off-by: John Spray <john.spray@redhat.com>
John Spray
client: simplify remove_cap interface
All this complexity only existed to handle the
case where trim wanted to iterate over an xlist
without getting disturbed by a deletion.  We
can simply increment the iterator before
maybe-deleting the Cap.

Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp

This comment has been minimized.

Contributor

jcsp commented Dec 7, 2016

I'm concerned that a test run including this PR had the following crash:

Assertion: /build/ceph-11.0.2-2290-g93eec04/src/client/Inode.cc: 357: FAILED assert(_ref >= 0)
ceph version 11.0.2-2290-g93eec04 (93eec04382359a5c052dca589969920f6fcf132e)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x80) [0x55d14de890f0]
 2: (Inode::_put(int)+0x31c) [0x55d14ddd93ec]
 3: (Client::put_inode(Inode*, int)+0x135) [0x55d14dd5be65]
 4: (std::_Rb_tree<snapid_t, std::pair<snapid_t const, CapSnap>, std::_Select1st<std::pair<snapid_t const, CapSnap> >, std::less<snapid_t>, std::allocator<std::pair<snapid_t const, CapSnap> > >::_M_erase(std::_Rb_tree_node<std::pair<snapid_t const, CapSnap> >*)+0x7a) [0x55d14ddd16ea]
 5: (Inode::~Inode()+0x4c4) [0x55d14dddde44]
 6: (Client::put_inode(Inode*, int)+0x333) [0x55d14dd5c063]
 7: (std::_Rb_tree<snapid_t, std::pair<snapid_t const, CapSnap>, std::_Select1st<std::pair<snapid_t const, CapSnap> >, std::less<snapid_t>, std::allocator<std::pair<snapid_t const, CapSnap> > >::_M_erase(std::_Rb_tree_node<std::pair<snapid_t const, CapSnap> >*)+0x7a) [0x55d14ddd16ea]
 8: (std::_Rb_tree<snapid_t, std::pair<snapid_t const, CapSnap>, std::_Select1st<std::pair<snapid_t const, CapSnap> >, std::less<snapid_t>, std::allocator<std::pair<snapid_t const, CapSnap> > >::erase(snapid_t const&)+0x79) [0x55d14ddd1789]
 9: (Client::handle_cap_flushsnap_ack(MetaSession*, Inode*, MClientCaps*)+0x3e7) [0x55d14dd60cd7]
 10: (Client::handle_caps(MClientCaps*)+0x3a1) [0x55d14dd963b1]
 11: (Client::ms_dispatch(Message*)+0x6ab) [0x55d14ddaa65b]
 12: (DispatchQueue::entry()+0xf4a) [0x55d14e0a0bca]
 13: (DispatchQueue::DispatchThread::entry()+0xd) [0x55d14df1879d]
 14: (()+0x770a) [0x7f963f3aa70a]
 15: (clone()+0x6d) [0x7f963da7982d]

It's possible that it's an unrelated intermittent failure but need more confidence.
http://pulpito.ceph.com/jspray-2016-12-07_00:24:14-fs-wip-jcsp-testing-20161206-distro-basic-smithi/612500

@jcsp jcsp merged commit 8a7ad3d into ceph:master Jan 9, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@jcsp jcsp deleted the jcsp:wip-remove-cap branch Jan 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment