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: new exclusive lock release journal event #7818

Closed
wants to merge 4 commits into from

Conversation

sahithi-rv
Copy link
Contributor

Signed-off-by: Sahithi R V sahithi.rv1@gmail.com

@sahithi-rv
Copy link
Contributor Author

The event created is not yet fully functional.

@dillaman dillaman changed the title librbd: subtask #14414 librbd: new exclusive lock release journal event Feb 26, 2016
@dillaman
Copy link

Can you put the tracker # in the commit message:

Fixes: #14414

@@ -131,9 +132,31 @@ Context *ReleaseRequest<I>::handle_flush_notifies(int *ret_val) {

assert(*ret_val == 0);
send_close_journal();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line -- we need to append the event before closing the journal

@dillaman
Copy link

Also, please add a test case for this new event. See https://github.com/ceph/ceph/blob/master/src/test/librbd/journal/test_mock_Replay.cc#L454 for an example.

@jdurgin
Copy link
Member

jdurgin commented Mar 2, 2016

Looks like this needs rebasing after some recent merges. Could you put the "Fixes: #XYZ" on a separate line in the commit messages, like in: 5d41735 ? It helps keep the first line shorter as well

@sahithi-rv sahithi-rv force-pushed the 14414 branch 4 times, most recently from 1ee95f9 to bf37151 Compare March 2, 2016 12:36
@@ -129,23 +130,50 @@ Context *ReleaseRequest<I>::handle_flush_notifies(int *ret_val) {
CephContext *cct = m_image_ctx.cct;
ldout(cct, 10) << __func__ << dendl;

assert(*ret_val == 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a valuable assert statement

@dillaman dillaman added this to the jewel milestone Mar 8, 2016
@sahithi-rv sahithi-rv force-pushed the 14414 branch 2 times, most recently from 0f7c52b to 8acb684 Compare March 9, 2016 20:52
@sahithi-rv
Copy link
Contributor Author

@dillaman can you please test it now?

@dillaman
Copy link

@sahithi-rv You can just check the github build status at the bottom of the PR. It says you need to rebase your PR to the current master branch.

@dillaman
Copy link

Getting closer -- the unit tests are crashing. You can repeat this locally by running "RBD_FEATURES=109 ./unittest_librbd" from the 'src' directory.

@@ -253,6 +254,18 @@ struct FlattenEvent : public OpEventBase {
using OpEventBase::dump;
};

struct ExclusiveLockReleaseEvent : public OpEventBase {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to inherit from OpEventBase -- this isn't a maintenance operation. As a result, you won't need an op_tid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what do I do about encode, decode, dump member functions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement them as empty methods just like UnknownEvent

@@ -239,6 +240,7 @@ class Journal {
};

Journaler *m_journaler;
uint64_t m_tag_id;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to be used -- perhaps pulled in from a bad rebase.

@sahithi-rv
Copy link
Contributor Author

I do not get error when I run RBD_FEATURES=61 ./unittest_librbd , but i get segfault when I run run-rbd-unit-tests.sh . But I think both are doing pretty much the same thing?

@dillaman
Copy link

RBD_FEATURES=61 doesn't test any journaling-related functionality. run-rbd-unit-tests.sh runs unittest_librbd with several combinations for RBD_FEATURES -- including RBD_FEATURES=109 which enables support for journaling.

@dillaman
Copy link

Which RBD_FEATURES=x causes the crash?

@sahithi-rv
Copy link
Contributor Author

x = 61 and x=109.
When I use m_image_ctx instead of m_journal ,x=61 or x=109 , donot throw errors but run-rbd-unit-tests throws segfault.

@dillaman
Copy link

Now I am confused since you said 109 doesn't have any issue.

@sahithi-rv
Copy link
Contributor Author

Ok. sorry I have used m_image_ctx in ReleaseRequest first and tested. then I get not errors with running unittest_librbd with RBD_FEATURES= 61, 109. But i got Segfault when I run run-rbd-unit-tests and i found that $i = 61 and $i -109 give error.

@sahithi-rv
Copy link
Contributor Author

When I use m_journal in ReleaseRequest.cc, and run RBD_FEATURES=61 ./unittest_librbd I get this error
err

@sahithi-rv
Copy link
Contributor Author

Now I tried to free mock_journal in every test in test_mock_ExclusiveLock and test_mock_ReleaseRequest. This does not give me errors as shown above. But it still throws segfault.
Is there a way to debug segfault, here?

@dillaman
Copy link

RBD_FEATURES=109 libtool e gdb ./unittest_librbd

Fixes: ceph#14414

Signed-off-by: Sahithi R V <sahithi.rv1@gmail.com>
Fixes: ceph#14414

Signed-off-by: Sahithi R V <sahithi.rv1@gmail.com>
Fixes: ceph#14414

Signed-off-by: Sahithi R V <sahithi.rv1@gmail.com>
@sahithi-rv
Copy link
Contributor Author

When I run RBD_FEATURES=109 ./unittest_librbd, the tests run successfully till "TestMockImageRefreshRequest.SuccessV2" , but is stuck at "TestMockImageRefreshRequest.SuccessSnapshotV2"

@sahithi-rv
Copy link
Contributor Author

What does it mean append_lock_release_event(_) is not called? I get this error:
err

@dillaman
Copy link

dillaman commented Apr 4, 2016

@sahithi-rv It's hard for me to say since the code available on github doesn't match up with that error message. It appears like in your version of the code, for the "UnlockError" test, you had an expectation for "expect_append_lock_release_event", but it was never called before the lock was released.

Fixes: ceph#14414

Signed-off-by: Sahithi R V <sahithi.rv1@gmail.com>
@dillaman dillaman modified the milestones: kraken, jewel Apr 11, 2016
@jdurgin jdurgin removed this from the kraken milestone May 10, 2016
@dillaman
Copy link

Closing this PR -- the Jewel version uses journal tag epochs to track the acquire/release of the exclusive lock.

@dillaman dillaman closed this Jun 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants