From 5c890e4a12cae427364506a56acf1944f0a939c8 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Fri, 27 Dec 2024 16:29:33 +0300 Subject: [PATCH] Fix rm-data hanging when some OSDs are inactive, add a test for it There's also another case which also needs to be fixed - we shouldn't retry deletions for indefinite time if an OSD is stopped during deletion --- src/client/cluster_client.h | 4 + src/client/cluster_client_list.cpp | 122 ++++++++++++++++++++--------- src/cmd/cli_ls.cpp | 1 + src/cmd/cli_rm.cpp | 1 + src/cmd/cli_rm_data.cpp | 17 +++- tests/test_rm_degraded.sh | 25 ++++++ 6 files changed, 129 insertions(+), 41 deletions(-) create mode 100755 tests/test_rm_degraded.sh diff --git a/src/client/cluster_client.h b/src/client/cluster_client.h index ea4dd9d2..05d218d7 100644 --- a/src/client/cluster_client.h +++ b/src/client/cluster_client.h @@ -71,6 +71,7 @@ protected: struct inode_list_t; struct inode_list_osd_t; +struct inode_list_pg_t; class writeback_cache_t; // FIXME: Split into public and private interfaces @@ -139,6 +140,7 @@ public: std::function&& objects, pg_num_t pg_num, osd_num_t primary_osd, int status)> callback); int list_pg_count(inode_list_t *lst); const std::vector & list_inode_get_inactive_osds(inode_list_t *lst); + const std::vector & list_inode_get_inactive_pgs(inode_list_t *lst); void list_inode_next(inode_list_t *lst, int next_pgs); //inline uint32_t get_bs_bitmap_granularity() { return st_cli.global_bitmap_granularity; } //inline uint64_t get_bs_block_size() { return st_cli.global_block_size; } @@ -169,6 +171,8 @@ protected: void continue_lists(); void continue_listing(inode_list_t *lst); void send_list(inode_list_osd_t *cur_list); + void finish_list_pg(inode_list_pg_t *pg); + bool check_finish_listing(inode_list_t *lst); void continue_raw_ops(osd_num_t peer_osd); friend class writeback_cache_t; diff --git a/src/client/cluster_client_list.cpp b/src/client/cluster_client_list.cpp index b0000632..2383aefa 100644 --- a/src/client/cluster_client_list.cpp +++ b/src/client/cluster_client_list.cpp @@ -36,7 +36,9 @@ struct inode_list_t inode_t inode = 0; int done_pgs = 0; int want = 0; + int onstack = 0; std::vector inactive_osds; + std::vector inactive_pgs; std::vector pgs; std::function&& objects, pg_num_t pg_num, osd_num_t primary_osd, int status)> callback; }; @@ -45,7 +47,6 @@ inode_list_t* cluster_client_t::list_inode_start(inode_t inode, std::function&& objects, pg_num_t pg_num, osd_num_t primary_osd, int status)> callback) { init_msgr(); - int skipped_pgs = 0; pool_id_t pool_id = INODE_POOL(inode); if (!pool_id || st_cli.pool_config.find(pool_id) == st_cli.pool_config.end()) { @@ -67,7 +68,7 @@ inode_list_t* cluster_client_t::list_inode_start(inode_t inode, auto & pg = pg_item.second; if (pg.pause || !pg.cur_primary || !(pg.cur_state & PG_ACTIVE)) { - skipped_pgs++; + lst->inactive_pgs.push_back(pg_item.first); if (log_level > 0) { fprintf(stderr, "PG %u is inactive, skipping\n", pg_item.first); @@ -122,7 +123,9 @@ inode_list_t* cluster_client_t::list_inode_start(inode_t inode, } } } - else + // FIXME: We should retry (or at least OPTIONALLY retry) connecting during delete, but only _for_some_time_ + // FIXME: Also we should handle the case when the OSD is disconnected during delete + else if (st_cli.peer_states.find(pg.cur_primary) != st_cli.peer_states.end()) { // Clean r->list_osds.push_back((inode_list_osd_t){ @@ -131,6 +134,10 @@ inode_list_t* cluster_client_t::list_inode_start(inode_t inode, .sent = false, }); } + else + { + inactive_osd_set.insert(pg.cur_primary); + } lst->pgs.push_back(r); } std::sort(lst->pgs.begin(), lst->pgs.end(), [](inode_list_pg_t *a, inode_list_pg_t *b) @@ -156,6 +163,11 @@ const std::vector & cluster_client_t::list_inode_get_inactive_osds(in return lst->inactive_osds; } +const std::vector & cluster_client_t::list_inode_get_inactive_pgs(inode_list_t *lst) +{ + return lst->inactive_pgs; +} + void cluster_client_t::list_inode_next(inode_list_t *lst, int next_pgs) { if (next_pgs >= 0) @@ -175,20 +187,39 @@ void cluster_client_t::continue_listing(inode_list_t *lst) { return; } + if (lst->onstack > 0) + { + return; + } + lst->onstack++; for (int i = 0; i < lst->pgs.size(); i++) { - if (lst->pgs[i] && lst->pgs[i]->sent < lst->pgs[i]->list_osds.size()) + if (!lst->pgs[i]) + { + } + else if (lst->pgs[i]->sent < lst->pgs[i]->list_osds.size()) { for (int j = 0; j < lst->pgs[i]->list_osds.size(); j++) { send_list(&lst->pgs[i]->list_osds[j]); if (lst->want <= 0) { + lst->onstack--; return; } } } + else if (!lst->pgs[i]->list_osds.size()) + { + finish_list_pg(lst->pgs[i]); + if (check_finish_listing(lst)) + { + // Do not change lst->onstack because it's already freed + return; + } + } } + lst->onstack--; } void cluster_client_t::send_list(inode_list_osd_t *cur_list) @@ -257,43 +288,12 @@ void cluster_client_t::send_list(inode_list_osd_t *cur_list) } delete op; auto lst = cur_list->pg->lst; - auto pg = cur_list->pg; - pg->done++; - if (pg->done >= pg->list_osds.size()) + cur_list->pg->done++; + finish_list_pg(cur_list->pg); + if (!check_finish_listing(lst)) { - int status = 0; - lst->done_pgs++; - if (lst->done_pgs >= lst->pgs.size()) - { - status |= INODE_LIST_DONE; - } - if (pg->has_unstable) - { - status |= INODE_LIST_HAS_UNSTABLE; - } - lst->callback(lst, std::move(pg->objects), pg->pg_num, pg->cur_primary, status); - lst->pgs[pg->pos] = NULL; - delete pg; - if (lst->done_pgs >= lst->pgs.size()) - { - // All done - for (int i = 0; i < lists.size(); i++) - { - if (lists[i] == lst) - { - lists.erase(lists.begin()+i, lists.begin()+i+1); - break; - } - } - delete lst; - return; - } + continue_listing(lst); } - else - { - lst->want++; - } - continue_listing(lst); }; msgr.outbox_push(op); cur_list->sent = true; @@ -301,6 +301,50 @@ void cluster_client_t::send_list(inode_list_osd_t *cur_list) cur_list->pg->lst->want--; } +void cluster_client_t::finish_list_pg(inode_list_pg_t *pg) +{ + auto lst = pg->lst; + if (pg->done >= pg->list_osds.size()) + { + int status = 0; + lst->done_pgs++; + if (lst->done_pgs >= lst->pgs.size()) + { + status |= INODE_LIST_DONE; + } + if (pg->has_unstable) + { + status |= INODE_LIST_HAS_UNSTABLE; + } + lst->pgs[pg->pos] = NULL; + lst->callback(lst, std::move(pg->objects), pg->pg_num, pg->cur_primary, status); + delete pg; + } + else + { + lst->want++; + } +} + +bool cluster_client_t::check_finish_listing(inode_list_t *lst) +{ + if (lst->done_pgs >= lst->pgs.size()) + { + // All done + for (int i = 0; i < lists.size(); i++) + { + if (lists[i] == lst) + { + lists.erase(lists.begin()+i, lists.begin()+i+1); + break; + } + } + delete lst; + return true; + } + return false; +} + void cluster_client_t::continue_lists() { for (auto lst: lists) diff --git a/src/cmd/cli_ls.cpp b/src/cmd/cli_ls.cpp index bab15856..5c73c0d3 100644 --- a/src/cmd/cli_ls.cpp +++ b/src/cmd/cli_ls.cpp @@ -68,6 +68,7 @@ struct image_lister_t { "pool_name", good_pool ? pool_it->second.name : "? (ID:"+std::to_string(INODE_POOL(ic.second.num))+")" }, { "inode_num", INODE_NO_POOL(ic.second.num) }, { "inode_id", ic.second.num }, + { "deleted", ic.second.deleted }, }; if (ic.second.parent_id) { diff --git a/src/cmd/cli_rm.cpp b/src/cmd/cli_rm.cpp index 18b86b45..d19a9764 100644 --- a/src/cmd/cli_rm.cpp +++ b/src/cmd/cli_rm.cpp @@ -230,6 +230,7 @@ resume_100: { result.data = my_result(result.data); state = 100; + return false; } else if (parent->progress) printf("%s\n", result.text.c_str()); diff --git a/src/cmd/cli_rm_data.cpp b/src/cmd/cli_rm_data.cpp index 69110862..6a89368f 100644 --- a/src/cmd/cli_rm_data.cpp +++ b/src/cmd/cli_rm_data.cpp @@ -32,6 +32,7 @@ struct rm_inode_t inode_list_t *lister = NULL; std::vector lists; std::vector inactive_osds; + std::vector inactive_pgs; uint64_t total_count = 0, total_done = 0, total_prev_pct = 0; uint64_t pgs_to_list = 0; bool lists_done = false; @@ -51,7 +52,7 @@ struct rm_inode_t .objects = objects, .obj_count = objects.size(), .obj_done = 0, - .synced = parent->cli->get_immediate_commit(inode), + .synced = !objects.size() || parent->cli->get_immediate_commit(inode), }); if (min_offset == 0 && max_offset == 0) { @@ -101,6 +102,17 @@ struct rm_inode_t } fprintf(stderr, "\n"); } + auto inactive_pgs = parent->cli->list_inode_get_inactive_pgs(lister); + if (inactive_pgs.size() && !parent->json_output) + { + fprintf(stderr, "Some data may remain after delete in PGs which are currently inactive: "); + for (int i = 0; i < inactive_pgs.size(); i++) + { + this->inactive_pgs.push_back((uint64_t)inactive_pgs[i]); + fprintf(stderr, i > 0 ? ", %u" : "%u", inactive_pgs[i]); + } + fprintf(stderr, "\n"); + } pgs_to_list = parent->cli->list_pg_count(lister); parent->cli->list_inode_next(lister, parent->parallel_osds); } @@ -225,7 +237,7 @@ struct rm_inode_t { fprintf(stderr, "\n"); } - bool is_error = (total_done < total_count || inactive_osds.size() > 0 || error_count > 0); + bool is_error = (total_done < total_count || inactive_osds.size() > 0 || inactive_pgs.size() > 0 || error_count > 0); if (parent->progress && is_error) { fprintf( @@ -243,6 +255,7 @@ struct rm_inode_t { "removed_objects", total_done }, { "total_objects", total_count }, { "inactive_osds", inactive_osds }, + { "inactive_pgs", inactive_pgs }, }, }; state = 100; diff --git a/tests/test_rm_degraded.sh b/tests/test_rm_degraded.sh new file mode 100755 index 00000000..0296bf09 --- /dev/null +++ b/tests/test_rm_degraded.sh @@ -0,0 +1,25 @@ +#!/bin/bash -ex + +SCHEME=xor +PG_COUNT=16 +PG_MINSIZE=2 +. `dirname $0`/run_3osds.sh + +build/src/cmd/vitastor-cli --etcd_address $ETCD_URL create -s 128M testimg + +LD_PRELOAD="build/src/client/libfio_vitastor.so" \ + fio -thread -name=test -ioengine=build/src/client/libfio_vitastor.so -bs=4M -direct=1 -iodepth=1 \ + -end_fsync=1 -fsync=1 -rw=write -etcd=$ETCD_URL -image=testimg -size=128M -cluster_log_level=10 + +kill -9 $OSD3_PID +$ETCDCTL del /vitastor/osd/state/3 + +if build/src/cmd/vitastor-cli --etcd_address $ETCD_URL rm testimg --log_level 10 ; then + format_error "Delete should not be successful with inactive OSDs" +fi + +if ! ( build/src/cmd/vitastor-cli --etcd_address $ETCD_URL ls | grep testimg | grep DEL ) ; then + format_error "Image should be marked as partially deleted" +fi + +format_green OK