From fc3a1e076a5da1378df6f3dc1aea30699cad8df3 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Sat, 25 Sep 2021 19:30:25 +0300 Subject: [PATCH] Fix minor bugs in snapshot removal, check it in tests --- src/cmd.cpp | 58 +++++++++++++++++++++++++++++------------- src/messenger.cpp | 2 +- src/messenger.h | 2 +- src/mock/messenger.cpp | 2 +- src/msgr_stop.cpp | 4 +-- tests/test_snapshot.sh | 11 ++++---- 6 files changed, 50 insertions(+), 29 deletions(-) diff --git a/src/cmd.cpp b/src/cmd.cpp index 60d9ca39..b787dc3a 100644 --- a/src/cmd.cpp +++ b/src/cmd.cpp @@ -125,6 +125,7 @@ public: exit(1); } inode_config_t *cur_cfg = &cur_cfg_it->second; + std::string cur_name = cur_cfg->name; std::string cur_cfg_key = base64_encode(cli->st_cli.etcd_prefix+ "/config/inode/"+std::to_string(INODE_POOL(cur))+ "/"+std::to_string(INODE_NO_POOL(cur))); @@ -149,7 +150,7 @@ public: { "target", "MOD" }, { "key", cur_cfg_key }, { "result", "LESS" }, - { "mod_revision", cur_cfg->mod_revision }, + { "mod_revision", cur_cfg->mod_revision+1 }, }, } }, { "success", json11::Json::array { @@ -160,18 +161,36 @@ public: } } }, } }, - }, ETCD_SLOW_TIMEOUT, [this, cur_cfg](std::string err, json11::Json res) + }, ETCD_SLOW_TIMEOUT, [this, new_parent, cur, cur_name](std::string err, json11::Json res) { if (err != "") { - fprintf(stderr, "Error changing parent of %s: %s\n", cur_cfg->name.c_str(), err.c_str()); + fprintf(stderr, "Error changing parent of %s: %s\n", cur_name.c_str(), err.c_str()); exit(1); } if (!res["succeeded"].bool_value()) { - fprintf(stderr, "Inode %s was modified during snapshot deletion\n", cur_cfg->name.c_str()); + fprintf(stderr, "Inode %s was modified during snapshot deletion\n", cur_name.c_str()); exit(1); } + if (new_parent) + { + auto new_parent_it = cli->st_cli.inode_config.find(new_parent); + std::string new_parent_name = new_parent_it != cli->st_cli.inode_config.end() + ? new_parent_it->second.name : ""; + printf( + "Parent of layer %s (inode %lu in pool %u) changed to %s (inode %lu in pool %u)\n", + cur_name.c_str(), INODE_NO_POOL(cur), INODE_POOL(cur), + new_parent_name.c_str(), INODE_NO_POOL(new_parent), INODE_POOL(new_parent) + ); + } + else + { + printf( + "Parent of layer %s (inode %lu in pool %u) detached\n", + cur_name.c_str(), INODE_NO_POOL(cur), INODE_POOL(cur) + ); + } waiting--; ringloop->wakeup(); }); @@ -218,7 +237,7 @@ struct rm_inode_t }); if (!lister) { - fprintf(stderr, "Failed to list inode %lx objects\n", inode); + fprintf(stderr, "Failed to list inode %lu from pool %u objects\n", INODE_NO_POOL(inode), INODE_POOL(inode)); exit(1); } pgs_to_list = parent->cli->list_pg_count(lister); @@ -305,7 +324,7 @@ struct rm_inode_t } if (lists_done && !lists.size()) { - printf("Done, inode %lu in pool %u removed\n", (inode & ((1l << (64-POOL_ID_BITS)) - 1)), pool_id); + printf("Done, inode %lu in pool %u data removed\n", INODE_NO_POOL(inode), pool_id); finished = true; } } @@ -477,9 +496,9 @@ struct snap_merger_t } sources.erase(target); printf( - "Merging %ld layer(s) into target %s%s (inode %lx)\n", + "Merging %ld layer(s) into target %s%s (inode %lu in pool %u)\n", sources.size(), target_cfg->name.c_str(), - use_cas ? " online (with CAS)" : "", target + use_cas ? " online (with CAS)" : "", INODE_NO_POOL(target), INODE_POOL(target) ); target_block_size = get_block_size(target); continue_merge_reent(); @@ -639,7 +658,7 @@ struct snap_merger_t if (status & INODE_LIST_DONE) { auto & name = parent->cli->st_cli.inode_config.at(src).name; - printf("Got listing of layer %s (inode %lx)\n", name.c_str(), src); + printf("Got listing of layer %s (inode %lu in pool %u)\n", name.c_str(), INODE_NO_POOL(src), INODE_POOL(src)); if (delete_source) { // Sort the inode listing @@ -1117,6 +1136,7 @@ resume_4: if (parent->waiting > 0) return; } + state = 5; resume_5: // Done return; @@ -1131,6 +1151,7 @@ resume_5: exit(1); } inode_config_t *cur_cfg = &cur_cfg_it->second; + std::string cur_name = cur_cfg->name; std::string cur_cfg_key = base64_encode(parent->cli->st_cli.etcd_prefix+ "/config/inode/"+std::to_string(INODE_POOL(cur))+ "/"+std::to_string(INODE_NO_POOL(cur))); @@ -1141,7 +1162,7 @@ resume_5: { "target", "MOD" }, { "key", cur_cfg_key }, { "result", "LESS" }, - { "mod_revision", cur_cfg->mod_revision }, + { "mod_revision", cur_cfg->mod_revision+1 }, }, } }, { "success", json11::Json::array { @@ -1151,18 +1172,19 @@ resume_5: } } }, } }, - }, ETCD_SLOW_TIMEOUT, [this, cur_cfg](std::string err, json11::Json res) + }, ETCD_SLOW_TIMEOUT, [this, cur_name](std::string err, json11::Json res) { if (err != "") { - fprintf(stderr, "Error deleting %s: %s\n", cur_cfg->name.c_str(), err.c_str()); + fprintf(stderr, "Error deleting %s: %s\n", cur_name.c_str(), err.c_str()); exit(1); } if (!res["succeeded"].bool_value()) { - fprintf(stderr, "Inode %s was modified during deletion\n", cur_cfg->name.c_str()); + fprintf(stderr, "Layer %s configuration was modified during deletion\n", cur_name.c_str()); exit(1); } + printf("Layer %s deleted\n", cur_name.c_str()); parent->waiting--; parent->ringloop->wakeup(); }); @@ -1232,8 +1254,8 @@ void cli_tool_t::run(json11::Json cfg) // Merge layer data without affecting metadata merger = new snap_merger_t(); merger->parent = this; - merger->from_name = cmd[1].string_value(); - merger->to_name = cmd[2].string_value(); + merger->from_name = cmd.size() > 1 ? cmd[1].string_value() : ""; + merger->to_name = cmd.size() > 2 ? cmd[2].string_value() : ""; merger->target_name = cfg["target"].string_value(); if (merger->from_name == "" || merger->to_name == "") { @@ -1252,7 +1274,7 @@ void cli_tool_t::run(json11::Json cfg) // Merge layer data without affecting metadata flattener = new snap_flattener_t(); flattener->parent = this; - flattener->target_name = cmd[1].string_value(); + flattener->target_name = cmd.size() > 1 ? cmd[1].string_value() : ""; if (flattener->target_name == "") { fprintf(stderr, "Layer to flatten argument is missing\n"); @@ -1269,8 +1291,8 @@ void cli_tool_t::run(json11::Json cfg) // Remove multiple snapshots and rebase their children snap_remover = new snap_remover_t(); snap_remover->parent = this; - snap_remover->from_name = cmd[1].string_value(); - snap_remover->to_name = cmd[2].string_value(); + snap_remover->from_name = cmd.size() > 1 ? cmd[1].string_value() : ""; + snap_remover->to_name = cmd.size() > 2 ? cmd[2].string_value() : ""; if (snap_remover->from_name == "") { fprintf(stderr, "Layer to remove argument is missing\n"); diff --git a/src/messenger.cpp b/src/messenger.cpp index 747402b6..1a0b7926 100644 --- a/src/messenger.cpp +++ b/src/messenger.cpp @@ -117,7 +117,7 @@ osd_messenger_t::~osd_messenger_t() } while (clients.size() > 0) { - stop_client(clients.begin()->first, true); + stop_client(clients.begin()->first, true, true); } #ifdef WITH_RDMA if (rdma_context) diff --git a/src/messenger.h b/src/messenger.h index 42d58980..09d1e0f7 100644 --- a/src/messenger.h +++ b/src/messenger.h @@ -156,7 +156,7 @@ public: void init(); void parse_config(const json11::Json & config); void connect_peer(uint64_t osd_num, json11::Json peer_state); - void stop_client(int peer_fd, bool force = false); + void stop_client(int peer_fd, bool force = false, bool force_delete = false); void outbox_push(osd_op_t *cur_op); std::function exec_op; std::function repeer_pgs; diff --git a/src/mock/messenger.cpp b/src/mock/messenger.cpp index 7ba9ffa4..00a41639 100644 --- a/src/mock/messenger.cpp +++ b/src/mock/messenger.cpp @@ -15,7 +15,7 @@ osd_messenger_t::~osd_messenger_t() { while (clients.size() > 0) { - stop_client(clients.begin()->first, true); + stop_client(clients.begin()->first, true, true); } } diff --git a/src/msgr_stop.cpp b/src/msgr_stop.cpp index cf8de660..29776666 100644 --- a/src/msgr_stop.cpp +++ b/src/msgr_stop.cpp @@ -41,7 +41,7 @@ void osd_messenger_t::cancel_op(osd_op_t *op) } } -void osd_messenger_t::stop_client(int peer_fd, bool force) +void osd_messenger_t::stop_client(int peer_fd, bool force, bool force_delete) { assert(peer_fd != 0); auto it = clients.find(peer_fd); @@ -136,7 +136,7 @@ void osd_messenger_t::stop_client(int peer_fd, bool force) clients.erase(it); } cl->refs--; - if (cl->refs <= 0) + if (cl->refs <= 0 || force_delete) { delete cl; } diff --git a/tests/test_snapshot.sh b/tests/test_snapshot.sh index 9e8b3bcf..91c7b214 100755 --- a/tests/test_snapshot.sh +++ b/tests/test_snapshot.sh @@ -6,18 +6,19 @@ $ETCDCTL put /vitastor/config/inode/1/2 '{"name":"testimg","size":'$((32*1024*1024))'}' -LD_PRELOAD=libasan.so.5 \ +LD_PRELOAD="libasan.so.5 build/src/libfio_vitastor.so" \ fio -thread -name=test -ioengine=build/src/libfio_vitastor.so -bs=4M -direct=1 -iodepth=1 -fsync=1 -rw=write \ -etcd=$ETCD_URL -pool=1 -inode=2 -size=32M -cluster_log_level=10 $ETCDCTL put /vitastor/config/inode/1/2 '{"name":"testimg@0","size":'$((32*1024*1024))'}' $ETCDCTL put /vitastor/config/inode/1/3 '{"parent_id":2,"name":"testimg","size":'$((32*1024*1024))'}' -LD_PRELOAD=libasan.so.5 \ +# Preload build/src/libfio_vitastor.so so libasan detects all symbols +LD_PRELOAD="libasan.so.5 build/src/libfio_vitastor.so" \ fio -thread -name=test -ioengine=build/src/libfio_vitastor.so -bs=4k -direct=1 -iodepth=1 -fsync=32 -buffer_pattern=0xdeadface \ -rw=randwrite -etcd=$ETCD_URL -image=testimg -number_ios=1024 -LD_PRELOAD=libasan.so.5 \ +LD_PRELOAD="libasan.so.5 build/src/libfio_vitastor.so" \ fio -thread -name=test -ioengine=build/src/libfio_vitastor.so -bs=4M -direct=1 -iodepth=1 -rw=read -etcd=$ETCD_URL -pool=1 -inode=3 -size=32M qemu-img convert -S 4096 -p \ @@ -42,9 +43,7 @@ cmp ./testdata/merged.bin ./testdata/check.bin $ETCDCTL put /vitastor/config/inode/1/3 '{"parent_id":2,"name":"testimg","size":'$((32*1024*1024))'}' -build/src/vitastor-cmd merge --etcd_address $ETCD_URL testimg@0 testimg --target testimg - -$ETCDCTL put /vitastor/config/inode/1/3 '{"name":"testimg","size":'$((32*1024*1024))'}' +build/src/vitastor-cmd snap-rm --etcd_address $ETCD_URL testimg@0 qemu-img convert -S 4096 -p \ -f raw "vitastor:etcd_host=127.0.0.1\:$ETCD_PORT/v3:image=testimg" \