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

@@ -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<void(inode_list_t* lst, std::set<object_id>&& 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<osd_num_t> & list_inode_get_inactive_osds(inode_list_t *lst);
+    const std::vector<pg_num_t> & 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;

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<osd_num_t> inactive_osds;
+    std::vector<pg_num_t> inactive_pgs;
     std::vector<inode_list_pg_t*> pgs;
     std::function<void(inode_list_t* lst, std::set<object_id>&& 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<void(inode_list_t* lst, std::set<object_id>&& 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<osd_num_t> & cluster_client_t::list_inode_get_inactive_osds(in
     return lst->inactive_osds;
 }
 
+const std::vector<pg_num_t> & 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)

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)
             {

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());

src/cmd/cli_rm_data.cpp

@@ -32,6 +32,7 @@ struct rm_inode_t
     inode_list_t *lister = NULL;
     std::vector<rm_pg_t*> lists;
     std::vector<osd_num_t> inactive_osds;
+    std::vector<uint64_t> 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;

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