Fix EC PGs hanging in REPEERING when flush error is last in the batch

src/osd/osd_flush.cpp

@@ -79,9 +79,9 @@ void osd_t::handle_flush_op(bool rollback, pool_id_t pool_id, pg_num_t pg_num, p
         // Throw the result away
         return;
     }
-    fb->flush_done++;
     if (retval != 0)
     {
+        fb->flush_errors++;
         if (peer_osd == this->osd_num)
         {
             throw std::runtime_error(
@@ -97,17 +97,26 @@ void osd_t::handle_flush_op(bool rollback, pool_id_t pool_id, pg_num_t pg_num, p
             auto peer_it = msgr.osd_peers.find(peer_osd);
             if (peer_it != msgr.osd_peers.end())
             {
-                // Will repeer/stop this PG
+                // stop_client won't repeer the PG because flush_batch is not deleted yet
                 msgr.stop_client(peer_it->second->client_id);
-                return;
             }
         }
     }
+    auto errors = fb->flush_errors;
+    fb->flush_done++;
     if (fb->flush_done == fb->flush_ops)
     {
         // This flush batch is done
-        std::vector<osd_op_t*> continue_ops;
         auto & pg = pgs.at(pg_id);
+        delete fb;
+        pg.flush_batch = NULL;
+        if (errors > 0)
+        {
+            // Repeer the PG on errors
+            repeer_pg(pg);
+            return;
+        }
+        std::vector<osd_op_t*> continue_ops;
         auto it = pg.flush_actions.begin(), prev_it = it;
         while (1)
         {
@@ -143,8 +152,6 @@ void osd_t::handle_flush_op(bool rollback, pool_id_t pool_id, pg_num_t pg_num, p
             }
             prev_it = it++;
         }
-        delete fb;
-        pg.flush_batch = NULL;
         if (!pg.flush_actions.size())
         {
             pg.state = pg.state & ~PG_HAS_UNCLEAN;

src/osd/osd_peering_pg.h

@@ -78,7 +78,7 @@ struct pg_flush_batch_t
 {
     std::map<osd_num_t, std::vector<obj_ver_id>> rollback_lists;
     std::map<osd_num_t, std::vector<obj_ver_id>> stable_lists;
-    int flush_ops = 0, flush_done = 0;
+    int flush_ops = 0, flush_done = 0, flush_errors = 0;
     int flush_objects = 0;
 };
 

src/test/osd_test.cpp

@@ -1138,6 +1138,77 @@ void test_chained_read_eio_retry()
     printf("test_chained_read_eio_retry passed\n");
 }
 
+// Regression test for the bug in osd_flush.cpp where a remote
+// stabilize/rollback failure permanently hangs the PG in PG_REPEERING.
+bool test_flush_error_pg_repeer(int ctr)
+{
+    printf("test_flush_error_pg_repeer\n");
+
+    osd_test_fixture_t f;
+    f.configure_ec_pool(2, 1);
+    f.start(json11::Json::object {
+        { "osd_num", 2 },
+        { "etcd_address", "127.0.0.1:2379" },
+        { "immediate_commit", "all" },
+        { "block_size", 131072 },
+        { "bitmap_granularity", 4096 },
+    });
+    f.connect_peer(1);
+    f.connect_peer(3);
+
+    // Object exists on all osds but unstable on osd 1 and 3
+    inode_t ino = INODE_WITH_POOL(1, 1);
+    f.reply_peer_list(1, { { { ino, 0 }, 1 } }, /*stable_count*/ 0);
+    f.reply_local_list({ { { ino, 1 }, 1 } }, /*stable_count*/ 1);
+    f.reply_peer_list(3, { { { ino, 2 }, 1 } }, /*stable_count*/ 0);
+    f.pump();
+
+    auto & pg = f.pg(1, 1);
+    assert(pg.state == (PG_ACTIVE|PG_HAS_UNCLEAN));
+    auto fb = pg.flush_batch;
+    assert(fb != nullptr);
+    assert(fb->flush_ops == 2);
+
+    if (ctr == 0)
+    {
+        // Complete OP_SEC_STAB on osd 1, then fail on osd 3
+        auto *stab = f.peer_take(1, OSD_OP_SEC_STABILIZE);
+        stab->reply.hdr.retval = 0;
+        stab->callback(stab);
+
+        assert(pg.state == (PG_ACTIVE|PG_HAS_UNCLEAN));
+
+        stab = f.peer_take(3, OSD_OP_SEC_STABILIZE);
+        stab->reply.hdr.retval = -EPIPE;
+        stab->callback(stab);
+    }
+    else
+    {
+        // Fail OP_SEC_STAB on osd 3, then complete on osd 1
+        auto *stab = f.peer_take(3, OSD_OP_SEC_STABILIZE);
+        stab->reply.hdr.retval = -EPIPE;
+        stab->callback(stab);
+
+        // The PG should only repeer when the batch is completed
+        assert(pg.state == (PG_HAS_UNCLEAN|PG_REPEERING));
+        assert(pg.flush_batch == fb);
+
+        stab = f.peer_take(1, OSD_OP_SEC_STABILIZE);
+        stab->reply.hdr.retval = 0;
+        stab->callback(stab);
+    }
+
+    // Now the PG should be peering
+    assert(pg.state == PG_PEERING);
+    assert(!pg.flush_batch);
+
+    // FIXME: Make this test pass without leaks without the following line:
+    f.complete_peering_empty();
+
+    printf("test_flush_error_pg_repeer[%d] passed\n", ctr);
+    return ctr < 1;
+}
+
 int main(int narg, char *args[])
 {
     test_load_global_config();
@@ -1149,5 +1220,6 @@ int main(int narg, char *args[])
     test_ec42_write_parityless();
     test_ec33_chain_read_phantom_bitmap_source();
     test_chained_read_eio_retry();
+    for (int i = 0; test_flush_error_pg_repeer(i); i++) {}
     return 0;
 }