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

jewel: librbd: restore journal access when force disabling mirroring #11916

Merged
1 commit merged into from Nov 14, 2016

Conversation

trociny
Copy link
Contributor

@trociny trociny commented Nov 11, 2016

If mirroring is force disabled on a demoted image, the journal was
being left in an inconsistent ownership state.

Fixes: http://tracker.ceph.com/issues/17767
Signed-off-by: Mykola Golub mgolub@mirantis.com

@trociny trociny added this to the jewel milestone Nov 11, 2016
@trociny trociny changed the title librbd: restore journal access when force disabling mirroring jewel: librbd: restore journal access when force disabling mirroring Nov 11, 2016
@trociny
Copy link
Contributor Author

trociny commented Nov 11, 2016

@dillaman I tested this using this test:

trociny@38b2d40

Note, the test uses workarounds for the following issues found:

  1. Removing a demoted image will fiail with the following error until you disable mirroring first (both master and jewel):
2016-11-11 16:18:20.454915 7fba651b8700 10 librbd::exclusive_lock::AcquireRequest: handle_open_journal: r=0
2016-11-11 16:18:20.454916 7fba651b8700 10 librbd::exclusive_lock::AcquireRequest: send_allocate_journal_tag
2016-11-11 16:18:20.454918 7fba651b8700 -1 librbd::journal::StandardPolicy: local image not promoted
2016-11-11 16:18:20.454921 7fba651b8700 10 librbd::exclusive_lock::AcquireRequest: handle_allocate_journal_tag: r=-1
2016-11-11 16:18:20.454923 7fba651b8700 -1 librbd::exclusive_lock::AcquireRequest: failed to allocate journal tag: (1) Operation not permitted
  1. If one do mirror_image_demote and mirror_image_disable without reopening the image between these operation this assertion in DisableRequest::send_promote_image is triggered (only master):
  // Not primary -- shouldn't have the journal open
  assert(m_image_ctx->journal == nullptr);

I am planning to report these issues and work on the fix.

@dillaman
Copy link

@trociny For (1), that is the correct behavior I would think -- we don't want to allow users to delete non-primary images easily. For (2), that does seem like an issue that the demote step didn't release the exclusive lock (which would close the journal).

@ghost ghost changed the base branch from jewel to jewel-next November 11, 2016 15:27
@ghost ghost assigned ghost and dillaman Nov 11, 2016
@ghost ghost changed the title jewel: librbd: restore journal access when force disabling mirroring DNM: jewel: librbd: restore journal access when force disabling mirroring Nov 11, 2016
@ghost
Copy link

ghost commented Nov 11, 2016

@trociny setting DNM while it is being discussed. It would be great if you could shortly explain in the commit message why this back has to be a unique snowflake (as Jason puts it ;-).

If mirroring is force disabled on a demoted image, the journal was
being left in an inconsistent ownership state.

This is a direct commit to jewel as the fix in the master was
against the newly added async version of mirror disable, which is
not going to be merged to jewel.

Fixes: http://tracker.ceph.com/issues/17767
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
@trociny
Copy link
Contributor Author

trociny commented Nov 11, 2016

@dachary The commit message has been updated.

As for DNM status, actually the discussion we had with Jason was about some other (not related) issues I had observed testing this, and it turned out that one of them was an expected behavior and another one was triggered only in master.

Still, I'd like to see Jason's approve before the merge.

And it needs teuthology run. @dachary Are you going to run teuthology for backport PRs soon? If you are, could you please test this too? If you are not, I can run the tests myself.

@ghost
Copy link

ghost commented Nov 11, 2016

@trociny a run finished yesterday and since this is the only PR that was not included in it, it's likely the next run will be in one or two weeks. Feel free to run the relevant jobs earlier if you'd like.

@ghost
Copy link

ghost commented Nov 11, 2016

jenkins test this please (general jenkins failure)

@ghost ghost changed the title DNM: jewel: librbd: restore journal access when force disabling mirroring jewel: librbd: restore journal access when force disabling mirroring Nov 14, 2016
@ghost ghost unassigned dillaman Nov 14, 2016
@dillaman
Copy link

lgtm

@ghost ghost merged commit 1bd8c04 into ceph:jewel-next Nov 14, 2016
@trociny trociny deleted the wip-17767-jewel branch November 15, 2016 14:10
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants