Skip to content
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

librbd: deadlock during cooperative exclusive lock transition #5319

Merged
38 commits merged into from Aug 30, 2015

Conversation

Projects
None yet
3 participants
@dillaman
Copy link
Contributor

dillaman commented Jul 22, 2015

http://tracker.ceph.com/issues/12235

Note: contains commits included in PR #5296 that can be rebased away once they merge.

@dillaman dillaman added this to the hammer milestone Jul 22, 2015

@ghost ghost changed the title librbd: deadlock during cooperative exclusive lock transition DNM: librbd: deadlock during cooperative exclusive lock transition Jul 28, 2015

@ghost ghost self-assigned this Jul 28, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 28, 2015

Marked DNM to ack that it is on top of PR #5280 and PR #5296

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 28, 2015

@dillaman could you please rebase now that #5280 is merged ?

@dillaman

This comment has been minimized.

Copy link
Contributor Author

dillaman commented Jul 28, 2015

@dachary should I just wait until #5296 merges as well?

dillaman and others added some commits Apr 8, 2015

WorkQueue: ContextWQ can now accept a return code
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit e5ffae5)
librbd: add work queue for op completions
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 21f990e)
common: add valgrind.h convenience wrapper
Conditionally support helgrind annotations if valgrind support is
enabled during the build.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 650ad32)
common: lockdep now support unregistering once destructed
librbd use of an image hierarchy resulted in lock names being
re-used and incorrectly analyzed.  librbd now uses unique lock
names per instance, but to prevent an unbounded growth of
tracked locks, we now remove lock tracking once a lock is
destructed.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 7c7df2c)
librados_test_stub: add support for flushing watches
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 6e400b9)
librados_test_stub: fix helgrind warnings
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit b65ae4b)
log: fix helgrind warnings regarding possible data race
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit c1e1445)
librbd: require callers to ObjectMap::aio_update to acquire lock
This is needed to allow an atomic compare and update operation
from the rebuild object map utility.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 2db758c)
librbd/AioRequest.h: fix UNINIT_CTOR
Fix for:

CID 1274319: Uninitialized scalar field (UNINIT_CTOR)
 uninit_member: Non-static class member m_object_state is not
 initialized in this constructor nor in any functions that it calls.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
(cherry picked from commit 48f18ea)
librbd: simplify AioRequest constructor parameters
Moved all parent overlap computation to within AioRequest so that
callers don't need to independently compute the overlap.  Also
removed the need to pass the snap_id for write operations since
it can only be CEPH_NOSNAP.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 4651597)
librbd: move copyup class method call to CopyupRequest
Move AbstractWrite's invocation of copyup to the CopyupRequest
class.  The AioRequest write path will now always create a
CopyupRequest, which will now append the actual write ops to the
copyup.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 7be3df6)
librbd: complete cache read in a new thread context
The ObjectCacher complete the read callback while still holding
the cache lock.  This introduces lock ordering issues which are
resolved by queuing the completion to execute in a clean (unlocked)
context.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 0024677)
librbd: AioCompletion shouldn't hold its lock during callback
The callback routine most likely will attempt to retrieve the result
code, which will result in a recursive lock attempt.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 3ad19ae)
librbd: give locks unique names to prevent false lockdep failures
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit c474ee4)
librbd: add AsyncRequest task enqueue helper method
In order to support the invariant that all state machine
callbacks occur without holding locks, transitions that
don't always involve a librados call should queue their
callback.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 218bc2d)
librbd: execute flush completion outside of cache_lock
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 5f157f2)
librbd: AsyncObjectThrottle should always hold owner_lock
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit c352bcd)
librbd: disable lockdep on AioCompletion
It is only used by clients and it causes a large slowdown
in performance due to the rate at which the lock is constructed/
destructed for each IO request.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 66e7464)
librbd: add object state accessor to ObjectMap
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
librbd: ObjectMap::aio_update can acquire snap_lock out-of-order
Detected during an fsx run where a refresh and CoR were occurring
concurrently.  The refresh held the snap_lock and was waiting on
the object_map_lock, while the CoR held object_map_lock and was
waiting for snap_lock.

Fixes: #11577
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 8cbd92b)
librbd: simplify state machine handling of exclusive lock
It is expected that all IO is flushed and all async ops are cancelled
prior to releasing the exclusive lock.  Therefore, replace handling of
lost exclusive locks in state machines with an assertion.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit d6b733d)
@ghost

This comment has been minimized.

Copy link

ghost commented Jul 28, 2015

