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.

Sign up for GitHub

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

Jump to bottom

pacific: librbd: fix regressions in ObjectListSnapsRequest #54860

Merged
merged 8 commits into from Dec 12, 2023
Merged

pacific: librbd: fix regressions in ObjectListSnapsRequest #54860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
df8ecf3
librados/snap_set_diff: set end_size only if end object exists
idryomov Nov 11, 2023
06fedbf
test/librbd: make ListSnapsWholeObject actually test stuff
idryomov Nov 19, 2023
10a45d3
librbd: fix LIST_SNAPS_FLAG_WHOLE_OBJECT behavior
idryomov Nov 27, 2023
d2ee564
librbd: fix read_whole_object handling in ObjectListSnapsRequest
idryomov Nov 27, 2023
825bda9
test/librbd: add DiffIterateTest.DiffIterateDeterministic{,PP}
idryomov Nov 29, 2023
6343800
test/librbd: drop TestLibRBD.SnapDiff
idryomov Dec 1, 2023
ab7e538
test/librbd: drop DiffIterateTest.DiffIterateRegression6926
idryomov Dec 1, 2023
bd0cbbf
test/librbd: close image in DiffIterateDeterministic
idryomov Dec 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librados/snap_set_diff.cc
View file
Open in desktop
Expand Up @@ -75,7 +75,6 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set,
saw_start = true;
}

