Merge pull request #59107 from nbalacha/wip-nbalacha-async-sorted-snaps

librbd: make "group snap list" async and optionally sorted by snap creation time

Reviewed-by: Ramana Raja <rraja@redhat.com>
Reviewed-by: Mykola Golub <mgolub@suse.com>
Reviewed-by: Ilya Dryomov <idryomov@gmail.com>

10 files changed, 480 insertions, 60 deletions

diff --git a/src/cls/rbd/cls_rbd_client.cc b/src/cls/rbd/cls_rbd_client.cc
index ad480c47d5c..458bfd985c3 100644
--- a/src/cls/rbd/cls_rbd_client.cc
+++ b/src/cls/rbd/cls_rbd_client.cc
@@ -2757,28 +2757,65 @@ int group_snap_get_by_id(librados::IoCtx *ioctx, const std::string &oid,
 
   return 0;
 }
+
+void group_snap_list_start(librados::ObjectReadOperation *op,
+                           const cls::rbd::GroupSnapshot &start,
+                           uint64_t max_return)
+{
+  bufferlist bl;
+  encode(start, bl);
+  encode(max_return, bl);
+
+  op->exec("rbd", "group_snap_list", bl);
+}
+
+int group_snap_list_finish(bufferlist::const_iterator *iter,
+                           std::vector<cls::rbd::GroupSnapshot> *snapshots)
+{
+  try {
+    decode(*snapshots, *iter);
+  } catch (const ceph::buffer::error &err) {
+    return -EBADMSG;
+  }
+  return 0;
+}
+
 int group_snap_list(librados::IoCtx *ioctx, const std::string &oid,
                     const cls::rbd::GroupSnapshot &start,
                     uint64_t max_return,
                     std::vector<cls::rbd::GroupSnapshot> *snapshots)
 {
-  using ceph::encode;
-  using ceph::decode;
-  bufferlist inbl, outbl;
-  encode(start, inbl);
-  encode(max_return, inbl);
+  librados::ObjectReadOperation op;
+  group_snap_list_start(&op, start, max_return);
 
-  int r = ioctx->exec(oid, "rbd", "group_snap_list", inbl, outbl);
+  bufferlist out_bl;
+  int r = ioctx->operate(oid, &op, &out_bl);
   if (r < 0) {
     return r;
   }
-  auto iter = outbl.cbegin();
+
+  auto it = out_bl.cbegin();
+  return group_snap_list_finish(&it, snapshots);
+}
+
+void group_snap_list_order_start(librados::ObjectReadOperation *op,
+                                 const std::string &start,
+                                 uint64_t max_return)
+{
+  bufferlist bl;
+  encode(start, bl);
+  encode(max_return, bl);
+  op->exec("rbd", "group_snap_list_order", bl);
+}
+
+int group_snap_list_order_finish(bufferlist::const_iterator *iter,
+                                 std::map<std::string, uint64_t> *snap_order)
+{
   try {
-    decode(*snapshots, iter);
+    decode(*snap_order, *iter);
   } catch (const ceph::buffer::error &err) {
     return -EBADMSG;
   }
-
   return 0;
 }
 
@@ -2786,24 +2823,17 @@ int group_snap_list_order(librados::IoCtx *ioctx, const std::string &oid,
                           const std::string &start, uint64_t max_return,
                           std::map<std::string, uint64_t> *snap_order)
 {
-  using ceph::encode;
-  using ceph::decode;
-  bufferlist inbl, outbl;
-  encode(start, inbl);
-  encode(max_return, inbl);
+  librados::ObjectReadOperation op;
+  group_snap_list_order_start(&op, start, max_return);
 
-  int r = ioctx->exec(oid, "rbd", "group_snap_list_order", inbl, outbl);
+  bufferlist out_bl;
+  int r = ioctx->operate(oid, &op, &out_bl);
   if (r < 0) {
     return r;
   }
-  auto iter = outbl.cbegin();
-  try {
-    decode(*snap_order, iter);
-  } catch (const ceph::buffer::error &err) {
-    return -EBADMSG;
-  }
 
-  return 0;
+  auto it = out_bl.cbegin();
+  return group_snap_list_order_finish(&it, snap_order);
 }
 
 // rbd_trash functions
