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: rbd: rbd_clone_copy_on_read ineffective with exclusive-lock #16124

Merged
merged 2 commits into from Aug 28, 2017

Conversation

Projects
None yet
3 participants
@smithfarm
Contributor

smithfarm commented Jul 5, 2017

librbd: acquire exclusive-lock during copy on read
Fixes: http://tracker.ceph.com/issues/18888
Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit 7dba531)

Conflicts:
    src/librbd/AioImageRequestWQ.h:
      - in master this file has morphed into src/librbd/io/ImageRequestWQ.h
      - jewel has AioImageRequest<ImageCtx> instead of ImageRequest<ImageCtx>
    src/librbd/image/RefreshRequest.cc:
      - rename image context element to "aio_work_queue" (from "io_work_queue")
        because jewel doesn't have de95d86
    src/test/librbd/image/test_mock_RefreshRequest.cc:
      - rename image context element to "aio_work_queue" (from "io_work_queue")
        because jewel doesn't have de95d86

@smithfarm smithfarm self-assigned this Jul 5, 2017

@smithfarm smithfarm added this to the jewel milestone Jul 5, 2017

@smithfarm smithfarm added bug fix core rbd and removed core labels Jul 5, 2017

@smithfarm smithfarm changed the title from jewel: rbd_clone_copy_on_read ineffective with exclusive-lock to [DNM] jewel: rbd_clone_copy_on_read ineffective with exclusive-lock Jul 5, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jul 5, 2017

Contributor
In file included from ../src/gmock/gtest/include/gtest/gtest.h:1929:0,
                 from ./test/librbd/test_fixture.h:7,
                 from ./test/librbd/test_mock_fixture.h:7,
                 from test/librbd/image/test_mock_RefreshRequest.cc:4:
test/librbd/image/test_mock_RefreshRequest.cc: In member function ‘virtual void librbd::image::TestMockImageRefreshRequest_ExclusiveLockWithCoR_Test::TestBody()’:
test/librbd/image/test_mock_RefreshRequest.cc:710:36: error: ‘class librbd::Operations<librbd::ImageCtx>’ has no member named ‘update_features’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_JOURNALING,
                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro ‘GTEST_ASSERT_’
   if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:166:3: note: in expansion of macro ‘GTEST_PRED_FORMAT2_’
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
   ^