@dillaman I'll try a new strategy for integration that should deal with dependencies. Simply by merging into the integration branch in the same order commits are merged in master. That should automatically merge in order and if one pull request is indeed including all the commits from another, that should just work nicely. So you can rebase this one, it won't be a loss of time: both can be tested at the same time even though they depend on each other.

@dillaman

This comment has been minimized.

Copy link
Contributor Author

dillaman commented Jul 28, 2015

@dachary sweet ... I'll take care of the rebases tonight.

@ghost ghost changed the title DNM: librbd: deadlock during cooperative exclusive lock transition ibrbd: deadlock during cooperative exclusive lock transition Jul 29, 2015

@ghost ghost changed the title ibrbd: deadlock during cooperative exclusive lock transition librbd: deadlock during cooperative exclusive lock transition Jul 29, 2015

@ghost ghost changed the title librbd: deadlock during cooperative exclusive lock transition DNM: librbd: deadlock during cooperative exclusive lock transition Jul 29, 2015

@dillaman dillaman force-pushed the wip-12235-hammer branch from 123726f to af67304 Jul 29, 2015

@dillaman dillaman changed the title DNM: librbd: deadlock during cooperative exclusive lock transition librbd: deadlock during cooperative exclusive lock transition Jul 29, 2015

@ghost ghost changed the title librbd: deadlock during cooperative exclusive lock transition DNM: librbd: deadlock during cooperative exclusive lock transition Jul 29, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 29, 2015

@dillaman when I try to merge this pull request on top of #5296, it conflicts. Which suggests the commits that look like they come from #5296 that are included in #5319 are not exactly the same really. Or I'm missing something ;-)

Auto-merging src/librbd/ImageWatcher.cc
CONFLICT (content): Merge conflict in src/librbd/ImageWatcher.cc
Auto-merging src/common/Mutex.cc
CONFLICT (content): Merge conflict in src/common/Mutex.cc
Automatic merge failed; fix conflicts and then commit the result.

dillaman added some commits May 7, 2015

librbd: don't hold owner_lock for write during flush
The various IO callback codepaths will attempt to take
the lock, which will result in deadlock since the flush
cannot complete.

Backport: hammer
Fixes: #11537
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

(cherry picked from commit 2b6d063)
librbd: retry lock requests periodically until acquired
If the exclusive lock owner acks the lock release request but crashes
before it actually releases the lock, the requestor will wait forever.
Therefore, after a certain timeout, retry the request again until it
succeeds.

Fixes: #11537
Backport: hammer
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 37c74e6)
tests: verify that librbd will periodically resend lock request
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit d2a1c22)
tests: new test for transitioning exclusive lock
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit f97ce46)
librados_test_stub: watcher id should be the instance id (gid)
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 3e1e561)
librbd: improve debugging output for ImageWatcher
Include the instance pointer so that different images
can be differentiated in the logs.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit b951a73)
common: Mutex shouldn't register w/ lockdep if disabled
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 879b8a7)
librbd: don't cancel request lock early
It's possible that a stale notice is received and will
be discarded after the request lock has been canceled.
This will stale the client.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit d9dd5c5)

@dillaman dillaman force-pushed the wip-12235-hammer branch from af67304 to 92272dd Jul 29, 2015

@dillaman

This comment has been minimized.

Copy link
Contributor Author

dillaman commented Jul 29, 2015

@dachary try this -- I rebuilt the branch

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 29, 2015

@dillaman this is what I see: http://paste2.org/fExaxzhm . FYI the reference is fetched with git fetch --force ceph +refs/pull/$pr/head:refs/remotes/ceph/pull/$pr/head

@dillaman

This comment has been minimized.

Copy link
Contributor Author

dillaman commented Jul 29, 2015

@dachary this is what I see: http://fpaste.org/249506/81929831/ (I have a slightly different PR ref)

@ghost ghost changed the title DNM: librbd: deadlock during cooperative exclusive lock transition librbd: deadlock during cooperative exclusive lock transition Jul 29, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 29, 2015

@dillaman all is well, I shot myself in the foot (the ref of this pr was not updated because it was not selected because it had DNM in the title). After fetching the updated pr things are as you describe. Thanks for your patience.

ghost pushed a commit that referenced this pull request Jul 29, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 30, 2015

ghost pushed a commit that referenced this pull request Aug 30, 2015

Loic Dachary
Merge pull request #5319 from ceph/wip-12235-hammer
librbd: deadlock during cooperative exclusive lock transition

Reviewed-by: Loic Dachary <ldachary@redhat.com>

@ghost ghost merged commit dc944fb into hammer Aug 30, 2015

@dillaman dillaman deleted the wip-12235-hammer branch Sep 7, 2015

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.