*end_size = r->size;
if (end < a) {
ldout(cct, 20) << " past end " << end << ", end object does not exist" << dendl;
*end_exists = false;
Expand All @@ -87,6 +86,7 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set,
}
if (end <= b) {
ldout(cct, 20) << " end" << dendl;
*end_size = r->size;
*end_exists = true;
*clone_end_snap_id = b;
break;
Expand Down
15 changes: 8 additions & 7 deletions src/librbd/io/ObjectRequest.cc
View file
Open in desktop
Expand Up @@ -834,16 +834,17 @@ void ObjectListSnapsRequest<I>::handle_list_snaps(int r) {
end_snap_id, &diff, &end_size, &exists,
&clone_end_snap_id, &read_whole_object);

if (read_whole_object ||
(!diff.empty() &&
((m_list_snaps_flags & LIST_SNAPS_FLAG_WHOLE_OBJECT) != 0))) {
if (read_whole_object) {
ldout(cct, 1) << "need to read full object" << dendl;
diff.clear();
diff.insert(0, image_ctx->layout.object_size);
exists = true;
end_size = image_ctx->layout.object_size;
clone_end_snap_id = end_snap_id;
} else if (!exists) {
end_size = 0;
} else if ((m_list_snaps_flags & LIST_SNAPS_FLAG_WHOLE_OBJECT) != 0 &&
!diff.empty()) {
ldout(cct, 20) << "expanding diff from " << diff << dendl;
diff.clear();
diff.insert(0, image_ctx->layout.object_size);
}

if (exists) {
Expand Down Expand Up @@ -884,7 +885,7 @@ void ObjectListSnapsRequest<I>::handle_list_snaps(int r) {
<< "end_size=" << end_size << ", "
<< "prev_end_size=" << prev_end_size << ", "
<< "exists=" << exists << ", "
<< "whole_object=" << read_whole_object << dendl;
<< "read_whole_object=" << read_whole_object << dendl;

// check if object exists prior to start of incremental snap delta so that
// we don't DNE the object if no additional deltas exist
Expand Down
115 changes: 111 additions & 4 deletions src/test/librbd/io/test_mock_ObjectRequest.cc
View file
Open in desktop
Expand Up @@ -1919,7 +1919,7 @@ TEST_F(TestMockIoObjectRequest, ListSnapsWholeObject) {
ASSERT_EQ(0, open_image(m_image_name, &ictx));

MockTestImageCtx mock_image_ctx(*ictx);
mock_image_ctx.parent = &mock_image_ctx;
mock_image_ctx.snaps = {3};

InSequence seq;

Expand All @@ -1930,13 +1930,120 @@ TEST_F(TestMockIoObjectRequest, ListSnapsWholeObject) {
clone_info.cloneid = 3;
clone_info.snaps = {3};
clone_info.overlap = std::vector<std::pair<uint64_t,uint64_t>>{{0, 1}};
clone_info.size = 4194304;
clone_info.size = mock_image_ctx.layout.object_size;
snap_set.clones.push_back(clone_info);

clone_info.cloneid = CEPH_NOSNAP;
clone_info.snaps = {};
clone_info.overlap = {};
clone_info.size = 4194304;
clone_info.size = mock_image_ctx.layout.object_size;
snap_set.clones.push_back(clone_info);

expect_list_snaps(mock_image_ctx, snap_set, 0);

{
SnapshotDelta snapshot_delta;
C_SaferCond ctx;
auto req = MockObjectListSnapsRequest::create(
&mock_image_ctx, 0, {{0, mock_image_ctx.layout.object_size - 1}},
{3, CEPH_NOSNAP}, 0, {}, &snapshot_delta, &ctx);
req->send();
ASSERT_EQ(0, ctx.wait());

SnapshotDelta expected_snapshot_delta;
expected_snapshot_delta[{CEPH_NOSNAP,CEPH_NOSNAP}].insert(
1, mock_image_ctx.layout.object_size - 2,
{SPARSE_EXTENT_STATE_DATA, mock_image_ctx.layout.object_size - 2});
EXPECT_EQ(expected_snapshot_delta, snapshot_delta);
}

expect_list_snaps(mock_image_ctx, snap_set, 0);

{
SnapshotDelta snapshot_delta;
C_SaferCond ctx;
auto req = MockObjectListSnapsRequest::create(
&mock_image_ctx, 0, {{0, mock_image_ctx.layout.object_size - 1}},
{3, CEPH_NOSNAP}, LIST_SNAPS_FLAG_WHOLE_OBJECT, {}, &snapshot_delta,
&ctx);
req->send();
ASSERT_EQ(0, ctx.wait());

SnapshotDelta expected_snapshot_delta;
expected_snapshot_delta[{CEPH_NOSNAP,CEPH_NOSNAP}].insert(
0, mock_image_ctx.layout.object_size - 1,
{SPARSE_EXTENT_STATE_DATA, mock_image_ctx.layout.object_size - 1});
EXPECT_EQ(expected_snapshot_delta, snapshot_delta);
}
}

TEST_F(TestMockIoObjectRequest, ListSnapsWholeObjectEndSize) {
librbd::ImageCtx *ictx;
ASSERT_EQ(0, open_image(m_image_name, &ictx));

MockTestImageCtx mock_image_ctx(*ictx);
mock_image_ctx.snaps = {3};

InSequence seq;

librados::snap_set_t snap_set;
snap_set.seq = 3;
librados::clone_info_t clone_info;

clone_info.cloneid = CEPH_NOSNAP;
clone_info.snaps = {};
clone_info.overlap = {};
// smaller than object extent (i.e. the op) to test end_size handling
clone_info.size = mock_image_ctx.layout.object_size - 2;
snap_set.clones.push_back(clone_info);

expect_list_snaps(mock_image_ctx, snap_set, 0);

{
SnapshotDelta snapshot_delta;
C_SaferCond ctx;
auto req = MockObjectListSnapsRequest::create(
&mock_image_ctx, 0, {{0, mock_image_ctx.layout.object_size - 1}},
{4, CEPH_NOSNAP}, 0, {}, &snapshot_delta, &ctx);
req->send();
ASSERT_EQ(0, ctx.wait());

EXPECT_TRUE(snapshot_delta.empty());
}

expect_list_snaps(mock_image_ctx, snap_set, 0);

{
SnapshotDelta snapshot_delta;
C_SaferCond ctx;
auto req = MockObjectListSnapsRequest::create(
&mock_image_ctx, 0, {{0, mock_image_ctx.layout.object_size - 1}},
{4, CEPH_NOSNAP}, LIST_SNAPS_FLAG_WHOLE_OBJECT, {}, &snapshot_delta,
&ctx);
req->send();
ASSERT_EQ(0, ctx.wait());

EXPECT_TRUE(snapshot_delta.empty());
}
}

TEST_F(TestMockIoObjectRequest, ListSnapsNoSnapsInSnapSet) {
librbd::ImageCtx *ictx;
ASSERT_EQ(0, open_image(m_image_name, &ictx));

MockTestImageCtx mock_image_ctx(*ictx);
mock_image_ctx.snaps = {3};

InSequence seq;

librados::snap_set_t snap_set;
snap_set.seq = 3;
librados::clone_info_t clone_info;

clone_info.cloneid = 3;
clone_info.snaps = {};
clone_info.overlap = {};
clone_info.size = 0;
snap_set.clones.push_back(clone_info);

expect_list_snaps(mock_image_ctx, snap_set, 0);
Expand All @@ -1953,7 +2060,7 @@ TEST_F(TestMockIoObjectRequest, ListSnapsWholeObject) {
expected_snapshot_delta[{CEPH_NOSNAP,CEPH_NOSNAP}].insert(
0, mock_image_ctx.layout.object_size - 1,
{SPARSE_EXTENT_STATE_DATA, mock_image_ctx.layout.object_size - 1});
ASSERT_EQ(expected_snapshot_delta, snapshot_delta);
EXPECT_EQ(expected_snapshot_delta, snapshot_delta);
}

} // namespace io
Expand Down