From 9c59e688eac2625e9fb340a3c21053b01b7a87cd Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 20 Jun 2018 15:00:01 -0400 Subject: [PATCH] librbd: image create state machine should validate namespace Fixes: http://tracker.ceph.com/issues/24558 Signed-off-by: Jason Dillaman --- src/librbd/image/CreateRequest.cc | 140 +++++++++++++++++------------- src/librbd/image/CreateRequest.h | 21 ++--- src/librbd/internal.cc | 5 ++ src/test/librbd/test_librbd.cc | 47 +++++++++- 4 files changed, 142 insertions(+), 71 deletions(-) diff --git a/src/librbd/image/CreateRequest.cc b/src/librbd/image/CreateRequest.cc index 1418ef38f3caa..5ebd6a6a718c2 100644 --- a/src/librbd/image/CreateRequest.cc +++ b/src/librbd/image/CreateRequest.cc @@ -140,12 +140,14 @@ CreateRequest::CreateRequest(IoCtx &ioctx, const std::string &image_name, const std::string &primary_mirror_uuid, bool skip_mirror_enable, ContextWQ *op_work_queue, Context *on_finish) - : m_ioctx(ioctx), m_image_name(image_name), m_image_id(image_id), + : m_image_name(image_name), m_image_id(image_id), m_size(size), m_non_primary_global_image_id(non_primary_global_image_id), m_primary_mirror_uuid(primary_mirror_uuid), m_skip_mirror_enable(skip_mirror_enable), m_op_work_queue(op_work_queue), m_on_finish(on_finish) { - m_cct = reinterpret_cast(m_ioctx.cct()); + + m_io_ctx.dup(ioctx); + m_cct = reinterpret_cast(m_io_ctx.cct()); m_id_obj = util::id_obj_name(m_image_name); m_header_obj = util::header_name(m_image_id); @@ -259,7 +261,7 @@ void CreateRequest::send() { return; } - r = validate_data_pool(m_cct, m_ioctx, m_features, m_data_pool, + r = validate_data_pool(m_cct, m_io_ctx, m_features, m_data_pool, &m_data_pool_id); if (r < 0) { complete(r); @@ -278,7 +280,7 @@ void CreateRequest::send() { template void CreateRequest::validate_pool() { if (!m_cct->_conf->get_val("rbd_validate_pool")) { - create_id_object(); + add_image_to_directory(); return; } @@ -292,7 +294,7 @@ void CreateRequest::validate_pool() { op.stat(NULL, NULL, NULL); m_outbl.clear(); - int r = m_ioctx.aio_operate(RBD_DIRECTORY, comp, &op, &m_outbl); + int r = m_io_ctx.aio_operate(RBD_DIRECTORY, comp, &op, &m_outbl); assert(r == 0); comp->release(); } @@ -318,7 +320,7 @@ void CreateRequest::handle_validate_pool(int r) { // deadlocks). uint64_t snap_id; - r = m_ioctx.selfmanaged_snap_create(&snap_id); + r = m_io_ctx.selfmanaged_snap_create(&snap_id); if (r == -EINVAL) { lderr(m_cct) << "pool not configured for self-managed RBD snapshot support" << dendl; @@ -331,7 +333,7 @@ void CreateRequest::handle_validate_pool(int r) { return; } - r = m_ioctx.selfmanaged_snap_remove(snap_id); + r = m_io_ctx.selfmanaged_snap_remove(snap_id); if (r < 0) { // we've already switched to self-managed snapshots -- no need to // error out in case of failure here. @@ -346,9 +348,9 @@ template void CreateRequest::validate_overwrite() { ldout(m_cct, 20) << dendl; - m_data_io_ctx = m_ioctx; + m_data_io_ctx = m_io_ctx; if (m_data_pool_id != -1) { - librados::Rados rados(m_ioctx); + librados::Rados rados(m_io_ctx); int r = rados.ioctx_create2(m_data_pool_id, m_data_io_ctx); if (r < 0) { lderr(m_cct) << "data pool " << m_data_pool << " does not exist" << dendl; @@ -378,7 +380,7 @@ void CreateRequest::handle_validate_overwrite(int r) { bl.append("overwrite validated"); if (r == 0 && m_outbl.contents_equal(bl)) { - create_id_object(); + add_image_to_directory(); return; } else if ((r < 0) && (r != -ENOENT)) { lderr(m_cct) << "failed to read RBD info: " << cpp_strerror(r) << dendl; @@ -406,64 +408,83 @@ void CreateRequest::handle_validate_overwrite(int r) { return; } - create_id_object(); + add_image_to_directory(); } template -void CreateRequest::create_id_object() { +void CreateRequest::add_image_to_directory() { ldout(m_cct, 20) << dendl; librados::ObjectWriteOperation op; - op.create(true); - cls_client::set_id(&op, m_image_id); + if (!m_io_ctx.get_namespace().empty()) { + cls_client::dir_state_assert(&op, cls::rbd::DIRECTORY_STATE_READY); + } + cls_client::dir_add_image(&op, m_image_name, m_image_id); using klass = CreateRequest; librados::AioCompletion *comp = - create_rados_callback(this); - int r = m_ioctx.aio_operate(m_id_obj, comp, &op); + create_rados_callback(this); + int r = m_io_ctx.aio_operate(RBD_DIRECTORY, comp, &op); assert(r == 0); comp->release(); } template -void CreateRequest::handle_create_id_object(int r) { +void CreateRequest::handle_add_image_to_directory(int r) { ldout(m_cct, 20) << "r=" << r << dendl; - if (r < 0) { - lderr(m_cct) << "error creating RBD id object: " << cpp_strerror(r) + if (r == -EEXIST) { + ldout(m_cct, 5) << "directory entry for image " << m_image_name + << " already exists" << dendl; + complete(r); + return; + } else if (!m_io_ctx.get_namespace().empty() && r == -ENOENT) { + ldout(m_cct, 5) << "namespace " << m_io_ctx.get_namespace() + << " does not exist" << dendl; + complete(r); + return; + } else if (r < 0) { + lderr(m_cct) << "error adding image to directory: " << cpp_strerror(r) << dendl; complete(r); return; } - add_image_to_directory(); + create_id_object(); } template -void CreateRequest::add_image_to_directory() { +void CreateRequest::create_id_object() { ldout(m_cct, 20) << dendl; librados::ObjectWriteOperation op; - cls_client::dir_add_image(&op, m_image_name, m_image_id); + op.create(true); + cls_client::set_id(&op, m_image_id); using klass = CreateRequest; librados::AioCompletion *comp = - create_rados_callback(this); - int r = m_ioctx.aio_operate(RBD_DIRECTORY, comp, &op); + create_rados_callback(this); + int r = m_io_ctx.aio_operate(m_id_obj, comp, &op); assert(r == 0); comp->release(); } template -void CreateRequest::handle_add_image_to_directory(int r) { +void CreateRequest::handle_create_id_object(int r) { ldout(m_cct, 20) << "r=" << r << dendl; - if (r < 0) { - lderr(m_cct) << "error adding image to directory: " << cpp_strerror(r) + if (r == -EEXIST) { + ldout(m_cct, 5) << "id object for " << m_image_name << " already exists" + << dendl; + m_r_saved = r; + remove_from_dir(); + return; + } else if (r < 0) { + lderr(m_cct) << "error creating RBD id object: " << cpp_strerror(r) << dendl; - m_r_saved = r; - remove_id_object(); + remove_from_dir(); + return; } negotiate_features(); @@ -486,7 +507,7 @@ void CreateRequest::negotiate_features() { create_rados_callback(this); m_outbl.clear(); - int r = m_ioctx.aio_operate(RBD_DIRECTORY, comp, &op, &m_outbl); + int r = m_io_ctx.aio_operate(RBD_DIRECTORY, comp, &op, &m_outbl); assert(r == 0); comp->release(); } @@ -520,12 +541,13 @@ void CreateRequest::create_image() { ostringstream oss; oss << RBD_DATA_PREFIX; if (m_data_pool_id != -1) { - oss << stringify(m_ioctx.get_id()) << "."; + oss << stringify(m_io_ctx.get_id()) << "."; } oss << m_image_id; if (oss.str().length() > RBD_MAX_BLOCK_NAME_PREFIX_LENGTH) { lderr(m_cct) << "object prefix '" << oss.str() << "' too large" << dendl; - complete(-EINVAL); + m_r_saved = -EINVAL; + remove_id_object(); return; } @@ -537,7 +559,7 @@ void CreateRequest::create_image() { using klass = CreateRequest; librados::AioCompletion *comp = create_rados_callback(this); - int r = m_ioctx.aio_operate(m_header_obj, comp, &op); + int r = m_io_ctx.aio_operate(m_header_obj, comp, &op); assert(r == 0); comp->release(); } @@ -549,7 +571,7 @@ void CreateRequest::handle_create_image(int r) { if (r < 0) { lderr(m_cct) << "error writing header: " << cpp_strerror(r) << dendl; m_r_saved = r; - remove_from_dir(); + remove_id_object(); return; } @@ -572,7 +594,7 @@ void CreateRequest::set_stripe_unit_count() { using klass = CreateRequest; librados::AioCompletion *comp = create_rados_callback(this); - int r = m_ioctx.aio_operate(m_header_obj, comp, &op); + int r = m_io_ctx.aio_operate(m_header_obj, comp, &op); assert(r == 0); comp->release(); } @@ -608,7 +630,7 @@ void CreateRequest::object_map_resize() { using klass = CreateRequest; librados::AioCompletion *comp = create_rados_callback(this); - int r = m_ioctx.aio_operate(m_objmap_name, comp, &op); + int r = m_io_ctx.aio_operate(m_objmap_name, comp, &op); assert(r == 0); comp->release(); } @@ -645,7 +667,7 @@ void CreateRequest::fetch_mirror_mode() { librados::AioCompletion *comp = create_rados_callback(this); m_outbl.clear(); - int r = m_ioctx.aio_operate(RBD_MIRRORING, comp, &op, &m_outbl); + int r = m_io_ctx.aio_operate(RBD_MIRRORING, comp, &op, &m_outbl); assert(r == 0); comp->release(); } @@ -708,7 +730,7 @@ void CreateRequest::journal_create() { librbd::journal::CreateRequest *req = librbd::journal::CreateRequest::create( - m_ioctx, m_image_id, m_journal_order, m_journal_splay_width, + m_io_ctx, m_image_id, m_journal_order, m_journal_splay_width, m_journal_pool, cls::journal::Tag::TAG_CLASS_NEW, tag_data, librbd::Journal::IMAGE_CLIENT_ID, m_op_work_queue, ctx); req->send(); @@ -741,7 +763,7 @@ void CreateRequest::mirror_image_enable() { ldout(m_cct, 20) << dendl; auto ctx = create_context_callback< CreateRequest, &CreateRequest::handle_mirror_image_enable>(this); - auto req = mirror::EnableRequest::create(m_ioctx, m_image_id, + auto req = mirror::EnableRequest::create(m_io_ctx, m_image_id, m_non_primary_global_image_id, m_op_work_queue, ctx); req->send(); @@ -792,7 +814,7 @@ void CreateRequest::journal_remove() { librbd::journal::RemoveRequest *req = librbd::journal::RemoveRequest::create( - m_ioctx, m_image_id, librbd::Journal::IMAGE_CLIENT_ID, m_op_work_queue, + m_io_ctx, m_image_id, librbd::Journal::IMAGE_CLIENT_ID, m_op_work_queue, ctx); req->send(); } @@ -821,7 +843,7 @@ void CreateRequest::remove_object_map() { using klass = CreateRequest; librados::AioCompletion *comp = create_rados_callback(this); - int r = m_ioctx.aio_remove(m_objmap_name, comp); + int r = m_io_ctx.aio_remove(m_objmap_name, comp); assert(r == 0); comp->release(); } @@ -845,7 +867,7 @@ void CreateRequest::remove_header_object() { using klass = CreateRequest; librados::AioCompletion *comp = create_rados_callback(this); - int r = m_ioctx.aio_remove(m_header_obj, comp); + int r = m_io_ctx.aio_remove(m_header_obj, comp); assert(r == 0); comp->release(); } @@ -859,55 +881,55 @@ void CreateRequest::handle_remove_header_object(int r) { << cpp_strerror(r) << dendl; } - remove_from_dir(); + remove_id_object(); } template -void CreateRequest::remove_from_dir() { +void CreateRequest::remove_id_object() { ldout(m_cct, 20) << dendl; - librados::ObjectWriteOperation op; - cls_client::dir_remove_image(&op, m_image_name, m_image_id); - using klass = CreateRequest; librados::AioCompletion *comp = - create_rados_callback(this); - int r = m_ioctx.aio_operate(RBD_DIRECTORY, comp, &op); + create_rados_callback(this); + int r = m_io_ctx.aio_remove(m_id_obj, comp); assert(r == 0); comp->release(); } template -void CreateRequest::handle_remove_from_dir(int r) { +void CreateRequest::handle_remove_id_object(int r) { ldout(m_cct, 20) << "r=" << r << dendl; if (r < 0) { - lderr(m_cct) << "error cleaning up image from rbd_directory object " - << "after creation failed: " << cpp_strerror(r) << dendl; + lderr(m_cct) << "error cleaning up id object after creation failed: " + << cpp_strerror(r) << dendl; } - remove_id_object(); + remove_from_dir(); } template -void CreateRequest::remove_id_object() { +void CreateRequest::remove_from_dir() { ldout(m_cct, 20) << dendl; + librados::ObjectWriteOperation op; + cls_client::dir_remove_image(&op, m_image_name, m_image_id); + using klass = CreateRequest; librados::AioCompletion *comp = - create_rados_callback(this); - int r = m_ioctx.aio_remove(m_id_obj, comp); + create_rados_callback(this); + int r = m_io_ctx.aio_operate(RBD_DIRECTORY, comp, &op); assert(r == 0); comp->release(); } template -void CreateRequest::handle_remove_id_object(int r) { +void CreateRequest::handle_remove_from_dir(int r) { ldout(m_cct, 20) << "r=" << r << dendl; if (r < 0) { - lderr(m_cct) << "error cleaning up id object after creation failed: " - << cpp_strerror(r) << dendl; + lderr(m_cct) << "error cleaning up image from rbd_directory object " + << "after creation failed: " << cpp_strerror(r) << dendl; } complete(m_r_saved); diff --git a/src/librbd/image/CreateRequest.h b/src/librbd/image/CreateRequest.h index 9464a5cb0a038..00a0818d40652 100644 --- a/src/librbd/image/CreateRequest.h +++ b/src/librbd/image/CreateRequest.h @@ -60,18 +60,18 @@ private: * VALIDATE OVERWRITE . * | . * v . - * (error: bottom up) CREATE ID OBJECT. . < . . . . . + * (error: bottom up) ADD IMAGE TO DIRECTORY < . . . . * _______<_______ | * | | v - * | | ADD IMAGE TO DIRECTORY + * | | CREATE ID OBJECT * | | / | - * | REMOVE ID OBJECT<-------/ v + * | REMOVE FROM DIR <-------/ v * | | NEGOTIATE FEATURES (when using default features) * | | | * | | v (stripingv2 disabled) * | | CREATE IMAGE. . . . > . . . . * v | / | . - * | REMOVE FROM DIR<--------/ v . + * | REMOVE ID OBJ <---------/ v . * | | SET STRIPE UNIT COUNT . * | | / | \ . . . . . > . . . . * | REMOVE HEADER OBJ<------/ v /. (object-map @@ -101,7 +101,7 @@ private: bool skip_mirror_enable, ContextWQ *op_work_queue, Context *on_finish); - IoCtx &m_ioctx; + IoCtx m_io_ctx; IoCtx m_data_io_ctx; std::string m_image_name; std::string m_image_id; @@ -139,12 +139,12 @@ private: void validate_overwrite(); void handle_validate_overwrite(int r); - void create_id_object(); - void handle_create_id_object(int r); - void add_image_to_directory(); void handle_add_image_to_directory(int r); + void create_id_object(); + void handle_create_id_object(int r); + void negotiate_features(); void handle_negotiate_features(int r); @@ -178,11 +178,12 @@ private: void remove_header_object(); void handle_remove_header_object(int r); + void remove_id_object(); + void handle_remove_id_object(int r); + void remove_from_dir(); void handle_remove_from_dir(int r); - void remove_id_object(); - void handle_remove_id_object(int r); }; } //namespace image diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index a62b379b5e85a..cea83d9c29a96 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -746,6 +746,11 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2) return r; } + if (!io_ctx.get_namespace().empty()) { + lderr(cct) << "attempting to add v1 image to namespace" << dendl; + return -EINVAL; + } + ldout(cct, 2) << "adding rbd image to directory..." << dendl; r = tmap_set(io_ctx, imgname); if (r < 0) { diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index 10666fcc09923..df671728eaf3a 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -6667,11 +6667,54 @@ TEST_F(TestLibRBD, NamespacesPP) { std::vector names; ASSERT_EQ(0, rbd.namespace_list(ioctx, &names)); - ASSERT_EQ(2U, names.size()); - ASSERT_EQ(2U, names.size()); ASSERT_EQ("name1", names[0]); ASSERT_EQ("name3", names[1]); + + librados::IoCtx ns_io_ctx; + ns_io_ctx.dup(ioctx); + + std::string name = get_temp_image_name(); + int order = 0; + uint64_t features = 0; + if (!get_features(&features)) { + // old format doesn't support namespaces + ns_io_ctx.set_namespace("name1"); + ASSERT_EQ(-EINVAL, create_image_pp(rbd, ns_io_ctx, name.c_str(), 0, + &order)); + return; + } + + ns_io_ctx.set_namespace("missing"); + ASSERT_EQ(-ENOENT, create_image_pp(rbd, ns_io_ctx, name.c_str(), 0, &order)); + + ns_io_ctx.set_namespace("name1"); + ASSERT_EQ(0, create_image_pp(rbd, ns_io_ctx, name.c_str(), 0, &order)); + ASSERT_EQ(-EBUSY, rbd.namespace_remove(ns_io_ctx, "name1")); + + std::string image_id; + { + librbd::Image image; + ASSERT_EQ(-ENOENT, rbd.open(ioctx, image, name.c_str(), NULL)); + ASSERT_EQ(0, rbd.open(ns_io_ctx, image, name.c_str(), NULL)); + ASSERT_EQ(0, get_image_id(image, &image_id)); + } + + ASSERT_EQ(-ENOENT, rbd.trash_move(ioctx, name.c_str(), 0)); + ASSERT_EQ(0, rbd.trash_move(ns_io_ctx, name.c_str(), 0)); + ASSERT_EQ(-EBUSY, rbd.namespace_remove(ns_io_ctx, "name1")); + + PrintProgress pp; + ASSERT_EQ(-ENOENT, rbd.trash_remove_with_progress(ioctx, image_id.c_str(), + false, pp)); + ASSERT_EQ(0, rbd.trash_remove_with_progress(ns_io_ctx, image_id.c_str(), + false, pp)); + ASSERT_EQ(0, rbd.namespace_remove(ns_io_ctx, "name1")); + + names.clear(); + ASSERT_EQ(0, rbd.namespace_list(ioctx, &names)); + ASSERT_EQ(1U, names.size()); + ASSERT_EQ("name3", names[0]); } // poorman's assert() -- 2.39.5