From: Mykola Golub Date: Mon, 12 Aug 2019 13:42:51 +0000 (+0100) Subject: librbd: behave more gracefully when data pool removed X-Git-Tag: v14.2.5~149^2~1 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=eb8adcbe50e8199ca40e3986a595ffc2bb580395;p=ceph.git librbd: behave more gracefully when data pool removed allowing to open the image and do some maintenance operations, and returning -ENODEV for ops that require data pool. Fixes: https://tracker.ceph.com/issues/41206 Signed-off-by: Mykola Golub (cherry picked from commit 00b2c86f7c486fbd82e6ce27edbd90f866de88af) Conflicts: src/librbd/cache/ObjectCacherObjectDispatch.cc: trivial resolution src/librbd/cache/ParentCacheObjectDispatch.cc: DNE src/librbd/image/RefreshRequest.cc: trivial resolution src/librbd/image/OpenRequest.cc: don't init cache on invalid data_ctx src/librbd/io/ImageRequestWQ.cc: get reference to AioCompletion before failing --- diff --git a/src/librbd/DeepCopyRequest.cc b/src/librbd/DeepCopyRequest.cc index 45191158e38c4..1d56b255d4cba 100644 --- a/src/librbd/DeepCopyRequest.cc +++ b/src/librbd/DeepCopyRequest.cc @@ -50,6 +50,18 @@ DeepCopyRequest::~DeepCopyRequest() { template void DeepCopyRequest::send() { + if (!m_src_image_ctx->data_ctx.is_valid()) { + lderr(m_cct) << "missing data pool for source image" << dendl; + finish(-ENODEV); + return; + } + + if (!m_dst_image_ctx->data_ctx.is_valid()) { + lderr(m_cct) << "missing data pool for destination image" << dendl; + finish(-ENODEV); + return; + } + int r = validate_copy_points(); if (r < 0) { finish(r); diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index 19d3ce70c9bee..6ad3b7455e471 100644 --- a/src/librbd/ImageCtx.cc +++ b/src/librbd/ImageCtx.cc @@ -172,7 +172,9 @@ public: delete[] format_string; md_ctx.aio_flush(); - data_ctx.aio_flush(); + if (data_ctx.is_valid()) { + data_ctx.aio_flush(); + } io_work_queue->drain(); delete io_object_dispatcher; @@ -191,7 +193,7 @@ public: asok_hook = new LibrbdAdminSocketHook(this); string pname = string("librbd-") + id + string("-") + - data_ctx.get_pool_name() + string("-") + name; + md_ctx.get_pool_name() + string("-") + name; if (!snap_name.empty()) { pname += "-"; pname += snap_name; @@ -212,7 +214,7 @@ public: asok_hook = nullptr; } - void ImageCtx::init_layout() + void ImageCtx::init_layout(int64_t pool_id) { if (stripe_unit == 0 || stripe_count == 0) { stripe_unit = 1ull << order; @@ -229,7 +231,7 @@ public: layout.stripe_unit = stripe_unit; layout.stripe_count = stripe_count; layout.object_size = 1ull << order; - layout.pool_id = data_ctx.get_id(); // FIXME: pool id overflow? + layout.pool_id = pool_id; // FIXME: pool id overflow? delete[] format_string; size_t len = object_prefix.length() + 16; @@ -329,7 +331,9 @@ public: snap_namespace = it->second.snap_namespace; snap_name = it->second.name; snap_exists = true; - data_ctx.snap_set_read(snap_id); + if (data_ctx.is_valid()) { + data_ctx.snap_set_read(snap_id); + } return 0; } return -ENOENT; @@ -342,7 +346,9 @@ public: snap_namespace = {}; snap_name = ""; snap_exists = true; - data_ctx.snap_set_read(snap_id); + if (data_ctx.is_valid()) { + data_ctx.snap_set_read(snap_id); + } } snap_t ImageCtx::get_snap_id(const cls::rbd::SnapshotNamespace& in_snap_namespace, diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h index fe1c4828cabbd..f60dec9886406 100644 --- a/src/librbd/ImageCtx.h +++ b/src/librbd/ImageCtx.h @@ -223,7 +223,7 @@ namespace librbd { ~ImageCtx(); void init(); void shutdown(); - void init_layout(); + void init_layout(int64_t pool_id); void perf_start(std::string name); void perf_stop(); void set_read_flag(unsigned flag); diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc index 148734b7e50a8..55fefcc211e7e 100644 --- a/src/librbd/api/DiffIterate.cc +++ b/src/librbd/api/DiffIterate.cc @@ -238,6 +238,10 @@ int DiffIterate::diff_iterate(I *ictx, ldout(ictx->cct, 20) << "diff_iterate " << ictx << " off = " << off << " len = " << len << dendl; + if (!ictx->data_ctx.is_valid()) { + return -ENODEV; + } + // ensure previous writes are visible to listsnaps C_SaferCond flush_ctx; { @@ -276,6 +280,8 @@ template int DiffIterate::execute() { CephContext* cct = m_image_ctx.cct; + ceph_assert(m_image_ctx.data_ctx.is_valid()); + librados::IoCtx head_ctx; librados::snap_t from_snap_id = 0; librados::snap_t end_snap_id; diff --git a/src/librbd/api/Image.cc b/src/librbd/api/Image.cc index 13c5a90d8c547..bb995e0f6ddbe 100644 --- a/src/librbd/api/Image.cc +++ b/src/librbd/api/Image.cc @@ -76,6 +76,23 @@ int pre_remove_image(librados::IoCtx& io_ctx, const std::string& image_id) { } // anonymous namespace +template +int64_t Image::get_data_pool_id(I *ictx) { + if (ictx->data_ctx.is_valid()) { + return ictx->data_ctx.get_id(); + } + + int64_t pool_id; + int r = cls_client::get_data_pool(&ictx->md_ctx, ictx->header_oid, &pool_id); + if (r < 0) { + CephContext *cct = ictx->cct; + lderr(cct) << "error getting data pool ID: " << cpp_strerror(r) << dendl; + return r; + } + + return pool_id; +} + template int Image::get_op_features(I *ictx, uint64_t *op_features) { CephContext *cct = ictx->cct; diff --git a/src/librbd/api/Image.h b/src/librbd/api/Image.h index 8bfadae70de93..af928c84c06c9 100644 --- a/src/librbd/api/Image.h +++ b/src/librbd/api/Image.h @@ -24,6 +24,8 @@ template struct Image { typedef std::map ImageNameToIds; + static int64_t get_data_pool_id(ImageCtxT *ictx); + static int get_op_features(ImageCtxT *ictx, uint64_t *op_features); static int list_images(librados::IoCtx& io_ctx, diff --git a/src/librbd/api/Snapshot.cc b/src/librbd/api/Snapshot.cc index 99010bf4d3537..3d29cfb263a20 100644 --- a/src/librbd/api/Snapshot.cc +++ b/src/librbd/api/Snapshot.cc @@ -111,7 +111,7 @@ int Snapshot::get_group_namespace(I *ictx, uint64_t snap_id, return -ENOENT; } - GetGroupVisitor ggv = GetGroupVisitor(ictx->cct, &ictx->data_ctx, group_snap); + GetGroupVisitor ggv = GetGroupVisitor(ictx->cct, &ictx->md_ctx, group_snap); r = boost::apply_visitor(ggv, snap_info->snap_namespace); if (r < 0) { return r; diff --git a/src/librbd/cache/ObjectCacherObjectDispatch.cc b/src/librbd/cache/ObjectCacherObjectDispatch.cc index f108247b25423..5bced71bfdf36 100644 --- a/src/librbd/cache/ObjectCacherObjectDispatch.cc +++ b/src/librbd/cache/ObjectCacherObjectDispatch.cc @@ -78,6 +78,7 @@ ObjectCacherObjectDispatch::ObjectCacherObjectDispatch( : m_image_ctx(image_ctx), m_cache_lock(util::unique_lock_name( "librbd::cache::ObjectCacherObjectDispatch::cache_lock", this)) { + ceph_assert(m_image_ctx->data_ctx.is_valid()); } template diff --git a/src/librbd/deep_copy/ObjectCopyRequest.cc b/src/librbd/deep_copy/ObjectCopyRequest.cc index 14acda37edfb6..92a1750f1152e 100644 --- a/src/librbd/deep_copy/ObjectCopyRequest.cc +++ b/src/librbd/deep_copy/ObjectCopyRequest.cc @@ -50,6 +50,8 @@ ObjectCopyRequest::ObjectCopyRequest(I *src_image_ctx, m_dst_image_ctx(dst_image_ctx), m_cct(dst_image_ctx->cct), m_snap_map(snap_map), m_dst_object_number(dst_object_number), m_flatten(flatten), m_on_finish(on_finish) { + ceph_assert(src_image_ctx->data_ctx.is_valid()); + ceph_assert(dst_image_ctx->data_ctx.is_valid()); ceph_assert(!m_snap_map.empty()); m_src_async_op = new io::AsyncOperation(); diff --git a/src/librbd/image/OpenRequest.cc b/src/librbd/image/OpenRequest.cc index dd9e603dc98bd..7d4625aea1c28 100644 --- a/src/librbd/image/OpenRequest.cc +++ b/src/librbd/image/OpenRequest.cc @@ -473,13 +473,19 @@ Context *OpenRequest::handle_v2_get_data_pool(int *result) { *result = util::create_ioctx(m_image_ctx->md_ctx, "data pool", data_pool_id, {}, &m_image_ctx->data_ctx); if (*result < 0) { - send_close_image(*result); - return nullptr; + if (*result != -ENOENT) { + send_close_image(*result); + return nullptr; + } + m_image_ctx->data_ctx.close(); + } else { + m_image_ctx->data_ctx.set_namespace(m_image_ctx->md_ctx.get_namespace()); } - m_image_ctx->data_ctx.set_namespace(m_image_ctx->md_ctx.get_namespace()); + } else { + data_pool_id = m_image_ctx->md_ctx.get_id(); } - m_image_ctx->init_layout(); + m_image_ctx->init_layout(data_pool_id); send_refresh(); return nullptr; } @@ -516,7 +522,8 @@ Context *OpenRequest::handle_refresh(int *result) { template Context *OpenRequest::send_init_cache(int *result) { // cache is disabled or parent image context - if (!m_image_ctx->cache || m_image_ctx->child != nullptr) { + if (!m_image_ctx->cache || m_image_ctx->child != nullptr || + !m_image_ctx->data_ctx.is_valid()) { return send_register_watch(result); } diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc index 664f5b8b793e5..7ec565d6cafc3 100644 --- a/src/librbd/image/RefreshRequest.cc +++ b/src/librbd/image/RefreshRequest.cc @@ -1313,7 +1313,7 @@ void RefreshRequest::apply() { m_image_ctx.op_features = 0; m_image_ctx.operations_disabled = false; m_image_ctx.object_prefix = std::move(m_object_prefix); - m_image_ctx.init_layout(); + m_image_ctx.init_layout(m_image_ctx.md_ctx.get_id()); } else { // HEAD revision doesn't have a defined overlap so it's only // applicable to snapshots @@ -1396,8 +1396,10 @@ void RefreshRequest::apply() { if (m_refresh_parent != nullptr) { m_refresh_parent->apply(); } - m_image_ctx.data_ctx.selfmanaged_snap_set_write_ctx(m_image_ctx.snapc.seq, - m_image_ctx.snaps); + if (m_image_ctx.data_ctx.is_valid()) { + m_image_ctx.data_ctx.selfmanaged_snap_set_write_ctx(m_image_ctx.snapc.seq, + m_image_ctx.snaps); + } // handle dynamically enabled / disabled features if (m_image_ctx.exclusive_lock != nullptr && diff --git a/src/librbd/image/RemoveRequest.cc b/src/librbd/image/RemoveRequest.cc index 1cff51b3eae5d..8e029f8b91223 100644 --- a/src/librbd/image/RemoveRequest.cc +++ b/src/librbd/image/RemoveRequest.cc @@ -126,6 +126,11 @@ void RemoveRequest::handle_pre_remove_image(int r) { return; } + if (!m_image_ctx->data_ctx.is_valid()) { + detach_child(); + return; + } + trim_image(); } diff --git a/src/librbd/image/RemoveRequest.h b/src/librbd/image/RemoveRequest.h index 40a571e5aab85..98d597645a491 100644 --- a/src/librbd/image/RemoveRequest.h +++ b/src/librbd/image/RemoveRequest.h @@ -63,7 +63,7 @@ private: * PRE REMOVE IMAGE * * * | * | * | * v * | - * TRIM IMAGE * * * * * | + * (skip if invalid data pool) TRIM IMAGE * * * * * | * | * | * v * | * DETACH CHILD * | diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc index 3ab44d0ec8599..06ef47ca86504 100644 --- a/src/librbd/io/CopyupRequest.cc +++ b/src/librbd/io/CopyupRequest.cc @@ -114,6 +114,7 @@ CopyupRequest::CopyupRequest(I *ictx, const std::string &oid, m_trace(util::create_trace(*m_image_ctx, "copy-up", parent_trace)), m_lock("CopyupRequest", false, false) { + ceph_assert(m_image_ctx->data_ctx.is_valid()); m_async_op.start_op(*util::get_image_ctx(m_image_ctx)); } diff --git a/src/librbd/io/ImageRequestWQ.cc b/src/librbd/io/ImageRequestWQ.cc index 32584640d7ddb..34f2c2cf2bae4 100644 --- a/src/librbd/io/ImageRequestWQ.cc +++ b/src/librbd/io/ImageRequestWQ.cc @@ -844,6 +844,15 @@ int ImageRequestWQ::start_in_flight_io(AioCompletion *c) { return false; } + if (!m_image_ctx.data_ctx.is_valid()) { + CephContext *cct = m_image_ctx.cct; + lderr(cct) << "missing data pool" << dendl; + + c->get(); + c->fail(-ENODEV); + return false; + } + m_in_flight_ios++; return true; } diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc index 57bd7291c6824..58b1708455eb0 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -115,6 +115,7 @@ ObjectRequest::ObjectRequest(I *ictx, const std::string &oid, : m_ictx(ictx), m_oid(oid), m_object_no(objectno), m_object_off(off), m_object_len(len), m_snap_id(snap_id), m_completion(completion), m_trace(util::create_trace(*ictx, "", trace)) { + ceph_assert(m_ictx->data_ctx.is_valid()); if (m_trace.valid()) { m_trace.copy_name(trace_name + std::string(" ") + oid); m_trace.event("start"); diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc index 605fe13323928..80dcbf8b2f50d 100644 --- a/src/librbd/librbd.cc +++ b/src/librbd/librbd.cc @@ -1457,7 +1457,7 @@ namespace librbd { int64_t Image::get_data_pool_id() { ImageCtx *ictx = reinterpret_cast(ctx); - return ictx->data_ctx.get_id(); + return librbd::api::Image<>::get_data_pool_id(ictx); } int Image::parent_info(string *parent_pool_name, string *parent_name, @@ -4228,7 +4228,7 @@ extern "C" int rbd_get_block_name_prefix(rbd_image_t image, char *prefix, extern "C" int64_t rbd_get_data_pool_id(rbd_image_t image) { librbd::ImageCtx *ictx = reinterpret_cast(image); - return ictx->data_ctx.get_id(); + return librbd::api::Image<>::get_data_pool_id(ictx); } extern "C" int rbd_get_parent_info(rbd_image_t image, diff --git a/src/librbd/operation/ObjectMapIterate.cc b/src/librbd/operation/ObjectMapIterate.cc index 658dbb4a73762..669584164902e 100644 --- a/src/librbd/operation/ObjectMapIterate.cc +++ b/src/librbd/operation/ObjectMapIterate.cc @@ -194,6 +194,11 @@ private: template void ObjectMapIterateRequest::send() { + if (!m_image_ctx.data_ctx.is_valid()) { + this->async_complete(-ENODEV); + return; + } + send_verify_objects(); } @@ -201,6 +206,12 @@ template bool ObjectMapIterateRequest::should_complete(int r) { CephContext *cct = m_image_ctx.cct; ldout(cct, 5) << this << " should_complete: " << " r=" << r << dendl; + + if (r == -ENODEV) { + lderr(cct) << "missing data pool" << dendl; + return true; + } + if (r < 0) { lderr(cct) << "object map operation encountered an error: " << cpp_strerror(r) << dendl; diff --git a/src/librbd/operation/SnapshotCreateRequest.cc b/src/librbd/operation/SnapshotCreateRequest.cc index 31553177306f0..66293609166e1 100644 --- a/src/librbd/operation/SnapshotCreateRequest.cc +++ b/src/librbd/operation/SnapshotCreateRequest.cc @@ -36,6 +36,15 @@ SnapshotCreateRequest::SnapshotCreateRequest(I &image_ctx, template void SnapshotCreateRequest::send_op() { + I &image_ctx = this->m_image_ctx; + CephContext *cct = image_ctx.cct; + + if (!image_ctx.data_ctx.is_valid()) { + lderr(cct) << "missing data pool" << dendl; + this->async_complete(-ENODEV); + return; + } + send_suspend_requests(); } diff --git a/src/librbd/operation/SnapshotRemoveRequest.cc b/src/librbd/operation/SnapshotRemoveRequest.cc index 8c34ebf6c5342..fde41a5f363dc 100644 --- a/src/librbd/operation/SnapshotRemoveRequest.cc +++ b/src/librbd/operation/SnapshotRemoveRequest.cc @@ -263,6 +263,11 @@ template void SnapshotRemoveRequest::release_snap_id() { I &image_ctx = this->m_image_ctx; + if (!image_ctx.data_ctx.is_valid()) { + remove_snap(); + return; + } + CephContext *cct = image_ctx.cct; ldout(cct, 5) << "snap_name=" << m_snap_name << ", " << "snap_id=" << m_snap_id << dendl; diff --git a/src/librbd/operation/SparsifyRequest.cc b/src/librbd/operation/SparsifyRequest.cc index 4dc6bde7e3240..53049f886e7a5 100644 --- a/src/librbd/operation/SparsifyRequest.cc +++ b/src/librbd/operation/SparsifyRequest.cc @@ -124,6 +124,11 @@ public: ldout(m_cct, 20) << dendl; + if (!image_ctx.data_ctx.is_valid()) { + lderr(m_cct) << "missing data pool" << dendl; + return -ENODEV; + } + if (image_ctx.exclusive_lock != nullptr && !image_ctx.exclusive_lock->is_lock_owner()) { ldout(m_cct, 1) << "lost exclusive lock during sparsify" << dendl; diff --git a/src/librbd/operation/TrimRequest.cc b/src/librbd/operation/TrimRequest.cc index cecf37b3e6138..15f6a5dfd8185 100644 --- a/src/librbd/operation/TrimRequest.cc +++ b/src/librbd/operation/TrimRequest.cc @@ -174,6 +174,15 @@ bool TrimRequest::should_complete(int r) template void TrimRequest::send() { + I &image_ctx = this->m_image_ctx; + CephContext *cct = image_ctx.cct; + + if (!image_ctx.data_ctx.is_valid()) { + lderr(cct) << "missing data pool" << dendl; + send_finish(-ENODEV); + return; + } + send_pre_trim(); } diff --git a/src/test/librbd/image/test_mock_RefreshRequest.cc b/src/test/librbd/image/test_mock_RefreshRequest.cc index 5ce1feda777d0..a920948fb11e4 100644 --- a/src/test/librbd/image/test_mock_RefreshRequest.cc +++ b/src/test/librbd/image/test_mock_RefreshRequest.cc @@ -378,7 +378,7 @@ public: } void expect_init_layout(MockRefreshImageCtx &mock_image_ctx) { - EXPECT_CALL(mock_image_ctx, init_layout()); + EXPECT_CALL(mock_image_ctx, init_layout(_)); } void expect_test_features(MockRefreshImageCtx &mock_image_ctx) { diff --git a/src/test/librbd/mock/MockImageCtx.h b/src/test/librbd/mock/MockImageCtx.h index 4c45011d3ce54..5c0db6a8fc164 100644 --- a/src/test/librbd/mock/MockImageCtx.h +++ b/src/test/librbd/mock/MockImageCtx.h @@ -139,7 +139,7 @@ struct MockImageCtx { ctx.wait(); } - MOCK_METHOD0(init_layout, void()); + MOCK_METHOD1(init_layout, void(int64_t)); MOCK_CONST_METHOD1(get_object_name, std::string(uint64_t)); MOCK_CONST_METHOD0(get_object_size, uint64_t()); diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc index e95be0b8a9f20..0caf9ffff431d 100644 --- a/src/test/librbd/test_internal.cc +++ b/src/test/librbd/test_internal.cc @@ -1683,3 +1683,72 @@ TEST_F(TestInternal, SparsifyClone) { ASSERT_EQ(0, ictx->data_ctx.stat(oid, &size, NULL)); ASSERT_EQ(0, ictx->data_ctx.read(oid, read_bl, 4096, 0)); } + +TEST_F(TestInternal, MissingDataPool) { + REQUIRE_FORMAT_V2(); + + librbd::ImageCtx *ictx; + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + ASSERT_EQ(0, snap_create(*ictx, "snap1")); + std::string header_oid = ictx->header_oid; + close_image(ictx); + + // emulate non-existent data pool + int64_t pool_id = 1234; + std::string pool_name; + int r; + while ((r = _rados.pool_reverse_lookup(pool_id, &pool_name)) == 0) { + pool_id++; + } + ASSERT_EQ(r, -ENOENT); + bufferlist bl; + using ceph::encode; + encode(pool_id, bl); + ASSERT_EQ(0, m_ioctx.omap_set(header_oid, {{"data_pool_id", bl}})); + + ASSERT_EQ(0, open_image(m_image_name, &ictx)); + + ASSERT_FALSE(ictx->data_ctx.is_valid()); + ASSERT_EQ(pool_id, librbd::api::Image<>::get_data_pool_id(ictx)); + + librbd::image_info_t info; + ASSERT_EQ(0, librbd::info(ictx, info, sizeof(info))); + + vector snaps; + EXPECT_EQ(0, librbd::snap_list(ictx, snaps)); + EXPECT_EQ(1U, snaps.size()); + EXPECT_EQ("snap1", snaps[0].name); + + bufferptr read_ptr(256); + bufferlist read_bl; + read_bl.push_back(read_ptr); + librbd::io::ReadResult read_result{&read_bl}; + ASSERT_EQ(-ENODEV, + ictx->io_work_queue->read(0, 256, + librbd::io::ReadResult{read_result}, 0)); + ASSERT_EQ(-ENODEV, + ictx->io_work_queue->write(0, bl.length(), bufferlist{bl}, 0)); + ASSERT_EQ(-ENODEV, ictx->io_work_queue->discard(0, 1, 256)); + ASSERT_EQ(-ENODEV, + ictx->io_work_queue->writesame(0, bl.length(), bufferlist{bl}, 0)); + uint64_t mismatch_off; + ASSERT_EQ(-ENODEV, + ictx->io_work_queue->compare_and_write(0, bl.length(), + bufferlist{bl}, + bufferlist{bl}, + &mismatch_off, 0)); + ASSERT_EQ(-ENODEV, ictx->io_work_queue->flush()); + + ASSERT_EQ(-ENODEV, snap_create(*ictx, "snap2")); + ASSERT_EQ(0, ictx->operations->snap_remove(cls::rbd::UserSnapshotNamespace(), + "snap1")); + + librbd::NoOpProgressContext no_op; + ASSERT_EQ(-ENODEV, ictx->operations->resize(0, true, no_op)); + + close_image(ictx); + + ASSERT_EQ(0, librbd::api::Image<>::remove(m_ioctx, m_image_name, no_op)); + + ASSERT_EQ(0, create_image_pp(m_rbd, m_ioctx, m_image_name, m_image_size)); +}