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

rbd-mirror: deleting a snapshot during sync can result in read errors #13568

Merged
merged 3 commits into from Feb 23, 2017
Merged
Diff settings

Always

Just for now

@@ -17,7 +17,7 @@
void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set,
librados::snap_t start, librados::snap_t end,
interval_set<uint64_t> *diff, uint64_t *end_size,
bool *end_exists)
bool *end_exists, librados::snap_t *clone_end_snap_id)
{
ldout(cct, 10) << "calc_snap_set_diff start " << start << " end " << end
<< ", snap_set seq " << snap_set.seq << dendl;
@@ -26,6 +26,7 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set,
diff->clear();
*end_size = 0;
*end_exists = false;
*clone_end_snap_id = 0;

for (vector<librados::clone_info_t>::const_iterator r = snap_set.clones.begin();
r != snap_set.clones.end();
@@ -79,6 +80,7 @@ void calc_snap_set_diff(CephContext *cct, const librados::snap_set_t& snap_set,
if (end <= b) {
ldout(cct, 20) << " end" << dendl;
*end_exists = true;
*clone_end_snap_id = b;
break;
}

@@ -12,6 +12,6 @@ void calc_snap_set_diff(CephContext *cct,
const librados::snap_set_t& snap_set,
librados::snap_t start, librados::snap_t end,
interval_set<uint64_t> *diff, uint64_t *end_size,
bool *end_exists);
bool *end_exists, librados::snap_t *clone_end_snap_id);

#endif
@@ -125,9 +125,10 @@ class C_DiffObject : public Context {
interval_set<uint64_t> diff;
uint64_t end_size;
bool end_exists;
librados::snap_t clone_end_snap_id;
calc_snap_set_diff(cct, m_snap_set, m_diff_context.from_snap_id,
m_diff_context.end_snap_id, &diff, &end_size,
&end_exists);
&end_exists, &clone_end_snap_id);
ldout(cct, 20) << " diff " << diff << " end_exists=" << end_exists
<< dendl;
if (diff.empty()) {
@@ -84,6 +84,10 @@ class MockTestMemIoCtxImpl : public TestMemIoCtxImpl {
return TestMemIoCtxImpl::notify(o, bl, timeout_ms, pbl);
}

MOCK_METHOD1(set_snap_read, void(snap_t));
void do_set_snap_read(snap_t snap_id) {
return TestMemIoCtxImpl::set_snap_read(snap_id);
}
MOCK_METHOD5(sparse_read, int(const std::string& oid,
uint64_t off,
size_t len,
@@ -158,6 +162,7 @@ class MockTestMemIoCtxImpl : public TestMemIoCtxImpl {
ON_CALL(*this, list_watchers(_, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_list_watchers));
ON_CALL(*this, notify(_, _, _, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_notify));
ON_CALL(*this, read(_, _, _, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_read));
ON_CALL(*this, set_snap_read(_)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_set_snap_read));
ON_CALL(*this, sparse_read(_, _, _, _, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_sparse_read));
ON_CALL(*this, remove(_, _)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_remove));
ON_CALL(*this, selfmanaged_snap_create(_)).WillByDefault(Invoke(this, &MockTestMemIoCtxImpl::do_selfmanaged_snap_create));
@@ -32,11 +32,16 @@ struct MockTestImageCtx : public librbd::MockImageCtx {
#include "tools/rbd_mirror/image_sync/ObjectCopyRequest.cc"
template class rbd::mirror::image_sync::ObjectCopyRequest<librbd::MockTestImageCtx>;

bool operator==(const SnapContext& rhs, const SnapContext& lhs) {
return (rhs.seq == lhs.seq && rhs.snaps == lhs.snaps);
}

namespace rbd {
namespace mirror {
namespace image_sync {

using ::testing::_;
using ::testing::DoAll;
using ::testing::DoDefault;
using ::testing::InSequence;
using ::testing::Invoke;
@@ -83,8 +88,21 @@ class TestMockImageSyncObjectCopyRequest : public TestMockFixture {
ASSERT_EQ(0, open_image(m_local_io_ctx, m_image_name, &m_local_image_ctx));
}

void expect_list_snaps(librbd::MockTestImageCtx &mock_image_ctx,
librados::MockTestMemIoCtxImpl &mock_io_ctx,
const librados::snap_set_t &snap_set) {
expect_set_snap_read(mock_io_ctx, CEPH_SNAPDIR);
EXPECT_CALL(mock_io_ctx,
list_snaps(mock_image_ctx.image_ctx->get_object_name(0), _))
.WillOnce(DoAll(WithArg<1>(Invoke([&snap_set](librados::snap_set_t *out_snap_set) {
*out_snap_set = snap_set;
})),
Return(0)));
}

void expect_list_snaps(librbd::MockTestImageCtx &mock_image_ctx,
librados::MockTestMemIoCtxImpl &mock_io_ctx, int r) {
expect_set_snap_read(mock_io_ctx, CEPH_SNAPDIR);
auto &expect = EXPECT_CALL(mock_io_ctx,
list_snaps(mock_image_ctx.image_ctx->get_object_name(0),
_));
@@ -110,6 +128,11 @@ class TestMockImageSyncObjectCopyRequest : public TestMockFixture {
0, on_finish);
}

void expect_set_snap_read(librados::MockTestMemIoCtxImpl &mock_io_ctx,
uint64_t snap_id) {
EXPECT_CALL(mock_io_ctx, set_snap_read(snap_id));
}

void expect_sparse_read(librados::MockTestMemIoCtxImpl &mock_io_ctx, uint64_t offset,
uint64_t length, int r) {

@@ -132,8 +155,9 @@ class TestMockImageSyncObjectCopyRequest : public TestMockFixture {
}

void expect_write(librados::MockTestMemIoCtxImpl &mock_io_ctx,
uint64_t offset, uint64_t length, int r) {
auto &expect = EXPECT_CALL(mock_io_ctx, write(_, _, length, offset, _));
uint64_t offset, uint64_t length,
const SnapContext &snapc, int r) {
auto &expect = EXPECT_CALL(mock_io_ctx, write(_, _, length, offset, snapc));
if (r < 0) {
expect.WillOnce(Return(r));
} else {
@@ -142,9 +166,10 @@ class TestMockImageSyncObjectCopyRequest : public TestMockFixture {
}

void expect_write(librados::MockTestMemIoCtxImpl &mock_io_ctx,
const interval_set<uint64_t> &extents, int r) {
const interval_set<uint64_t> &extents,
const SnapContext &snapc, int r) {
for (auto extent : extents) {
expect_write(mock_io_ctx, extent.first, extent.second, r);
expect_write(mock_io_ctx, extent.first, extent.second, snapc, r);
if (r < 0) {
break;
}
@@ -215,6 +240,7 @@ class TestMockImageSyncObjectCopyRequest : public TestMockFixture {
m_snap_map.rbegin()->second.end());
}
m_snap_map[remote_snap_id] = local_snap_ids;
m_remote_snap_ids.push_back(remote_snap_id);
m_local_snap_ids.push_back(local_snap_id);

return 0;
@@ -301,6 +327,7 @@ class TestMockImageSyncObjectCopyRequest : public TestMockFixture {
librbd::ImageCtx *m_local_image_ctx;

MockObjectCopyRequest::SnapMap m_snap_map;
std::vector<librados::snap_t> m_remote_snap_ids;
std::vector<librados::snap_t> m_local_snap_ids;
};

@@ -353,8 +380,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Write) {

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[0], OBJECT_EXISTS, 0);

@@ -363,6 +391,99 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Write) {
ASSERT_EQ(0, compare_objects());
}

TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingStaleSnapSet) {
ASSERT_EQ(0, create_snap("one"));
ASSERT_EQ(0, create_snap("two"));

// scribble some data
interval_set<uint64_t> one;
scribble(m_remote_image_ctx, 10, 102400, &one);
ASSERT_EQ(0, create_snap("three"));

ASSERT_EQ(0, create_snap("sync"));
librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);

librbd::MockObjectMap mock_object_map;
mock_local_image_ctx.object_map = &mock_object_map;

expect_test_features(mock_local_image_ctx);

C_SaferCond ctx;
MockObjectCopyRequest *request = create_request(mock_remote_image_ctx,
mock_local_image_ctx, &ctx);

librados::MockTestMemIoCtxImpl &mock_remote_io_ctx(get_mock_io_ctx(
request->get_remote_io_ctx()));
librados::MockTestMemIoCtxImpl &mock_local_io_ctx(get_mock_io_ctx(
request->get_local_io_ctx()));

librados::clone_info_t dummy_clone_info;
dummy_clone_info.cloneid = librados::SNAP_HEAD;
dummy_clone_info.size = 123;

librados::snap_set_t dummy_snap_set1;
dummy_snap_set1.clones.push_back(dummy_clone_info);

dummy_clone_info.size = 234;
librados::snap_set_t dummy_snap_set2;
dummy_snap_set2.clones.push_back(dummy_clone_info);

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, dummy_snap_set1);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]);
expect_sparse_read(mock_remote_io_ctx, 0, 123, -ENOENT);
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, dummy_snap_set2);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]);
expect_sparse_read(mock_remote_io_ctx, 0, 234, -ENOENT);
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[3]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(),
{m_local_snap_ids[1], {m_local_snap_ids[1],
m_local_snap_ids[0]}},
0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[2], OBJECT_EXISTS, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[3], OBJECT_EXISTS_CLEAN, 0);

request->send();
ASSERT_EQ(0, ctx.wait());
ASSERT_EQ(0, compare_objects());
}

TEST_F(TestMockImageSyncObjectCopyRequest, ReadMissingUpToDateSnapMap) {
// scribble some data
interval_set<uint64_t> one;
scribble(m_remote_image_ctx, 10, 102400, &one);

ASSERT_EQ(0, create_snap("sync"));
librbd::MockTestImageCtx mock_remote_image_ctx(*m_remote_image_ctx);
librbd::MockTestImageCtx mock_local_image_ctx(*m_local_image_ctx);

librbd::MockObjectMap mock_object_map;
mock_local_image_ctx.object_map = &mock_object_map;

expect_test_features(mock_local_image_ctx);

C_SaferCond ctx;
MockObjectCopyRequest *request = create_request(mock_remote_image_ctx,
mock_local_image_ctx, &ctx);

librados::MockTestMemIoCtxImpl &mock_remote_io_ctx(get_mock_io_ctx(
request->get_remote_io_ctx()));

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), -ENOENT);
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);

