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

os/bluestore: fix deferred_aio deadlock #16051

Merged
merged 1 commit into from Jul 5, 2017

Conversation

Projects
None yet
4 participants
@liewegas
Member

liewegas commented Jun 30, 2017

We submit aios while holding deferred_lock, and we take deferred_lock in
the aio completion handler (of which there is only 1). That means that
if the aio queue fills up, the submitter will block with the lock while
the finisher won't be able to take the lock, progress, and drain completed
aios, leading to a deadlock.

Fix this by submiting aios without deferred_lock held. Instead, demote
to a deferred_submit_lock. Further, in the finisher, submit aios
via a finisher (this only happens when in deferred_aggressive mode which
is rare and not performance-sensitive).

Signed-off-by: Sage Weil sage@redhat.com

@liewegas liewegas requested a review from xiexingguo Jun 30, 2017

@xiexingguo

👍

@liewegas liewegas added the needs-qa label Jul 1, 2017

os/bluestore: fix deferred_aio deadlock
We submit aios while holding deferred_lock, and we take deferred_lock in
the aio completion handler (of which there is only 1).  That means that
if the aio queue fills up, the submitter will block with the lock while
the finisher won't be able to take the lock, progress, and drain completed
aios, leading to a deadlock.

Fix this by submiting aios *without* deferred_lock held.  Instead, demote
to a deferred_submit_lock.  Further, in the finisher, submit aios
via a finisher (this only happens when in deferred_aggressive mode which
is rare and not performance-sensitive).

Signed-off-by: Sage Weil <sage@redhat.com>
dout(20) << __func__ << " queuing async deferred_try_submit" << dendl;
finishers[0]->queue(new FunctionContext([&](int) {
deferred_try_submit();
}));

This comment has been minimized.

@tanghaodong25

tanghaodong25 Jul 1, 2017

Contributor

if we use async 'deferred_try_submit' here, we actually don't need introduce 'deferred_submit_lock'? Because we'll not block in the aio completion handle.

@tanghaodong25

tanghaodong25 Jul 1, 2017

Contributor

if we use async 'deferred_try_submit' here, we actually don't need introduce 'deferred_submit_lock'? Because we'll not block in the aio completion handle.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jul 1, 2017

Member
Member

liewegas commented Jul 1, 2017

@tanghaodong25

This comment has been minimized.

Show comment
Hide comment
@tanghaodong25

tanghaodong25 Jul 1, 2017

Contributor

@liewegas I understand now, thanks. What's more, seems introducing 'deferred_submit_lock' is better than throw the whole function of '_defer_aio_finisher' to a finisher queue.

Contributor

tanghaodong25 commented Jul 1, 2017

@liewegas I understand now, thanks. What's more, seems introducing 'deferred_submit_lock' is better than throw the whole function of '_defer_aio_finisher' to a finisher queue.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jul 3, 2017

Member

retest this please

Member

liewegas commented Jul 3, 2017

retest this please

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jul 4, 2017

Member

retest this please

Member

xiexingguo commented Jul 4, 2017

retest this please

@tchaikov tchaikov merged commit 33b68a4 into ceph:master Jul 5, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@liewegas liewegas deleted the liewegas:wip-20381 branch Jul 7, 2017

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