../src/gmock/gtest/include/gtest/gtest.h:1993:3: note: in expansion of macro ‘ASSERT_PRED_FORMAT2’
   ASSERT_PRED_FORMAT2(::testing::internal:: \
   ^
../src/gmock/gtest/include/gtest/gtest.h:2011:32: note: in expansion of macro ‘GTEST_ASSERT_EQ’
 # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                ^
test/librbd/image/test_mock_RefreshRequest.cc:710:5: note: in expansion of macro ‘ASSERT_EQ’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_JOURNALING,
     ^
test/librbd/image/test_mock_RefreshRequest.cc:715:36: error: ‘class librbd::Operations<librbd::ImageCtx>’ has no member named ‘update_features’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_FAST_DIFF,
                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro ‘GTEST_ASSERT_’
   if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:166:3: note: in expansion of macro ‘GTEST_PRED_FORMAT2_’
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
   ^
../src/gmock/gtest/include/gtest/gtest.h:1993:3: note: in expansion of macro ‘ASSERT_PRED_FORMAT2’
   ASSERT_PRED_FORMAT2(::testing::internal:: \
   ^
../src/gmock/gtest/include/gtest/gtest.h:2011:32: note: in expansion of macro ‘GTEST_ASSERT_EQ’
 # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                ^
test/librbd/image/test_mock_RefreshRequest.cc:715:5: note: in expansion of macro ‘ASSERT_EQ’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_FAST_DIFF,
     ^
test/librbd/image/test_mock_RefreshRequest.cc:720:36: error: ‘class librbd::Operations<librbd::ImageCtx>’ has no member named ‘update_features’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_OBJECT_MAP,
                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro ‘GTEST_ASSERT_’
   if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:166:3: note: in expansion of macro ‘GTEST_PRED_FORMAT2_’
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
   ^
../src/gmock/gtest/include/gtest/gtest.h:1993:3: note: in expansion of macro ‘ASSERT_PRED_FORMAT2’
   ASSERT_PRED_FORMAT2(::testing::internal:: \
   ^
../src/gmock/gtest/include/gtest/gtest.h:2011:32: note: in expansion of macro ‘GTEST_ASSERT_EQ’
 # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                ^
test/librbd/image/test_mock_RefreshRequest.cc:720:5: note: in expansion of macro ‘ASSERT_EQ’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_OBJECT_MAP,
     ^
test/librbd/image/test_mock_RefreshRequest.cc:730:37: error: ‘expect_get_group’ was not declared in this scope
   expect_get_group(mock_image_ctx, 0);
                                     ^
make[4]: *** [test/librbd/image/unittest_librbd-test_mock_RefreshRequest.o] Error 1
Contributor

smithfarm commented Jul 5, 2017

In file included from ../src/gmock/gtest/include/gtest/gtest.h:1929:0,
                 from ./test/librbd/test_fixture.h:7,
                 from ./test/librbd/test_mock_fixture.h:7,
                 from test/librbd/image/test_mock_RefreshRequest.cc:4:
test/librbd/image/test_mock_RefreshRequest.cc: In member function ‘virtual void librbd::image::TestMockImageRefreshRequest_ExclusiveLockWithCoR_Test::TestBody()’:
test/librbd/image/test_mock_RefreshRequest.cc:710:36: error: ‘class librbd::Operations<librbd::ImageCtx>’ has no member named ‘update_features’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_JOURNALING,
                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro ‘GTEST_ASSERT_’
   if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:166:3: note: in expansion of macro ‘GTEST_PRED_FORMAT2_’
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
   ^
../src/gmock/gtest/include/gtest/gtest.h:1993:3: note: in expansion of macro ‘ASSERT_PRED_FORMAT2’
   ASSERT_PRED_FORMAT2(::testing::internal:: \
   ^
../src/gmock/gtest/include/gtest/gtest.h:2011:32: note: in expansion of macro ‘GTEST_ASSERT_EQ’
 # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                ^
test/librbd/image/test_mock_RefreshRequest.cc:710:5: note: in expansion of macro ‘ASSERT_EQ’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_JOURNALING,
     ^
test/librbd/image/test_mock_RefreshRequest.cc:715:36: error: ‘class librbd::Operations<librbd::ImageCtx>’ has no member named ‘update_features’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_FAST_DIFF,
                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro ‘GTEST_ASSERT_’
   if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:166:3: note: in expansion of macro ‘GTEST_PRED_FORMAT2_’
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
   ^
../src/gmock/gtest/include/gtest/gtest.h:1993:3: note: in expansion of macro ‘ASSERT_PRED_FORMAT2’
   ASSERT_PRED_FORMAT2(::testing::internal:: \
   ^
../src/gmock/gtest/include/gtest/gtest.h:2011:32: note: in expansion of macro ‘GTEST_ASSERT_EQ’
 # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                ^
test/librbd/image/test_mock_RefreshRequest.cc:715:5: note: in expansion of macro ‘ASSERT_EQ’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_FAST_DIFF,
     ^
test/librbd/image/test_mock_RefreshRequest.cc:720:36: error: ‘class librbd::Operations<librbd::ImageCtx>’ has no member named ‘update_features’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_OBJECT_MAP,
                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro ‘GTEST_ASSERT_’
   if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                    ^
../src/gmock/gtest/include/gtest/gtest_pred_impl.h:166:3: note: in expansion of macro ‘GTEST_PRED_FORMAT2_’
   GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
   ^
../src/gmock/gtest/include/gtest/gtest.h:1993:3: note: in expansion of macro ‘ASSERT_PRED_FORMAT2’
   ASSERT_PRED_FORMAT2(::testing::internal:: \
   ^
../src/gmock/gtest/include/gtest/gtest.h:2011:32: note: in expansion of macro ‘GTEST_ASSERT_EQ’
 # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                ^
test/librbd/image/test_mock_RefreshRequest.cc:720:5: note: in expansion of macro ‘ASSERT_EQ’
     ASSERT_EQ(0, ictx->operations->update_features(RBD_FEATURE_OBJECT_MAP,
     ^
test/librbd/image/test_mock_RefreshRequest.cc:730:37: error: ‘expect_get_group’ was not declared in this scope
   expect_get_group(mock_image_ctx, 0);
                                     ^
make[4]: *** [test/librbd/image/unittest_librbd-test_mock_RefreshRequest.o] Error 1
@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jul 5, 2017

Contributor

@dillaman Could you suggest a fix for this build failure, caused by the absence of 93cf63f in jewel? (If the PR is not salvageable in its current form, feel free to do whatever you like with it.)

Contributor

smithfarm commented Jul 5, 2017

@dillaman Could you suggest a fix for this build failure, caused by the absence of 93cf63f in jewel? (If the PR is not salvageable in its current form, feel free to do whatever you like with it.)

@@ -682,6 +687,58 @@ TEST_F(TestMockImageRefreshRequest, JournalDisabledByPolicy) {
ASSERT_EQ(0, ctx.wait());
}
TEST_F(TestMockImageRefreshRequest, ExclusiveLockWithCoR) {
REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);

This comment has been minimized.

@dillaman

dillaman Jul 6, 2017

Contributor

Add REQUIRE(!is_feature_enabled(RBD_FEATURE_OBJECT_MAP | RBD_FEATURE_FAST_DIFF | RBD_FEATURE_JOURNALING)) here

@dillaman

dillaman Jul 6, 2017

Contributor

Add REQUIRE(!is_feature_enabled(RBD_FEATURE_OBJECT_MAP | RBD_FEATURE_FAST_DIFF | RBD_FEATURE_JOURNALING)) here

Show outdated Hide outdated src/test/librbd/image/test_mock_RefreshRequest.cc
Show outdated Hide outdated src/test/librbd/image/test_mock_RefreshRequest.cc
tests: librbd: adapt test_mock_RefreshRequest for jewel
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jul 6, 2017

Contributor

Jenkins re-test this please

Contributor

smithfarm commented Jul 6, 2017

Jenkins re-test this please

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jul 6, 2017

Contributor

@dillaman Thanks, that worked.

Contributor

smithfarm commented Jul 6, 2017

@dillaman Thanks, that worked.

@smithfarm smithfarm changed the title from [DNM] jewel: rbd_clone_copy_on_read ineffective with exclusive-lock to jewel: rbd: rbd_clone_copy_on_read ineffective with exclusive-lock Jul 6, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jul 12, 2017

Contributor

test this please

Contributor

smithfarm commented Jul 12, 2017

test this please

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 28, 2017

Contributor

@dillaman This passed an rbd suite at http://tracker.ceph.com/issues/20613#note-31 (there was 1 failure that passed on rerun).

The rados run is putting up a bit of a fight; there are a couple failures but they don't appear to be reliably reproducible or related to rbd (or this PR, but what do I know?): http://tracker.ceph.com/issues/20613#note-25

I also ran three different upgrade suites; again, some failures failures that were either not reproducible or could be "hand-waved away".

OK to merge?

Contributor

smithfarm commented Aug 28, 2017

@dillaman This passed an rbd suite at http://tracker.ceph.com/issues/20613#note-31 (there was 1 failure that passed on rerun).

The rados run is putting up a bit of a fight; there are a couple failures but they don't appear to be reliably reproducible or related to rbd (or this PR, but what do I know?): http://tracker.ceph.com/issues/20613#note-25

I also ran three different upgrade suites; again, some failures failures that were either not reproducible or could be "hand-waved away".

OK to merge?

@dillaman

lgtm

@smithfarm smithfarm merged commit 9d1b08e into ceph:jewel Aug 28, 2017

4 checks passed

Docs: build check OK - docs built
Details
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

@smithfarm smithfarm deleted the smithfarm:wip-19174-jewel branch Aug 28, 2017

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