diff --git a/src/cls/rbd/cls_rbd_client.h b/src/cls/rbd/cls_rbd_client.h
index 4005c51836c..b1553bd1f17 100644
--- a/src/cls/rbd/cls_rbd_client.h
+++ b/src/cls/rbd/cls_rbd_client.h
@@ -580,10 +580,20 @@ int group_snap_remove(librados::IoCtx *ioctx, const std::string &oid,
 int group_snap_get_by_id(librados::IoCtx *ioctx, const std::string &oid,
                          const std::string &snap_id,
                          cls::rbd::GroupSnapshot *snapshot);
+void group_snap_list_start(librados::ObjectReadOperation *op,
+                           const cls::rbd::GroupSnapshot &start,
+                           uint64_t max_return);
+int group_snap_list_finish(ceph::buffer::list::const_iterator *iter,
+                           std::vector<cls::rbd::GroupSnapshot> *snapshots);
 int group_snap_list(librados::IoCtx *ioctx, const std::string &oid,
                     const cls::rbd::GroupSnapshot &start,
                     uint64_t max_return,
                     std::vector<cls::rbd::GroupSnapshot> *snapshots);
+void group_snap_list_order_start(librados::ObjectReadOperation *op,
+                                 const std::string &start_snap_id,
+                                 uint64_t max_return);
+int group_snap_list_order_finish(ceph::buffer::list::const_iterator *iter,
+                                 std::map<std::string, uint64_t> *snap_order);
 int group_snap_list_order(librados::IoCtx *ioctx, const std::string &oid,
                           const std::string &snap_id, uint64_t max_return,
                           std::map<std::string, uint64_t> *snap_order);
diff --git a/src/librbd/CMakeLists.txt b/src/librbd/CMakeLists.txt
index a5975bf350b..b66e5ead40a 100644
--- a/src/librbd/CMakeLists.txt
+++ b/src/librbd/CMakeLists.txt
@@ -77,6 +77,7 @@ set(librbd_internal_srcs
   exclusive_lock/PostAcquireRequest.cc
   exclusive_lock/PreReleaseRequest.cc
   exclusive_lock/StandardPolicy.cc
+  group/ListSnapshotsRequest.cc
   image/AttachChildRequest.cc
   image/AttachParentRequest.cc
   image/CloneRequest.cc
diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc
index bf1c53284f2..97c53abd70f 100644
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -11,6 +11,7 @@
 #include "librbd/ImageWatcher.h"
 #include "librbd/Operations.h"
 #include "librbd/Utils.h"
+#include "librbd/group/ListSnapshotsRequest.h"
 #include "librbd/internal.h"
 #include "librbd/io/AioCompletion.h"
 
@@ -53,36 +54,18 @@ snap_t get_group_snap_id(I* ictx,
   return CEPH_NOSNAP;
 }
 
+template <typename I>
 int group_snap_list(librados::IoCtx& group_ioctx, const std::string& group_id,
-		    std::vector<cls::rbd::GroupSnapshot> *cls_snaps)
+                    bool try_to_sort, bool fail_if_not_sorted,
+                    std::vector<cls::rbd::GroupSnapshot> *cls_snaps)
 {
-  CephContext *cct = (CephContext *)group_ioctx.cct();
-
-  string group_header_oid = util::group_header_name(group_id);
-
-  const int max_read = 1024;
-  cls::rbd::GroupSnapshot snap_last;
-  int r;
-
-  for (;;) {
-    vector<cls::rbd::GroupSnapshot> snaps_page;
-
-    r = cls_client::group_snap_list(&group_ioctx, group_header_oid,
-				    snap_last, max_read, &snaps_page);
-
-    if (r < 0) {
-      lderr(cct) << "error reading snap list from group: "
-	<< cpp_strerror(-r) << dendl;
-      return r;
-    }
-    cls_snaps->insert(cls_snaps->end(), snaps_page.begin(), snaps_page.end());
-    if (snaps_page.size() < max_read) {
-      break;
-    }
-    snap_last = *snaps_page.rbegin();
-  }
-
-  return 0;
+  C_SaferCond cond;
+  auto req = group::ListSnapshotsRequest<I>::create(group_ioctx, group_id,
+                                                    try_to_sort,
+                                                    fail_if_not_sorted,
+                                                    cls_snaps, &cond);
+  req->send();
+  return cond.wait();
 }
 
 std::string calc_ind_image_snap_name(uint64_t pool_id,
@@ -593,7 +576,7 @@ int Group<I>::remove(librados::IoCtx& io_ctx, const char *group_name)
   string group_header_oid = util::group_header_name(group_id);
 
   std::vector<cls::rbd::GroupSnapshot> snaps;
-  r = group_snap_list(io_ctx, group_id, &snaps);
+  r = group_snap_list<I>(io_ctx, group_id, false, false, &snaps);
   if (r < 0 && r != -ENOENT) {
     lderr(cct) << "error listing group snapshots" << dendl;
     return r;
@@ -1191,7 +1174,7 @@ int Group<I>::snap_remove(librados::IoCtx& group_ioctx, const char *group_name,
   }
 
   std::vector<cls::rbd::GroupSnapshot> snaps;
-  r = group_snap_list(group_ioctx, group_id, &snaps);
+  r = group_snap_list<I>(group_ioctx, group_id, false, false, &snaps);
   if (r < 0) {
     return r;
   }
@@ -1232,7 +1215,7 @@ int Group<I>::snap_rename(librados::IoCtx& group_ioctx, const char *group_name,
   }
 
   std::vector<cls::rbd::GroupSnapshot> group_snaps;
-  r = group_snap_list(group_ioctx, group_id, &group_snaps);
+  r = group_snap_list<I>(group_ioctx, group_id, false, false, &group_snaps);
   if (r < 0) {
     return r;
   }
@@ -1261,6 +1244,7 @@ int Group<I>::snap_rename(librados::IoCtx& group_ioctx, const char *group_name,
 
 template <typename I>
 int Group<I>::snap_list(librados::IoCtx& group_ioctx, const char *group_name,
+                        bool try_to_sort, bool fail_if_not_sorted,
 			std::vector<group_snap_info2_t> *group_snaps)
 {
   CephContext *cct = (CephContext *)group_ioctx.cct();
@@ -1275,7 +1259,8 @@ int Group<I>::snap_list(librados::IoCtx& group_ioctx, const char *group_name,
   }
 
   std::vector<cls::rbd::GroupSnapshot> cls_group_snaps;
-  r = group_snap_list(group_ioctx, group_id, &cls_group_snaps);
+  r = group_snap_list<I>(group_ioctx, group_id, try_to_sort, fail_if_not_sorted,
+                         &cls_group_snaps);
   if (r < 0) {
     return r;
   }
@@ -1311,7 +1296,7 @@ int Group<I>::snap_get_info(librados::IoCtx& group_ioctx,
   }
 
   std::vector<cls::rbd::GroupSnapshot> cls_group_snaps;
-  r = group_snap_list(group_ioctx, group_id, &cls_group_snaps);
+  r = group_snap_list<I>(group_ioctx, group_id, false, false, &cls_group_snaps);
   if (r < 0) {
     return r;
   }
@@ -1353,7 +1338,7 @@ int Group<I>::snap_rollback(librados::IoCtx& group_ioctx,
   }
 
   std::vector<cls::rbd::GroupSnapshot> snaps;
-  r = group_snap_list(group_ioctx, group_id, &snaps);
+  r = group_snap_list<I>(group_ioctx, group_id, false, false, &snaps);
   if (r < 0) {
     return r;
   }
diff --git a/src/librbd/api/Group.h b/src/librbd/api/Group.h
index 2a7574612ec..ffbb9afea1a 100644
--- a/src/librbd/api/Group.h
+++ b/src/librbd/api/Group.h
@@ -47,6 +47,7 @@ struct Group {
   static int snap_rename(librados::IoCtx& group_ioctx, const char *group_name,
                          const char *old_snap_name, const char *new_snap_name);
   static int snap_list(librados::IoCtx& group_ioctx, const char *group_name,
+                       bool try_to_sort, bool fail_if_not_sorted,
                        std::vector<group_snap_info2_t> *snaps);
   static int snap_get_info(librados::IoCtx& group_ioctx,
                            const char *group_name, const char *snap_name,
diff --git a/src/librbd/group/ListSnapshotsRequest.cc b/src/librbd/group/ListSnapshotsRequest.cc
new file mode 100644
index 00000000000..bbd3759ff2b
--- /dev/null
+++ b/src/librbd/group/ListSnapshotsRequest.cc
@@ -0,0 +1,187 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#include "librbd/group/ListSnapshotsRequest.h"
+#include "include/ceph_assert.h"
+#include "common/dout.h"
+#include "common/errno.h"
+#include "common/ceph_context.h"
+#include "cls/rbd/cls_rbd_client.h"
+#include "librbd/Utils.h"
+
+#define dout_subsys ceph_subsys_rbd
+#undef dout_prefix
+#define dout_prefix *_dout << "librbd::group::ListSnapshotsRequest: " << this \
+                           << " " << __func__ << ": "
+
+namespace librbd {
+namespace group {
+
+namespace {
+
+const uint32_t MAX_RETURN = 1024;
+
+} // anonymous namespace
+
+template <typename I>
+ListSnapshotsRequest<I>::ListSnapshotsRequest(librados::IoCtx &group_io_ctx,
+                                              const std::string &group_id,
+                                              bool try_to_sort,
+                                              bool fail_if_not_sorted,
+                                              std::vector<cls::rbd::GroupSnapshot> *snaps,
+                                              Context *on_finish)
+     : m_group_io_ctx(group_io_ctx), m_group_id(group_id),
+       m_try_to_sort(try_to_sort), m_fail_if_not_sorted(fail_if_not_sorted),
+       m_snaps(snaps), m_on_finish(on_finish) {
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 20) << "group_id=" << m_group_id
+                 << ", try_to_sort=" << m_try_to_sort
+                 << ", fail_if_not_sorted=" << m_fail_if_not_sorted
+                 << dendl;
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::send() {
+  list_snap_orders();
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::list_snap_orders() {
+  if (!m_try_to_sort) {
+    list_snaps();
+    return;
+  }
+
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 10) << dendl;
+
+  librados::ObjectReadOperation op;
+  cls_client::group_snap_list_order_start(&op, m_start_after_order, MAX_RETURN);
+  auto comp = util::create_rados_callback<
+      ListSnapshotsRequest<I>,
+      &ListSnapshotsRequest<I>::handle_list_snap_orders>(this);
+  m_out_bl.clear();
+  int r = m_group_io_ctx.aio_operate(util::group_header_name(m_group_id), comp,
+                                     &op, &m_out_bl);
+  ceph_assert(r == 0);
+  comp->release();
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::handle_list_snap_orders(int r) {
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 10) << "r=" << r << dendl;
+
+  std::map<std::string, uint64_t> snap_orders;
+  if (r == 0) {
+    auto iter = m_out_bl.cbegin();
+    r = cls_client::group_snap_list_order_finish(&iter, &snap_orders);
+  }
+
+  if (r < 0) {
+    if (r == -EOPNOTSUPP && !m_fail_if_not_sorted) {
+      list_snaps();
+      return;
+    } else {
+      lderr(cct) << "failed to get group snapshot orders: " << cpp_strerror(r)
+                 << dendl;
+      finish(r);
+      return;
+    }
+  }
+
+  m_snap_orders.insert(snap_orders.begin(), snap_orders.end());
+  if (snap_orders.size() < MAX_RETURN) {
+    list_snaps();
+    return;
+  }
+
+  m_start_after_order = snap_orders.rbegin()->first;
+  list_snap_orders();
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::list_snaps() {
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 10) << dendl;
+
+  librados::ObjectReadOperation op;
+  cls_client::group_snap_list_start(&op, m_start_after, MAX_RETURN);
+  auto comp = util::create_rados_callback<
+      ListSnapshotsRequest<I>,
+      &ListSnapshotsRequest<I>::handle_list_snaps>(this);
+  m_out_bl.clear();
+  int r = m_group_io_ctx.aio_operate(util::group_header_name(m_group_id), comp,
+                                     &op, &m_out_bl);
+  ceph_assert(r == 0);
+  comp->release();
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::handle_list_snaps(int r) {
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 10) << "r=" << r << dendl;
+
+  std::vector<cls::rbd::GroupSnapshot> snaps;
+  if (r == 0) {
+    auto iter = m_out_bl.cbegin();
+    r = cls_client::group_snap_list_finish(&iter, &snaps);
+  }
+
+  if (r < 0) {
+    lderr(cct) << "failed to list group snapshots: " << cpp_strerror(r)
+               << dendl;
+    finish(r);
+    return;
+  }
+
+  m_snaps->insert(m_snaps->end(), snaps.begin(), snaps.end());
+  if (snaps.size() < MAX_RETURN) {
+    sort_snaps();
+    return;
+  }
+
+  m_start_after = *snaps.rbegin();
+  list_snaps();
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::sort_snaps() {
+  if (!m_try_to_sort) {
+    finish(0);
+    return;
+  }
+
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 10) << dendl;
+
+  for (const auto& snap : *m_snaps) {
+    if (m_snap_orders.find(snap.id) == m_snap_orders.end()) {
+      ldout(cct, 10) << "Missing order for snap_id=" << snap.id << dendl;
+      finish(m_fail_if_not_sorted ? -EINVAL : 0);
+      return;
+    }
+  }
+
+  std::sort(m_snaps->begin(), m_snaps->end(),
+            [this](const cls::rbd::GroupSnapshot &a,
+                   const cls::rbd::GroupSnapshot &b) {
+	       return this->m_snap_orders[a.id] < this->m_snap_orders[b.id];
+	    });
+
+  finish(0);
+}
+
+template <typename I>
+void ListSnapshotsRequest<I>::finish(int r) {
+  auto cct = reinterpret_cast<CephContext*>(m_group_io_ctx.cct());
+  ldout(cct, 10) << "r=" << r << dendl;
+
+  m_on_finish->complete(r);
+  delete this;
+}
+
+} // namespace group
+} // namespace librbd
+
+template class librbd::group::ListSnapshotsRequest<librbd::ImageCtx>;
diff --git a/src/librbd/group/ListSnapshotsRequest.h b/src/librbd/group/ListSnapshotsRequest.h
new file mode 100644
index 00000000000..7152dd981ab
--- /dev/null
+++ b/src/librbd/group/ListSnapshotsRequest.h
@@ -0,0 +1,93 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#ifndef CEPH_LIBRBD_GROUP_LIST_SNAPSHOTS_REQUEST_H
+#define CEPH_LIBRBD_GROUP_LIST_SNAPSHOTS_REQUEST_H
+
+#include "include/int_types.h"
+#include "include/types.h"
+#include "include/rados/librados.hpp"
+#include "cls/rbd/cls_rbd_types.h"
+
+#include <string>
+#include <vector>
+
+class Context;
+
+namespace librbd {
+
+struct ImageCtx;
+
+namespace group {
+
+template <typename ImageCtxT = librbd::ImageCtx>
+class ListSnapshotsRequest {
+public:
+  static ListSnapshotsRequest *create(
+      librados::IoCtx &group_io_ctx, const std::string &group_id,
+      bool try_to_sort, bool fail_if_not_sorted,
+      std::vector<cls::rbd::GroupSnapshot> *snaps, Context *on_finish) {
+    return new ListSnapshotsRequest(group_io_ctx, group_id, try_to_sort,
+                                    fail_if_not_sorted, snaps, on_finish);
+  }
+
+  ListSnapshotsRequest(librados::IoCtx &group_io_ctx,
+                       const std::string &group_id,
+                       bool try_to_sort, bool fail_if_not_sorted,
+                       std::vector<cls::rbd::GroupSnapshot> *snaps,
+                       Context *on_finish);
+
+  void send();
+
+private:
+  /**
+   * @verbatim
+   *
+   * <start>    /--------\
+   *    |       |        | (if required. repeat if more
+   *    v       v        |  entries)
+   *  LIST_SNAP_ORDERS --/
+   *    |       /--------\
+   *    |       |        | (repeat if more
+   *    v       v        |  snapshots)
+   *  LIST_SNAPS --------/
+   *    |
+   *    v
+   *  SORT_SNAPS (if required)  
+   *    |
+   *    v
+   *  <finish>
+   *
+   * @endverbatim
+   */
+
+  librados::IoCtx &m_group_io_ctx;
+  std::string m_group_id;
+  bool m_try_to_sort;
+  //Fail if m_try_to_sort is true and sorting fails. Ignored if m_try_to_sort is false.
+  bool m_fail_if_not_sorted;
+  std::vector<cls::rbd::GroupSnapshot> *m_snaps;
+  std::map<std::string, uint64_t> m_snap_orders;
+  Context *m_on_finish;
+
+  cls::rbd::GroupSnapshot m_start_after;
+  std::string m_start_after_order;
+  bufferlist m_out_bl;
+
+  void list_snaps();
+  void handle_list_snaps(int r);
+
+  void list_snap_orders();
+  void handle_list_snap_orders(int r);
+
+  void sort_snaps();
+
+  void finish(int r);
+};
+
+} // namespace group
+} // namespace librbd
+
+extern template class librbd::group::ListSnapshotsRequest<librbd::ImageCtx>;
+
+#endif // CEPH_LIBRBD_GROUP_LIST_SNAPSHOTS_REQUEST_H
diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index ed8ec9e9130..c389282c0cc 100644
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -1456,7 +1456,8 @@ namespace librbd {
     }
 
     std::vector<group_snap_info2_t> snaps2;
-    int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, &snaps2);
+    int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, true,
+                                            false, &snaps2);
 
     for (const auto& snap : snaps2) {
       snaps->push_back(
@@ -1473,7 +1474,8 @@ namespace librbd {
   int RBD::group_snap_list2(IoCtx& group_ioctx, const char *group_name,
                             std::vector<group_snap_info2_t> *snaps)
   {
-    return librbd::api::Group<>::snap_list(group_ioctx, group_name, snaps);
+    return librbd::api::Group<>::snap_list(group_ioctx, group_name, true,
+                                           false, snaps);
   }
 
   int RBD::group_snap_get_info(IoCtx& group_ioctx, const char *group_name,
@@ -7322,7 +7324,8 @@ extern "C" int rbd_group_snap_list(rados_ioctx_t group_p,
   }
 
   std::vector<librbd::group_snap_info2_t> cpp_snaps;
-  int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, &cpp_snaps);
+  int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, true, false,
+                                          &cpp_snaps);
 
   if (r == -ENOENT) {
     *snaps_size = 0;
@@ -7372,7 +7375,8 @@ extern "C" int rbd_group_snap_list2(rados_ioctx_t group_p,
   librados::IoCtx::from_rados_ioctx_t(group_p, group_ioctx);
 
   std::vector<librbd::group_snap_info2_t> cpp_snaps;
-  int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, &cpp_snaps);
+  int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, true, false,
+                                          &cpp_snaps);
   if (r < 0) {
     return r;
   }
diff --git a/src/test/librados_test_stub/LibradosTestStub.cc b/src/test/librados_test_stub/LibradosTestStub.cc
index 238cffa1999..507bd6d1b26 100644
--- a/src/test/librados_test_stub/LibradosTestStub.cc
+++ b/src/test/librados_test_stub/LibradosTestStub.cc
@@ -603,6 +603,13 @@ int IoCtx::omap_get_vals(const std::string& oid,
                      max_return, out_vals));
 }
 
+int IoCtx::omap_rm_keys(const std::string& oid,
+                        const std::set<std::string>& keys) {
+  TestIoCtxImpl *ctx = reinterpret_cast<TestIoCtxImpl*>(io_ctx_impl);
+  return ctx->execute_operation(
+    oid, std::bind(&TestIoCtxImpl::omap_rm_keys, _1, _2, keys));
+}
+
 int IoCtx::operate(const std::string& oid, ObjectWriteOperation *op) {
   TestIoCtxImpl *ctx = reinterpret_cast<TestIoCtxImpl*>(io_ctx_impl);
   TestObjectOperationImpl *ops = reinterpret_cast<TestObjectOperationImpl*>(op->impl);
diff --git a/src/test/librbd/test_Groups.cc b/src/test/librbd/test_Groups.cc
index eaef20611ac..eb18473b988 100644
--- a/src/test/librbd/test_Groups.cc
+++ b/src/test/librbd/test_Groups.cc
@@ -5,12 +5,15 @@
 #include "test/librbd/test_support.h"
 #include "include/rbd/librbd.h"
 #include "include/rbd/librbd.hpp"
+#include "librbd/api/Group.h"
 #include "test/librados/test.h"
 #include "gtest/gtest.h"
 
 #include <boost/scope_exit.hpp>
 #include <chrono>
 #include <vector>
+#include <set>
+#include <algorithm>
 
 void register_test_groups() {
 }
@@ -772,3 +775,102 @@ TEST_F(TestGroup, snap_list2PP)
   ASSERT_EQ(0, m_rbd.group_remove(m_ioctx, gp_name));
   ASSERT_EQ(0, _rados.pool_delete(pool_name2.c_str()));
 }
+
+TEST_F(TestGroup, snap_list_internal)
+{
+  REQUIRE_FORMAT_V2();
+
+  // Check that the listing works with different
+  // values for try_to_sort and fail_if_not_sorted
+
+  librados::IoCtx ioctx;
+  ASSERT_EQ(0, _rados.ioctx_create(_pool_name.c_str(), ioctx));
+
+  const char *group_name = "gp_snaplist_internalPP";
+
+  librbd::RBD rbd;
+  ASSERT_EQ(0, rbd.group_create(ioctx, group_name));
+
+  std::vector<librbd::group_snap_info2_t> gp_snaps;
+
+  // No snaps present
+  ASSERT_EQ(0, librbd::api::Group<>::snap_list(ioctx, group_name, true, true,
+                                               &gp_snaps));
+  ASSERT_EQ(0U, gp_snaps.size());
+
+  ASSERT_EQ(0, librbd::api::Group<>::snap_list(ioctx, group_name, false, false,
+                                               &gp_snaps));
+  ASSERT_EQ(0U, gp_snaps.size());
+
+  // Create a stale snap_order key by deleting the snapshot_ key
+  ASSERT_EQ(0, librbd::api::Group<>::snap_create(ioctx, group_name,
+                                                 "test-snap", 0));
+  ASSERT_EQ(0, librbd::api::Group<>::snap_list(ioctx, group_name, false, false,
+                                               &gp_snaps));
+  ASSERT_EQ(1U, gp_snaps.size());
+
+  std::string group_id;
+  ASSERT_EQ(0, librbd::api::Group<>::get_id(ioctx, group_name, &group_id));
+
+  std::string group_header = RBD_GROUP_HEADER_PREFIX + group_id;
+  std::set<std::string> keys = {"snapshot_" + gp_snaps[0].id};
+  ASSERT_EQ(0, ioctx.omap_rm_keys(group_header, keys));
+
+  for (int i = 0; i < 20; i++) {
+    std::string name = "snap" + stringify(i);
+    ASSERT_EQ(0, librbd::api::Group<>::snap_create(ioctx, group_name,
+                                                   name.c_str(), 0));
+  }
+
+  ASSERT_EQ(0, librbd::api::Group<>::snap_list(ioctx, group_name, true, true,
+                                               &gp_snaps));
+  ASSERT_EQ(20U, gp_snaps.size());
+
+  // Verify that the sorted list is correct
+  for (size_t i = 0; i < gp_snaps.size(); i++){
+    std::string name = "snap" + stringify(i);
+    ASSERT_EQ(name, gp_snaps[i].name);
+  }
+
+  // Sort on group snap ids to simulate the unsorted list.
+  std::vector<librbd::group_snap_info2_t> snaps_sorted_by_id = gp_snaps;
+  std::sort(snaps_sorted_by_id.begin(), snaps_sorted_by_id.end(),
+            [](const librbd::group_snap_info2_t &a,
+	       const librbd::group_snap_info2_t &b) {
+	      return a.id < b.id;
+	    });
+
+  // Check that the vectors actually differ
+  bool differ = false;
+  for (size_t i = 0; i < gp_snaps.size(); i++) {
+    if (gp_snaps[i].id != snaps_sorted_by_id[i].id) {
+      differ = true;
+      break;
+    }
+  }
+  ASSERT_TRUE(differ);
+
+  // Remove the snap_order key for one of the snaps.
+  keys = {"snap_order_" + gp_snaps[1].id};
+  ASSERT_EQ(0, ioctx.omap_rm_keys(group_header, keys));
+
+  //This should fail.
+  ASSERT_EQ(-EINVAL, librbd::api::Group<>::snap_list(ioctx, group_name, true,
+                                                     true, &gp_snaps));
+
+  // Should work if fail_if_not_sorted is false
+  ASSERT_EQ(0, librbd::api::Group<>::snap_list(ioctx, group_name, true, false,
+                                               &gp_snaps));
+  ASSERT_EQ(20U, gp_snaps.size());
+
+  ASSERT_EQ(0, librbd::api::Group<>::snap_list(ioctx, group_name, false, false,
+                                               &gp_snaps));
+  ASSERT_EQ(20U, gp_snaps.size());
+
+  //Compare unsorted listing
+  for (size_t i = 0; i < gp_snaps.size(); i++){
+    ASSERT_EQ(snaps_sorted_by_id[i].id, gp_snaps[i].id);
+  }
+
+  ASSERT_EQ(0, rbd.group_remove(ioctx, group_name));
+}