Skip to content
Permalink
Browse files

Merge pull request #13568 from dillaman/wip-18990

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

Reviewed-by: Mykola Golub <mgolub@mirantis.com>
  • Loading branch information...
Mykola Golub
Mykola Golub committed Feb 23, 2017
2 parents c733d48 + b4f36d5 commit 7a8e718585fd6443d6e70286eed4cf6d7c20e8e6
@@ -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.

0 comments on commit 7a8e718

Please sign in to comment.
You can’t perform that action at this time.