request->send();
ASSERT_EQ(-ENOENT, ctx.wait());
}

TEST_F(TestMockImageSyncObjectCopyRequest, ReadError) {
// scribble some data
interval_set<uint64_t> one;
@@ -386,6 +507,7 @@ TEST_F(TestMockImageSyncObjectCopyRequest, ReadError) {

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), -EINVAL);

request->send();
@@ -417,8 +539,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteError) {

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), -EINVAL);
expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, -EINVAL);

request->send();
ASSERT_EQ(-EINVAL, ctx.wait());
@@ -460,10 +583,13 @@ TEST_F(TestMockImageSyncObjectCopyRequest, WriteSnaps) {

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[2]);
expect_sparse_read(mock_remote_io_ctx, two, 0);
expect_write(mock_local_io_ctx, two, 0);
expect_write(mock_local_io_ctx, two,
{m_local_snap_ids[0], {m_local_snap_ids[0]}}, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[0], OBJECT_EXISTS, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
@@ -509,8 +635,9 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Trim) {

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[0]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
expect_truncate(mock_local_io_ctx, trim_offset, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[0], OBJECT_EXISTS, 0);
@@ -527,6 +654,7 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Remove) {
interval_set<uint64_t> one;
scribble(m_remote_image_ctx, 10, 102400, &one);
ASSERT_EQ(0, create_snap("one"));
ASSERT_EQ(0, create_snap("two"));

// remove the object
uint64_t object_size = 1 << m_remote_image_ctx->order;
@@ -551,11 +679,14 @@ TEST_F(TestMockImageSyncObjectCopyRequest, Remove) {

InSequence seq;
expect_list_snaps(mock_remote_image_ctx, mock_remote_io_ctx, 0);
expect_set_snap_read(mock_remote_io_ctx, m_remote_snap_ids[1]);
expect_sparse_read(mock_remote_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), 0);
expect_write(mock_local_io_ctx, 0, one.range_end(), {0, {}}, 0);
expect_remove(mock_local_io_ctx, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[0], OBJECT_EXISTS, 0);
expect_update_object_map(mock_local_image_ctx, mock_object_map,
m_local_snap_ids[1], OBJECT_EXISTS_CLEAN, 0);

request->send();
ASSERT_EQ(0, ctx.wait());
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.