librbd: simplified retrieving snapshots from image header

Added a new cls 'snapshot_get' method that retrieves all snapshot
specific details to avoid the need to add a new cls method to
retrieve each individual snapshot data field.

The cls 'get_snapshot_namespace' method has been dropped since Luminous
clients will treat that as a user-generated snapshot (and that's all
that is supported by Luminous clients). The cls 'get_snapshot_timestamp'
method has been deprecated and can be removed once <=Luminous clients
are no longer supported.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

src/cls/rbd/cls_rbd.cc

@@ -1562,6 +1562,14 @@ int get_data_pool(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
   return 0;
 }
 
+/**
+ * Input:
+ * @param snap_id which snapshot to query
+ *
+ * Output:
+ * @param name (string) of the snapshot
+ * @returns 0 on success, negative error code on failure
+ */
 int get_snapshot_name(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
 {
   uint64_t snap_id;
@@ -1590,6 +1598,16 @@ int get_snapshot_name(cls_method_context_t hctx, bufferlist *in, bufferlist *out
   return 0;
 }
 
+/**
+ * Input:
+ * @param snap_id which snapshot to query
+ *
+ * Output:
+ * @param timestamp (utime_t) of the snapshot
+ * @returns 0 on success, negative error code on failure
+ *
+ * NOTE: deprecated - remove this method after Luminous is unsupported
+ */
 int get_snapshot_timestamp(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
 {
   uint64_t snap_id;
@@ -1620,16 +1638,14 @@ int get_snapshot_timestamp(cls_method_context_t hctx, bufferlist *in, bufferlist
 }
 
 /**
- * Retrieve namespace of a snapshot.
- *
  * Input:
- * @param snap_id id of the snapshot (uint64_t)
+ * @param snap_id which snapshot to query
  *
  * Output:
- * @param SnapshotNamespace
+ * @param snapshot (cls::rbd::SnapshotInfo)
- * @returns 0 on success, negative error code on failure.
+ * @returns 0 on success, negative error code on failure
  */
-int get_snapshot_namespace(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
+int snapshot_get(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
 {
   uint64_t snap_id;
 
@@ -1640,8 +1656,7 @@ int get_snapshot_namespace(cls_method_context_t hctx, bufferlist *in, bufferlist
     return -EINVAL;
   }
 
-  CLS_LOG(20, "get_snapshot_namespace snap_id=%" PRIu64, snap_id);
+  CLS_LOG(20, "snapshot_get snap_id=%llu", (unsigned long long)snap_id);
-
   if (snap_id == CEPH_NOSNAP) {
     return -EINVAL;
   }
@@ -1654,8 +1669,10 @@ int get_snapshot_namespace(cls_method_context_t hctx, bufferlist *in, bufferlist
     return r;
   }
 
-  encode(snap.snapshot_namespace, *out);
+  cls::rbd::SnapshotInfo snapshot_info{snap.id, snap.snapshot_namespace,
-
+                                       snap.name, snap.image_size,
+                                       snap.timestamp};
+  encode(snapshot_info, *out);
   return 0;
 }
 
@@ -1665,7 +1682,7 @@ int get_snapshot_namespace(cls_method_context_t hctx, bufferlist *in, bufferlist
  * Input:
  * @param snap_name name of the snapshot (string)
  * @param snap_id id of the snapshot (uint64_t)
- * @param snap_namespace namespace of the snapshot (cls::rbd::SnapshotNamespaceOnDisk)
+ * @param snap_namespace namespace of the snapshot (cls::rbd::SnapshotNamespace)
  *
  * Output:
  * @returns 0 on success, negative error code on failure.
@@ -1690,7 +1707,7 @@ int snapshot_add(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
   }
 
   if (boost::get<cls::rbd::UnknownSnapshotNamespace>(
-        &snap_meta.snapshot_namespace.snapshot_namespace) != nullptr) {
+        &snap_meta.snapshot_namespace) != nullptr) {
     CLS_ERR("Unknown snapshot namespace provided");
     return -EINVAL;
   }
@@ -5678,8 +5695,8 @@ CLS_INIT(rbd)
   cls_method_handle_t h_get_object_prefix;
   cls_method_handle_t h_get_data_pool;
   cls_method_handle_t h_get_snapshot_name;
-  cls_method_handle_t h_get_snapshot_namespace;
   cls_method_handle_t h_get_snapshot_timestamp;
+  cls_method_handle_t h_snapshot_get;
   cls_method_handle_t h_snapshot_add;
   cls_method_handle_t h_snapshot_remove;
   cls_method_handle_t h_snapshot_rename;
@@ -5780,12 +5797,12 @@ CLS_INIT(rbd)
   cls_register_cxx_method(h_class, "get_snapshot_name",
 			  CLS_METHOD_RD,
 			  get_snapshot_name, &h_get_snapshot_name);
-  cls_register_cxx_method(h_class, "get_snapshot_namespace",
-			  CLS_METHOD_RD,
-			  get_snapshot_namespace, &h_get_snapshot_namespace);
   cls_register_cxx_method(h_class, "get_snapshot_timestamp",
 			  CLS_METHOD_RD,
 			  get_snapshot_timestamp, &h_get_snapshot_timestamp);
+  cls_register_cxx_method(h_class, "snapshot_get",
+                          CLS_METHOD_RD,
+                          snapshot_get, &h_snapshot_get);
   cls_register_cxx_method(h_class, "snapshot_add",
 			  CLS_METHOD_RD | CLS_METHOD_WR,
 			  snapshot_add, &h_snapshot_add);

src/cls/rbd/cls_rbd.h

@@ -66,7 +66,7 @@ struct cls_rbd_snap {
   cls_rbd_parent parent;
   uint64_t flags;
   utime_t timestamp;
-  cls::rbd::SnapshotNamespaceOnDisk snapshot_namespace = {
+  cls::rbd::SnapshotNamespace snapshot_namespace = {
     cls::rbd::UserSnapshotNamespace{}};
 
   /// true if we have a parent

src/cls/rbd/cls_rbd_client.cc

@@ -78,9 +78,7 @@ namespace librbd {
       bufferlist empty_bl;
       op->exec("rbd", "get_snapcontext", empty_bl);
 
-      bufferlist parent_bl;
+      get_parent_start(op, snap);
-      encode(snap, parent_bl);
-      op->exec("rbd", "get_parent", parent_bl);
 
       rados::cls::lock::get_lock_info_start(op, RBD_LOCK_NAME);
     }
@@ -111,18 +109,15 @@ namespace librbd {
 	// get_snapcontext
 	decode(*snapc, *it);
 	// get_parent
-	decode(parent->spec.pool_id, *it);
+	int r = get_parent_finish(it, &parent->spec, &parent->overlap);
-	decode(parent->spec.image_id, *it);
+        if (r < 0) {
-	decode(parent->spec.snap_id, *it);
+          return r;
-	decode(parent->overlap, *it);
+        }
 
 	// get_lock_info
 	ClsLockType lock_type = LOCK_NONE;
-	int r = rados::cls::lock::get_lock_info_finish(it, lockers, &lock_type,
+	r = rados::cls::lock::get_lock_info_finish(it, lockers, &lock_type,
 						   lock_tag);
-        if (r == -EOPNOTSUPP) {
-          r = 0;
-        }
         if (r == 0) {
 	  *exclusive_lock = (lock_type == LOCK_EXCLUSIVE);
         }
@@ -304,28 +299,42 @@ namespace librbd {
       op->exec("rbd", "set_size", bl);
     }
 
+    void get_parent_start(librados::ObjectReadOperation *op, snapid_t snap_id)
+    {
+      bufferlist bl;
+      encode(snap_id, bl);
+      op->exec("rbd", "get_parent", bl);
+    }
+
+    int get_parent_finish(bufferlist::iterator *it, ParentSpec *pspec,
+                          uint64_t *parent_overlap)
+    {
+      try {
+	decode(pspec->pool_id, *it);
+	decode(pspec->image_id, *it);
+	decode(pspec->snap_id, *it);
+	decode(*parent_overlap, *it);
+      } catch (const buffer::error &) {
+	return -EBADMSG;
+      }
+      return 0;
+    }
+
     int get_parent(librados::IoCtx *ioctx, const std::string &oid,
 		   snapid_t snap_id, ParentSpec *pspec,
 		   uint64_t *parent_overlap)
     {
-      bufferlist inbl, outbl;
+      librados::ObjectReadOperation op;
-      encode(snap_id, inbl);
+      get_parent_start(&op, snap_id);
 
-      int r = ioctx->exec(oid, "rbd", "get_parent", inbl, outbl);
+      bufferlist out_bl;
-      if (r < 0)
+      int r = ioctx->operate(oid, &op, &out_bl);
+      if (r < 0) {
         return r;
-
-      try {
-	bufferlist::iterator iter = outbl.begin();
-	decode(pspec->pool_id, iter);
-	decode(pspec->image_id, iter);
-	decode(pspec->snap_id, iter);
-	decode(*parent_overlap, iter);
-      } catch (const buffer::error &err) {
-	return -EBADMSG;
       }
 
-      return 0;
+      bufferlist::iterator it = out_bl.begin();
+      return get_parent_finish(&it, pspec, parent_overlap);
     }
 
     int set_parent(librados::IoCtx *ioctx, const std::string &oid,
@@ -540,13 +549,78 @@ namespace librbd {
       return get_children_finish(&it, &children);
     }
 
+    void snapshot_get_start(librados::ObjectReadOperation *op,
+                            const std::vector<snapid_t> &ids)
+    {
+      for (auto snap_id : ids) {
+        bufferlist bl;
+        encode(snap_id, bl);
+        op->exec("rbd", "snapshot_get", bl);
+
+        get_parent_start(op, snap_id);
+        get_protection_status_start(op, snap_id);
+      }
+    }
+
+    int snapshot_get_finish(bufferlist::iterator *it,
+                            const std::vector<snapid_t> &ids,
+                            std::vector<cls::rbd::SnapshotInfo>* snaps,
+                            std::vector<ParentInfo> *parents,
+                            std::vector<uint8_t> *protection_statuses)
+    {
+      snaps->resize(ids.size());
+      parents->resize(ids.size());
+      protection_statuses->resize(ids.size());
+      try {
+	for (size_t i = 0; i < snaps->size(); ++i) {
+          decode((*snaps)[i], *it);
+
+	  // get_parent
+	  int r = get_parent_finish(it, &(*parents)[i].spec,
+                                    &(*parents)[i].overlap);
+          if (r < 0) {
+            return r;
+          }
+
+	  // get_protection_status
+	  r = get_protection_status_finish(it, &(*protection_statuses)[i]);
+          if (r < 0) {
+            return r;
+          }
+	}
+      } catch (const buffer::error &err) {
+        return -EBADMSG;
+      }
+      return 0;
+    }
+
+    int snapshot_get(librados::IoCtx* ioctx, const std::string& oid,
+                     const std::vector<snapid_t>& ids,
+                     std::vector<cls::rbd::SnapshotInfo>* snaps,
+                     std::vector<ParentInfo> *parents,
+                     std::vector<uint8_t> *protection_statuses)
+    {
+      librados::ObjectReadOperation op;
+      snapshot_get_start(&op, ids);
+
+      bufferlist out_bl;
+      int r = ioctx->operate(oid, &op, &out_bl);
+      if (r < 0) {
+        return r;
+      }
+
+      bufferlist::iterator it = out_bl.begin();
+      return snapshot_get_finish(&it, ids, snaps, parents, protection_statuses);
+    }
+
     void snapshot_add(librados::ObjectWriteOperation *op, snapid_t snap_id,
-		      const std::string &snap_name, const cls::rbd::SnapshotNamespace &snap_namespace)
+		      const std::string &snap_name,
+                      const cls::rbd::SnapshotNamespace &snap_namespace)
     {
       bufferlist bl;
       encode(snap_name, bl);
       encode(snap_id, bl);
-      encode(cls::rbd::SnapshotNamespaceOnDisk(snap_namespace), bl);
+      encode(snap_namespace, bl);
       op->exec("rbd", "snapshot_add", bl);
     }
 
@@ -606,15 +680,13 @@ namespace librbd {
     void snapshot_list_start(librados::ObjectReadOperation *op,
                              const std::vector<snapid_t> &ids) {
       for (auto snap_id : ids) {
-        bufferlist bl1, bl2, bl3, bl4;
+        bufferlist bl1, bl2;
         encode(snap_id, bl1);
         op->exec("rbd", "get_snapshot_name", bl1);
         encode(snap_id, bl2);
         op->exec("rbd", "get_size", bl2);
-        encode(snap_id, bl3);
+        get_parent_start(op, snap_id);
-        op->exec("rbd", "get_parent", bl3);
+        get_protection_status_start(op, snap_id);
-        encode(snap_id, bl4);
-        op->exec("rbd", "get_protection_status", bl4);
       }
     }
 
@@ -637,13 +709,19 @@ namespace librbd {
 	  // get_size
 	  decode(order, *it);
 	  decode((*sizes)[i], *it);
+
 	  // get_parent
-	  decode((*parents)[i].spec.pool_id, *it);
+	  int r = get_parent_finish(it, &(*parents)[i].spec,
-	  decode((*parents)[i].spec.image_id, *it);
+                                    &(*parents)[i].overlap);
-	  decode((*parents)[i].spec.snap_id, *it);
+          if (r < 0) {
-	  decode((*parents)[i].overlap, *it);
+            return r;
+          }
+
 	  // get_protection_status
-	  decode((*protection_statuses)[i], *it);
+	  r = get_protection_status_finish(it, &(*protection_statuses)[i]);
+          if (r < 0) {
+            return r;
+          }
 	}
       } catch (const buffer::error &err) {
         return -EBADMSG;
@@ -716,50 +794,6 @@ namespace librbd {
       return snapshot_timestamp_list_finish(&it, ids, timestamps);
     }
 
-    void snapshot_namespace_list_start(librados::ObjectReadOperation *op,
-                                       const std::vector<snapid_t> &ids)
-    {
-      for (auto snap_id : ids) {
-        bufferlist bl;
-        encode(snap_id, bl);
-        op->exec("rbd", "get_snapshot_namespace", bl);
-      }
-    }
-
-    int snapshot_namespace_list_finish(bufferlist::iterator *it,
-                                       const std::vector<snapid_t> &ids,
-                                       std::vector<cls::rbd::SnapshotNamespace> *namespaces)
-    {
-      namespaces->resize(ids.size());
-      try {
-	for (size_t i = 0; i < namespaces->size(); ++i) {
-	  cls::rbd::SnapshotNamespaceOnDisk e;
-	  decode(e, *it);
-	  (*namespaces)[i] = e.snapshot_namespace;
-	}
-      } catch (const buffer::error &err) {
-        return -EBADMSG;
-      }
-      return 0;
-    }
-
-    int snapshot_namespace_list(librados::IoCtx *ioctx, const std::string &oid,
-                                const std::vector<snapid_t> &ids,
-                                std::vector<cls::rbd::SnapshotNamespace> *namespaces)
-    {
-      librados::ObjectReadOperation op;
-      snapshot_namespace_list_start(&op, ids);
-
-      bufferlist out_bl;
-      int r = ioctx->operate(oid, &op, &out_bl);
-      if (r < 0) {
-        return r;
-      }
-
-      bufferlist::iterator it = out_bl.begin();
-      return snapshot_namespace_list_finish(&it, ids, namespaces);
-    }
-
     void old_snapshot_add(librados::ObjectWriteOperation *op,
 			  snapid_t snap_id, const std::string &snap_name)
     {
@@ -868,24 +902,39 @@ namespace librbd {
       return ioctx->exec(oid, "rbd", "copyup", data, out);
     }
 
+    void get_protection_status_start(librados::ObjectReadOperation *op,
+                                     snapid_t snap_id)
+    {
+      bufferlist bl;
+      encode(snap_id, bl);
+      op->exec("rbd", "get_protection_status", bl);
+    }
+
+    int get_protection_status_finish(bufferlist::iterator *it,
+                                     uint8_t *protection_status)
+    {
+      try {
+	decode(*protection_status, *it);
+      } catch (const buffer::error &) {
+	return -EBADMSG;
+      }
+      return 0;
+    }
+
     int get_protection_status(librados::IoCtx *ioctx, const std::string &oid,
 			      snapid_t snap_id, uint8_t *protection_status)
     {
-      bufferlist in, out;
+      librados::ObjectReadOperation op;
-      encode(snap_id.val, in);
+      get_protection_status_start(&op, snap_id);
 
-      int r = ioctx->exec(oid, "rbd", "get_protection_status", in, out);
+      bufferlist out_bl;
-      if (r < 0)
+      int r = ioctx->operate(oid, &op, &out_bl);
+      if (r < 0) {
         return r;
-
-      try {
-	bufferlist::iterator iter = out.begin();
-	decode(*protection_status, iter);
-      } catch (const buffer::error &err) {
-	return -EBADMSG;
       }
 
-      return 0;
+      bufferlist::iterator it = out_bl.begin();
+      return get_protection_status_finish(&it, protection_status);
     }
 
     int set_protection_status(librados::IoCtx *ioctx, const std::string &oid,

src/cls/rbd/cls_rbd_client.h

@@ -72,9 +72,14 @@ namespace librbd {
     int set_size(librados::IoCtx *ioctx, const std::string &oid,
 		 uint64_t size);
     void set_size(librados::ObjectWriteOperation *op, uint64_t size);
+
+    void get_parent_start(librados::ObjectReadOperation *op, snapid_t snap_id);
+    int get_parent_finish(bufferlist::iterator *it, ParentSpec *pspec,
+                          uint64_t *parent_overlap);
     int get_parent(librados::IoCtx *ioctx, const std::string &oid,
 		   snapid_t snap_id, ParentSpec *pspec,
 		   uint64_t *parent_overlap);
+
     int set_parent(librados::IoCtx *ioctx, const std::string &oid,
 		   const ParentSpec &pspec, uint64_t parent_overlap);
     void set_parent(librados::ObjectWriteOperation *op,
@@ -113,6 +118,20 @@ namespace librbd {
                             std::set<string> *children);
     int get_children(librados::IoCtx *ioctx, const std::string &oid,
                       const ParentSpec &pspec, set<string>& children);
+
+    void snapshot_get_start(librados::ObjectReadOperation *op,
+                            const std::vector<snapid_t> &ids);
+    int snapshot_get_finish(bufferlist::iterator *it,
+                            const std::vector<snapid_t> &ids,
+                            std::vector<cls::rbd::SnapshotInfo>* snaps,
+                            std::vector<ParentInfo> *parents,
+                            std::vector<uint8_t> *protection_statuses);
+    int snapshot_get(librados::IoCtx* ioctx, const std::string& oid,
+                     const std::vector<snapid_t>& ids,
+                     std::vector<cls::rbd::SnapshotInfo>* snaps,
+                     std::vector<ParentInfo> *parents,
+                     std::vector<uint8_t> *protection_statuses);
+
     void snapshot_add(librados::ObjectWriteOperation *op, snapid_t snap_id,
 		      const std::string &snap_name,
 		      const cls::rbd::SnapshotNamespace &snap_namespace);
@@ -126,6 +145,7 @@ namespace librbd {
     int get_snapcontext(librados::IoCtx *ioctx, const std::string &oid,
 			::SnapContext *snapc);
 
+    /// NOTE: remove after Luminous is retired
     void snapshot_list_start(librados::ObjectReadOperation *op,
                              const std::vector<snapid_t> &ids);
     int snapshot_list_finish(bufferlist::iterator *it,
@@ -134,17 +154,6 @@ namespace librbd {
                              std::vector<uint64_t> *sizes,
                              std::vector<ParentInfo> *parents,
                              std::vector<uint8_t> *protection_statuses);
-    void snapshot_timestamp_list_start(librados::ObjectReadOperation *op,
-                                       const std::vector<snapid_t> &ids);
-
-    int snapshot_timestamp_list_finish(bufferlist::iterator *it,
-                                       const std::vector<snapid_t> &ids,
-                                       std::vector<utime_t> *timestamps);
-
-    int snapshot_timestamp_list(librados::IoCtx *ioctx, const std::string &oid,
-                                const std::vector<snapid_t> &ids,
-                                std::vector<utime_t> *timestamps);
-
     int snapshot_list(librados::IoCtx *ioctx, const std::string &oid,
 		      const std::vector<snapid_t> &ids,
 		      std::vector<string> *names,
@@ -152,14 +161,15 @@ namespace librbd {
 		      std::vector<ParentInfo> *parents,
 		      std::vector<uint8_t> *protection_statuses);
 
-    void snapshot_namespace_list_start(librados::ObjectReadOperation *op,
+    /// NOTE: remove after Luminous is retired
+    void snapshot_timestamp_list_start(librados::ObjectReadOperation *op,
                                        const std::vector<snapid_t> &ids);
-    int snapshot_namespace_list_finish(bufferlist::iterator *it,
+    int snapshot_timestamp_list_finish(bufferlist::iterator *it,
                                        const std::vector<snapid_t> &ids,
-                                       std::vector<cls::rbd::SnapshotNamespace> *namespaces);
+                                       std::vector<utime_t> *timestamps);
-    int snapshot_namespace_list(librados::IoCtx *ioctx, const std::string &oid,
+    int snapshot_timestamp_list(librados::IoCtx *ioctx, const std::string &oid,
                                 const std::vector<snapid_t> &ids,
-                                std::vector<cls::rbd::SnapshotNamespace> *namespaces);
+                                std::vector<utime_t> *timestamps);
 
     void get_all_features_start(librados::ObjectReadOperation *op);
     int get_all_features_finish(bufferlist::iterator *it,
@@ -169,8 +179,15 @@ namespace librbd {
 
     int copyup(librados::IoCtx *ioctx, const std::string &oid,
 	       bufferlist data);
+
+    /// NOTE: remove protection after clone v1 is retired
+    void get_protection_status_start(librados::ObjectReadOperation *op,
+                                     snapid_t snap_id);
+    int get_protection_status_finish(bufferlist::iterator *it,
+                                     uint8_t *protection_status);
     int get_protection_status(librados::IoCtx *ioctx, const std::string &oid,
 			      snapid_t snap_id, uint8_t *protection_status);
+
     int set_protection_status(librados::IoCtx *ioctx, const std::string &oid,
 			      snapid_t snap_id, uint8_t protection_status);
     void set_protection_status(librados::ObjectWriteOperation *op,

src/cls/rbd/cls_rbd_types.cc

@@ -415,40 +415,79 @@ SnapshotNamespaceType get_snap_namespace_type(
     GetTypeVisitor(), snapshot_namespace));
 }
 
-void SnapshotNamespaceOnDisk::encode(bufferlist& bl) const {
+void SnapshotInfo::encode(bufferlist& bl) const {
   ENCODE_START(1, 1, bl);
-  boost::apply_visitor(EncodeSnapshotNamespaceVisitor(bl), snapshot_namespace);
+  encode(id, bl);
+  encode(snapshot_namespace, bl);
+  encode(name, bl);
+  encode(image_size, bl);
+  encode(timestamp, bl);
   ENCODE_FINISH(bl);
 }
 
-void SnapshotNamespaceOnDisk::decode(bufferlist::iterator &p)
+void SnapshotInfo::decode(bufferlist::iterator& it) {
+  DECODE_START(1, it);
+  decode(id, it);
+  decode(snapshot_namespace, it);
+  decode(name, it);
+  decode(image_size, it);
+  decode(timestamp, it);
+  DECODE_FINISH(it);
+}
+
+void SnapshotInfo::dump(Formatter *f) const {
+  f->dump_unsigned("id", id);
+  f->open_object_section("namespace");
+  boost::apply_visitor(DumpSnapshotNamespaceVisitor(f, "type"),
+                       snapshot_namespace);
+  f->close_section();
+  f->dump_string("name", name);
+  f->dump_unsigned("image_size", image_size);
+  f->dump_stream("timestamp") << timestamp;
+}
+
+void SnapshotInfo::generate_test_instances(std::list<SnapshotInfo*> &o) {
+  o.push_back(new SnapshotInfo(1ULL, UserSnapshotNamespace{}, "snap1", 123,
+                               {123456, 0}));
+  o.push_back(new SnapshotInfo(2ULL,
+                               GroupSnapshotNamespace{567, "group1", "snap1"},
+                               "snap1", 123, {123456, 0}));
+}
+
+void SnapshotNamespace::encode(bufferlist& bl) const {
+  ENCODE_START(1, 1, bl);
+  boost::apply_visitor(EncodeSnapshotNamespaceVisitor(bl), *this);
+  ENCODE_FINISH(bl);
+}
+
+void SnapshotNamespace::decode(bufferlist::iterator &p)
 {
   DECODE_START(1, p);
   uint32_t snap_type;
   decode(snap_type, p);
   switch (snap_type) {
     case cls::rbd::SNAPSHOT_NAMESPACE_TYPE_USER:
-      snapshot_namespace = UserSnapshotNamespace();
+      *this = UserSnapshotNamespace();
       break;
     case cls::rbd::SNAPSHOT_NAMESPACE_TYPE_GROUP:
-      snapshot_namespace = GroupSnapshotNamespace();
+      *this = GroupSnapshotNamespace();
       break;
     default:
-      snapshot_namespace = UnknownSnapshotNamespace();
+      *this = UnknownSnapshotNamespace();
       break;
   }
-  boost::apply_visitor(DecodeSnapshotNamespaceVisitor(p), snapshot_namespace);
+  boost::apply_visitor(DecodeSnapshotNamespaceVisitor(p), *this);
   DECODE_FINISH(p);
 }
 
-void SnapshotNamespaceOnDisk::dump(Formatter *f) const {
+void SnapshotNamespace::dump(Formatter *f) const {
-  boost::apply_visitor(DumpSnapshotNamespaceVisitor(f, "snapshot_namespace_type"), snapshot_namespace);
+  boost::apply_visitor(DumpSnapshotNamespaceVisitor(f, "snapshot_namespace_type"), *this);
 }
 
-void SnapshotNamespaceOnDisk::generate_test_instances(std::list<SnapshotNamespaceOnDisk *> &o) {
+void SnapshotNamespace::generate_test_instances(std::list<SnapshotNamespace*> &o) {
-  o.push_back(new SnapshotNamespaceOnDisk(UserSnapshotNamespace()));
+  o.push_back(new SnapshotNamespace(UserSnapshotNamespace()));
-  o.push_back(new SnapshotNamespaceOnDisk(GroupSnapshotNamespace(0, "10152ae8944a", "2118643c9732")));
+  o.push_back(new SnapshotNamespace(GroupSnapshotNamespace(0, "10152ae8944a", "2118643c9732")));
-  o.push_back(new SnapshotNamespaceOnDisk(GroupSnapshotNamespace(5, "1018643c9869", "33352be8933c")));
+  o.push_back(new SnapshotNamespace(GroupSnapshotNamespace(5, "1018643c9869", "33352be8933c")));
 }
 
 std::ostream& operator<<(std::ostream& os, const UserSnapshotNamespace& ns) {

src/cls/rbd/cls_rbd_types.h

@@ -320,29 +320,59 @@ std::ostream& operator<<(std::ostream& os, const UnknownSnapshotNamespace& ns);
 
 typedef boost::variant<UserSnapshotNamespace,
                        GroupSnapshotNamespace,
-		       UnknownSnapshotNamespace> SnapshotNamespace;
+                       UnknownSnapshotNamespace> SnapshotNamespaceVariant;
 
+struct SnapshotNamespace : public SnapshotNamespaceVariant {
 
-struct SnapshotNamespaceOnDisk {
+  SnapshotNamespace() {
+  }
 
-  SnapshotNamespaceOnDisk() : snapshot_namespace(UnknownSnapshotNamespace()) {}
+  template <typename T>
-  SnapshotNamespaceOnDisk(const SnapshotNamespace &sn) : snapshot_namespace(sn) {}
+  SnapshotNamespace(T&& t) : SnapshotNamespaceVariant(std::forward<T>(t)) {
-
+  }
-  SnapshotNamespace snapshot_namespace;
 
   void encode(bufferlist& bl) const;
   void decode(bufferlist::iterator& it);
   void dump(Formatter *f) const;
 
-  static void generate_test_instances(std::list<SnapshotNamespaceOnDisk *> &o);
+  static void generate_test_instances(std::list<SnapshotNamespace*> &o);
 
-  inline bool operator==(const SnapshotNamespaceOnDisk& gsn) const {
+  inline bool operator==(const SnapshotNamespaceVariant& sn) const {
-    return snapshot_namespace == gsn.snapshot_namespace;
+    return static_cast<const SnapshotNamespaceVariant&>(*this) == sn;
+  }
+  inline bool operator<(const SnapshotNamespaceVariant& sn) const {
+    return static_cast<const SnapshotNamespaceVariant&>(*this) < sn;
   }
 };
-WRITE_CLASS_ENCODER(SnapshotNamespaceOnDisk);
+WRITE_CLASS_ENCODER(SnapshotNamespace);
 
-SnapshotNamespaceType get_snap_namespace_type(const SnapshotNamespace& snapshot_namespace);
+SnapshotNamespaceType get_snap_namespace_type(
+    const SnapshotNamespace& snapshot_namespace);
+
+struct SnapshotInfo {
+  snapid_t id = CEPH_NOSNAP;
+  cls::rbd::SnapshotNamespace snapshot_namespace = {UserSnapshotNamespace{}};
+  std::string name;
+  uint64_t image_size = 0;
+  utime_t timestamp;
+
+  SnapshotInfo() {
+  }
+  SnapshotInfo(snapid_t id,
+               const cls::rbd::SnapshotNamespace& snapshot_namespace,
+               const std::string& name, uint64_t image_size,
+               const utime_t& timestamp)
+    : id(id), snapshot_namespace(snapshot_namespace),
+      name(name), image_size(image_size), timestamp(timestamp) {
+  }
+
+  void encode(bufferlist& bl) const;
+  void decode(bufferlist::iterator& it);
+  void dump(Formatter *f) const;
+
+  static void generate_test_instances(std::list<SnapshotInfo*> &o);
+};
+WRITE_CLASS_ENCODER(SnapshotInfo);
 
 enum GroupSnapshotState {
   GROUP_SNAPSHOT_STATE_INCOMPLETE = 0,

src/librbd/WatchNotifyTypes.cc

@@ -204,23 +204,20 @@ void ResizePayload::dump(Formatter *f) const {
 void SnapPayloadBase::encode(bufferlist &bl) const {
   using ceph::encode;
   encode(snap_name, bl);
-  encode(cls::rbd::SnapshotNamespaceOnDisk(snap_namespace), bl);
+  encode(snap_namespace, bl);
 }
 
 void SnapPayloadBase::decode(__u8 version, bufferlist::iterator &iter) {
   using ceph::decode;
   decode(snap_name, iter);
   if (version >= 6) {
-    cls::rbd::SnapshotNamespaceOnDisk sn;
+    decode(snap_namespace, iter);
-    decode(sn, iter);
-    snap_namespace = sn.snapshot_namespace;
   }
 }
 
 void SnapPayloadBase::dump(Formatter *f) const {
   f->dump_string("snap_name", snap_name);
-  cls::rbd::SnapshotNamespaceOnDisk sn(snap_namespace);
+  snap_namespace.dump(f);
-  sn.dump(f);
 }
 
 void SnapCreatePayload::encode(bufferlist &bl) const {
@@ -231,9 +228,7 @@ void SnapCreatePayload::decode(__u8 version, bufferlist::iterator &iter) {
   using ceph::decode;
   SnapPayloadBase::decode(version, iter);
   if (version == 5) {
-    cls::rbd::SnapshotNamespaceOnDisk sn;
+    decode(snap_namespace, iter);
-    decode(sn, iter);
-    snap_namespace = sn.snapshot_namespace;
   }
 }
 

src/librbd/image/RefreshRequest.cc

@@ -133,10 +133,12 @@ Context *RefreshRequest<I>::handle_v1_get_snapshots(int *result) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << ": " << "r=" << *result << dendl;
 
+  std::vector<std::string> snap_names;
+  std::vector<uint64_t> snap_sizes;
   if (*result == 0) {
     bufferlist::iterator it = m_out_bl.begin();
-    *result = cls_client::old_snapshot_list_finish(
+    *result = cls_client::old_snapshot_list_finish(&it, &snap_names,
-      &it, &m_snap_names, &m_snap_sizes, &m_snapc);
+                                                   &snap_sizes, &m_snapc);
   }
 
   if (*result < 0) {
@@ -151,12 +153,12 @@ Context *RefreshRequest<I>::handle_v1_get_snapshots(int *result) {
     return m_on_finish;
   }
 
-  //m_snap_namespaces = {m_snap_names.size(), cls::rbd::UserSnapshotNamespace()};
+  m_snap_infos.clear();
-  m_snap_namespaces = std::vector<cls::rbd::SnapshotNamespace>(
+  for (size_t i = 0; i < m_snapc.snaps.size(); ++i) {
-					    m_snap_names.size(),
+    m_snap_infos.push_back({m_snapc.snaps[i],
-					    cls::rbd::UserSnapshotNamespace());
+                            {cls::rbd::UserSnapshotNamespace{}},
-
+                            snap_names[i], snap_sizes[i], {}});
-  m_snap_timestamps = std::vector<utime_t>(m_snap_names.size(), utime_t());
+  }
 
   send_v1_get_locks();
   return nullptr;
@@ -378,6 +380,7 @@ Context *RefreshRequest<I>::handle_v2_get_flags(int *result) {
                  << "r=" << *result << dendl;
 
   if (*result == 0) {
+    /// NOTE: remove support for snap paramter after Luminous is retired
     bufferlist::iterator it = m_out_bl.begin();
     cls_client::get_flags_finish(&it, &m_flags, m_snapc.snaps, &m_snap_flags);
   }
@@ -495,12 +498,9 @@ Context *RefreshRequest<I>::handle_v2_get_group(int *result) {
 template <typename I>
 void RefreshRequest<I>::send_v2_get_snapshots() {
   if (m_snapc.snaps.empty()) {
-    m_snap_names.clear();
+    m_snap_infos.clear();
-    m_snap_namespaces.clear();
-    m_snap_sizes.clear();
     m_snap_parents.clear();
     m_snap_protection.clear();
-    m_snap_timestamps.clear();
     send_v2_refresh_parent();
     return;
   }
@@ -509,7 +509,7 @@ void RefreshRequest<I>::send_v2_get_snapshots() {
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
   librados::ObjectReadOperation op;
-  cls_client::snapshot_list_start(&op, m_snapc.snaps);
+  cls_client::snapshot_get_start(&op, m_snapc.snaps);
 
   using klass = RefreshRequest<I>;
   librados::AioCompletion *comp = create_rados_callback<
@@ -527,11 +527,62 @@ Context *RefreshRequest<I>::handle_v2_get_snapshots(int *result) {
   ldout(cct, 10) << this << " " << __func__ << ": "
                  << "r=" << *result << dendl;
 
+  if (*result == 0) {
+    bufferlist::iterator it = m_out_bl.begin();
+    *result = cls_client::snapshot_get_finish(&it, m_snapc.snaps, &m_snap_infos,
+                                              &m_snap_parents,
+                                              &m_snap_protection);
+  }
+  if (*result == -ENOENT) {
+    ldout(cct, 10) << "out-of-sync snapshot state detected" << dendl;
+    send_v2_get_mutable_metadata();
+    return nullptr;
+  } else if (*result == -EOPNOTSUPP) {
+    ldout(cct, 10) << "retrying using legacy snapshot methods" << dendl;
+    send_v2_get_snapshots_legacy();
+    return nullptr;
+  } else if (*result < 0) {
+    lderr(cct) << "failed to retrieve snapshots: " << cpp_strerror(*result)
+               << dendl;
+    return m_on_finish;
+  }
+
+  send_v2_refresh_parent();
+  return nullptr;
+}
+
+template <typename I>
+void RefreshRequest<I>::send_v2_get_snapshots_legacy() {
+  /// NOTE: remove after Luminous is retired
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << this << " " << __func__ << dendl;
+
+  librados::ObjectReadOperation op;
+  cls_client::snapshot_list_start(&op, m_snapc.snaps);
+
+  using klass = RefreshRequest<I>;
+  librados::AioCompletion *comp = create_rados_callback<
+    klass, &klass::handle_v2_get_snapshots_legacy>(this);
+  m_out_bl.clear();
+  int r = m_image_ctx.md_ctx.aio_operate(m_image_ctx.header_oid, comp, &op,
+                                         &m_out_bl);
+  assert(r == 0);
+  comp->release();
+}
+
+template <typename I>
+Context *RefreshRequest<I>::handle_v2_get_snapshots_legacy(int *result) {
+  /// NOTE: remove after Luminous is retired
+  CephContext *cct = m_image_ctx.cct;
+  ldout(cct, 10) << this << " " << __func__ << ": "
+                 << "r=" << *result << dendl;
+
+  std::vector<std::string> snap_names;
+  std::vector<uint64_t> snap_sizes;
   if (*result == 0) {
     bufferlist::iterator it = m_out_bl.begin();
     *result = cls_client::snapshot_list_finish(&it, m_snapc.snaps,
-                                               &m_snap_names,
+                                               &snap_names, &snap_sizes,
-					       &m_snap_sizes,
                                                &m_snap_parents,
                                                &m_snap_protection);
   }
@@ -545,12 +596,20 @@ Context *RefreshRequest<I>::handle_v2_get_snapshots(int *result) {
     return m_on_finish;
   }
 
+  m_snap_infos.clear();
+  for (size_t i = 0; i < m_snapc.snaps.size(); ++i) {
+    m_snap_infos.push_back({m_snapc.snaps[i],
+                            {cls::rbd::UserSnapshotNamespace{}},
+                            snap_names[i], snap_sizes[i], {}});
+  }
+
   send_v2_get_snap_timestamps();
   return nullptr;
 }
 
 template <typename I>
 void RefreshRequest<I>::send_v2_get_snap_timestamps() {
+  /// NOTE: remove after Luminous is retired
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << dendl;
 
@@ -569,79 +628,37 @@ void RefreshRequest<I>::send_v2_get_snap_timestamps() {
 
 template <typename I>
 Context *RefreshRequest<I>::handle_v2_get_snap_timestamps(int *result) {
+  /// NOTE: remove after Luminous is retired
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 10) << this << " " << __func__ << ": " << "r=" << *result << dendl;
 
+  std::vector<utime_t> snap_timestamps;
   if (*result == 0) {
     bufferlist::iterator it = m_out_bl.begin();
-    *result = cls_client::snapshot_timestamp_list_finish(&it, m_snapc.snaps, &m_snap_timestamps);
+    *result = cls_client::snapshot_timestamp_list_finish(&it, m_snapc.snaps,
+                                                         &snap_timestamps);
   }
   if (*result == -ENOENT) {
     ldout(cct, 10) << "out-of-sync snapshot state detected" << dendl;
     send_v2_get_mutable_metadata();
     return nullptr;
   } else if (*result == -EOPNOTSUPP) {
-    m_snap_timestamps = std::vector<utime_t>(m_snap_names.size(), utime_t());
     // Ignore it means no snap timestamps are available
   } else if (*result < 0) {
-    lderr(cct) << "failed to retrieve snapshots: " << cpp_strerror(*result)
+    lderr(cct) << "failed to retrieve snapshot timestamps: "
-               << dendl;
+               << cpp_strerror(*result) << dendl;
     return m_on_finish;
+  } else {
+    for (size_t i = 0; i < m_snapc.snaps.size(); ++i) {
+      m_snap_infos[i].timestamp = snap_timestamps[i];
     }
-
-  send_v2_get_snap_namespaces();
-  return nullptr;
-}
-
-template <typename I>
-void RefreshRequest<I>::send_v2_get_snap_namespaces() {
-  CephContext *cct = m_image_ctx.cct;
-  ldout(cct, 10) << this << " " << __func__ << dendl;
-
-  librados::ObjectReadOperation op;
-  cls_client::snapshot_namespace_list_start(&op, m_snapc.snaps);
-
-  using klass = RefreshRequest<I>;
-  librados::AioCompletion *comp = create_rados_callback<
-    klass, &klass::handle_v2_get_snap_namespaces>(this);
-  m_out_bl.clear();
-  int r = m_image_ctx.md_ctx.aio_operate(m_image_ctx.header_oid, comp, &op,
-                                         &m_out_bl);
-  assert(r == 0);
-  comp->release();
-}
-
-template <typename I>
-Context *RefreshRequest<I>::handle_v2_get_snap_namespaces(int *result) {
-  CephContext *cct = m_image_ctx.cct;
-  ldout(cct, 10) << this << " " << __func__ << ": "
-                 << "r=" << *result << dendl;
-
-  if (*result == 0) {
-    bufferlist::iterator it = m_out_bl.begin();
-    *result = cls_client::snapshot_namespace_list_finish(&it, m_snapc.snaps,
-                                                         &m_snap_namespaces);
-  }
-  if (*result == -ENOENT) {
-    ldout(cct, 10) << "out-of-sync snapshot state detected" << dendl;
-    send_v2_get_mutable_metadata();
-    return nullptr;
-  } else if (*result == -EOPNOTSUPP) {
-    m_snap_namespaces = std::vector
-				<cls::rbd::SnapshotNamespace>(
-					     m_snap_names.size(),
-					     cls::rbd::UserSnapshotNamespace());
-    // Ignore it means no snap namespaces are available
-  } else if (*result < 0) {
-    lderr(cct) << "failed to retrieve snapshots: " << cpp_strerror(*result)
-               << dendl;
-    return m_on_finish;
   }
 
   send_v2_refresh_parent();
   return nullptr;
 }
 
+
 template <typename I>
 void RefreshRequest<I>::send_v2_refresh_parent() {
   {
@@ -850,8 +867,8 @@ void RefreshRequest<I>::send_v2_open_object_map() {
   if (m_image_ctx.snap_name.empty()) {
     m_object_map = m_image_ctx.create_object_map(CEPH_NOSNAP);
   } else {
-    for (size_t snap_idx = 0; snap_idx < m_snap_names.size(); ++snap_idx) {
+    for (size_t snap_idx = 0; snap_idx < m_snap_infos.size(); ++snap_idx) {
-      if (m_snap_names[snap_idx] == m_image_ctx.snap_name) {
+      if (m_snap_infos[snap_idx].name == m_image_ctx.snap_name) {
         m_object_map = m_image_ctx.create_object_map(
           m_snapc.snaps[snap_idx].val);
         break;
@@ -1143,8 +1160,8 @@ void RefreshRequest<I>::apply() {
       if (it == m_image_ctx.snaps.end()) {
         m_flush_aio = true;
         ldout(cct, 20) << "new snapshot id=" << m_snapc.snaps[i].val
-                       << " name=" << m_snap_names[i]
+                       << " name=" << m_snap_infos[i].name
-                       << " size=" << m_snap_sizes[i]
+                       << " size=" << m_snap_infos[i].image_size
                        << dendl;
       }
     }
@@ -1162,9 +1179,11 @@ void RefreshRequest<I>::apply() {
         parent = m_snap_parents[i];
       }
 
-      m_image_ctx.add_snap(m_snap_namespaces[i], m_snap_names[i],
+      m_image_ctx.add_snap(m_snap_infos[i].snapshot_namespace,
-			   m_snapc.snaps[i].val, m_snap_sizes[i], parent,
+                           m_snap_infos[i].name, m_snapc.snaps[i].val,
-			   protection_status, flags, m_snap_timestamps[i]);
+                           m_snap_infos[i].image_size, parent,
+			   protection_status, flags,
+                           m_snap_infos[i].timestamp);
     }
     m_image_ctx.snapc = m_snapc;
 

src/librbd/image/RefreshRequest.h

@@ -65,14 +65,14 @@ private:
    *                |                                         |
    *                v                                         |
    *            V2_GET_SNAPSHOTS (skip if no snaps)           |
-   *                |                                         |
+   *                |       .                                 |
-   *                v                                         |
+   *                |       v (pre-mimic OSD)                 |
-   *            V2_GET_SNAP_TIMESTAMPS                        |
+   *                |   V2_GET_SNAPSHOTS_LEGACY               |
-   *                |                                         |
+   *                |       |                                 |
-   *                v                                         |
+   *                |       v                                 |
-   *            V2_GET_SNAP_NAMESPACES                        |
+   *                |   V2_GET_SNAP_TIMESTAMPS                |
-   *                |                                         |
+   *                |       |                                 |
-   *                v                                         |
+   *                v       v                                 |
    *            V2_REFRESH_PARENT (skip if no parent or       |
    *                |              refresh not needed)        |
    *                v                                         |
@@ -143,13 +143,10 @@ private:
   cls::rbd::GroupSpec m_group_spec;
 
   ::SnapContext m_snapc;
-  std::vector<std::string> m_snap_names;
+  std::vector<cls::rbd::SnapshotInfo> m_snap_infos;
-  std::vector<cls::rbd::SnapshotNamespace> m_snap_namespaces;
-  std::vector<uint64_t> m_snap_sizes;
   std::vector<ParentInfo> m_snap_parents;
   std::vector<uint8_t> m_snap_protection;
   std::vector<uint64_t> m_snap_flags;
-  std::vector<utime_t> m_snap_timestamps;
 
   std::map<rados::cls::lock::locker_id_t,
            rados::cls::lock::locker_info_t> m_lockers;
@@ -189,8 +186,8 @@ private:
   void send_v2_get_snapshots();
   Context *handle_v2_get_snapshots(int *result);
 
-  void send_v2_get_snap_namespaces();
+  void send_v2_get_snapshots_legacy();
-  Context *handle_v2_get_snap_namespaces(int *result);
+  Context *handle_v2_get_snapshots_legacy(int *result);
 
   void send_v2_get_snap_timestamps();
   Context *handle_v2_get_snap_timestamps(int *result);

src/librbd/journal/Types.cc

@@ -207,7 +207,7 @@ void SnapEventBase::encode(bufferlist& bl) const {
   using ceph::encode;
   OpEventBase::encode(bl);
   encode(snap_name, bl);
-  encode(cls::rbd::SnapshotNamespaceOnDisk(snap_namespace), bl);
+  encode(snap_namespace, bl);
 }
 
 void SnapEventBase::decode(__u8 version, bufferlist::iterator& it) {
@@ -216,16 +216,14 @@ void SnapEventBase::decode(__u8 version, bufferlist::iterator& it) {
   using ceph::decode;
   decode(snap_name, it);
   if (version >= 4) {
-    cls::rbd::SnapshotNamespaceOnDisk sn;
+    decode(snap_namespace, it);
-    decode(sn, it);
-    snap_namespace = sn.snapshot_namespace;
   }
 }
 
 void SnapEventBase::dump(Formatter *f) const {
   OpEventBase::dump(f);
   f->dump_string("snap_name", snap_name);
-  cls::rbd::SnapshotNamespaceOnDisk(snap_namespace).dump(f);
+  snap_namespace.dump(f);
 }
 
 void SnapCreateEvent::encode(bufferlist &bl) const {
@@ -236,9 +234,7 @@ void SnapCreateEvent::decode(__u8 version, bufferlist::iterator& it) {
   using ceph::decode;
   SnapEventBase::decode(version, it);
   if (version == 3) {
-    cls::rbd::SnapshotNamespaceOnDisk sn;
+    decode(snap_namespace, it);
-    decode(sn, it);
-    snap_namespace = sn.snapshot_namespace;
   }
 }
 
@@ -579,7 +575,7 @@ void MirrorPeerSyncPoint::encode(bufferlist& bl) const {
   encode(snap_name, bl);
   encode(from_snap_name, bl);
   encode(object_number, bl);
-  encode(cls::rbd::SnapshotNamespaceOnDisk(snap_namespace), bl);
+  encode(snap_namespace, bl);
 }
 
 void MirrorPeerSyncPoint::decode(__u8 version, bufferlist::iterator& it) {
@@ -588,9 +584,7 @@ void MirrorPeerSyncPoint::decode(__u8 version, bufferlist::iterator& it) {
   decode(from_snap_name, it);
   decode(object_number, it);
   if (version >= 2) {
-    cls::rbd::SnapshotNamespaceOnDisk sn;
+    decode(snap_namespace, it);
-    decode(sn, it);
-    snap_namespace = sn.snapshot_namespace;
   }
 }
 
@@ -600,7 +594,7 @@ void MirrorPeerSyncPoint::dump(Formatter *f) const {
   if (object_number) {
     f->dump_unsigned("object_number", *object_number);
   }
-  cls::rbd::SnapshotNamespaceOnDisk(snap_namespace).dump(f);
+  snap_namespace.dump(f);
 }
 
 void MirrorPeerClientMeta::encode(bufferlist& bl) const {

src/test/cls_rbd/test_cls_rbd.cc

@@ -796,8 +796,8 @@ TEST_F(TestClsRbd, snapshots)
 
   ASSERT_EQ(0, create_image(&ioctx, oid, 10, 22, 0, oid, -1));
 
+  vector<cls::rbd::SnapshotInfo> snaps;
   vector<string> snap_names;
-  vector<cls::rbd::SnapshotNamespace> snap_namespaces;
   vector<uint64_t> snap_sizes;
   SnapContext snapc;
   vector<ParentInfo> parents;
@@ -807,11 +807,14 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(0, get_snapcontext(&ioctx, oid, &snapc));
   ASSERT_EQ(0u, snapc.snaps.size());
   ASSERT_EQ(0u, snapc.seq);
+  ASSERT_EQ(0, snapshot_get(&ioctx, oid, snapc.snaps, &snaps,
+                            &parents, &protection_status));
+  ASSERT_EQ(0u, snaps.size());
+  ASSERT_EQ(0u, parents.size());
+  ASSERT_EQ(0u, protection_status.size());
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(0u, snap_names.size());
-  ASSERT_EQ(0u, snap_namespaces.size());
   ASSERT_EQ(0u, snap_sizes.size());
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(0u, snap_timestamps.size());
@@ -821,13 +824,17 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(1u, snapc.snaps.size());
   ASSERT_EQ(0u, snapc.snaps[0]);
   ASSERT_EQ(0u, snapc.seq);
+  ASSERT_EQ(0, snapshot_get(&ioctx, oid, snapc.snaps, &snaps,
+                            &parents, &protection_status));
+  ASSERT_EQ(1u, snaps.size());
+  ASSERT_EQ(1u, parents.size());
+  ASSERT_EQ(1u, protection_status.size());
+  ASSERT_EQ("snap1", snaps[0].name);
+  ASSERT_EQ(userSnapNamespace, snaps[0].snapshot_namespace);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(1u, snap_names.size());
   ASSERT_EQ("snap1", snap_names[0]);
-  ASSERT_EQ(1u, snap_namespaces.size());
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(1u, snap_timestamps.size());
@@ -840,11 +847,8 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(0u, snapc.seq);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(1u, snap_names.size());
   ASSERT_EQ("snap1", snap_names[0]);
-  ASSERT_EQ(1u, snap_namespaces.size());
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(1u, snap_timestamps.size());
@@ -858,11 +862,8 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(0u, snapc.seq);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(1u, snap_names.size());
   ASSERT_EQ("snap1", snap_names[0]);
-  ASSERT_EQ(1u, snap_namespaces.size());
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(1u, snap_timestamps.size());
@@ -875,11 +876,8 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(0u, snapc.seq);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(snap_names.size(), 1u);
   ASSERT_EQ(snap_names[0], "snap1");
-  ASSERT_EQ(1u, snap_namespaces.size());
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(snap_sizes[0], 10u);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(1u, snap_timestamps.size());
@@ -891,16 +889,21 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(1u, snapc.snaps[0]);
   ASSERT_EQ(0u, snapc.snaps[1]);
   ASSERT_EQ(1u, snapc.seq);
+  ASSERT_EQ(0, snapshot_get(&ioctx, oid, snapc.snaps, &snaps,
+                            &parents, &protection_status));
+  ASSERT_EQ(2u, snaps.size());
+  ASSERT_EQ(2u, parents.size());
+  ASSERT_EQ(2u, protection_status.size());
+  ASSERT_EQ("snap2", snaps[0].name);
+  ASSERT_EQ("snap1", snaps[1].name);
+  ASSERT_EQ(userSnapNamespace, snaps[0].snapshot_namespace);
+  ASSERT_EQ(userSnapNamespace, snaps[1].snapshot_namespace);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(2u, snap_names.size());
-  ASSERT_EQ(2u, snap_namespaces.size());
   ASSERT_EQ("snap2", snap_names[0]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ("snap1", snap_names[1]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[1]);
   ASSERT_EQ(10u, snap_sizes[1]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(2u, snap_timestamps.size());
@@ -908,14 +911,10 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(0, snapshot_rename(&ioctx, oid, 0, "snap1-rename"));
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(2u, snap_names.size());
-  ASSERT_EQ(2u, snap_namespaces.size());
   ASSERT_EQ("snap2", snap_names[0]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ("snap1-rename", snap_names[1]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[1]);
   ASSERT_EQ(10u, snap_sizes[1]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(2u, snap_timestamps.size());
@@ -925,13 +924,17 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(1u, snapc.snaps.size());
   ASSERT_EQ(1u, snapc.snaps[0]);
   ASSERT_EQ(1u, snapc.seq);
+  ASSERT_EQ(0, snapshot_get(&ioctx, oid, snapc.snaps, &snaps,
+                            &parents, &protection_status));
+  ASSERT_EQ(1u, snaps.size());
+  ASSERT_EQ(1u, parents.size());
+  ASSERT_EQ(1u, protection_status.size());
+  ASSERT_EQ("snap2", snaps[0].name);
+  ASSERT_EQ(userSnapNamespace, snaps[0].snapshot_namespace);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(1u, snap_names.size());
-  ASSERT_EQ(1u, snap_namespaces.size());
   ASSERT_EQ("snap2", snap_names[0]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(1u, snap_timestamps.size());
@@ -952,14 +955,10 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(large_snap_id, snapc.seq);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(2u, snap_names.size());
-  ASSERT_EQ(2u, snap_namespaces.size());
   ASSERT_EQ("snap3", snap_names[0]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(0u, snap_sizes[0]);
   ASSERT_EQ("snap2", snap_names[1]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[1]);
   ASSERT_EQ(10u, snap_sizes[1]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(2u, snap_timestamps.size());
@@ -979,11 +978,8 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(large_snap_id, snapc.seq);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(1u, snap_names.size());
-  ASSERT_EQ(1u, snap_namespaces.size());
   ASSERT_EQ("snap2", snap_names[0]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[0]);
   ASSERT_EQ(10u, snap_sizes[0]);
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(1u, snap_timestamps.size());
@@ -995,9 +991,7 @@ TEST_F(TestClsRbd, snapshots)
   ASSERT_EQ(large_snap_id, snapc.seq);
   ASSERT_EQ(0, snapshot_list(&ioctx, oid, snapc.snaps, &snap_names,
 			     &snap_sizes, &parents, &protection_status));
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
   ASSERT_EQ(0u, snap_names.size());
-  ASSERT_EQ(0u, snap_namespaces.size());
   ASSERT_EQ(0u, snap_sizes.size());
   ASSERT_EQ(0, snapshot_timestamp_list(&ioctx, oid, snapc.snaps, &snap_timestamps));
   ASSERT_EQ(0u, snap_timestamps.size());
@@ -1005,45 +999,6 @@ TEST_F(TestClsRbd, snapshots)
   ioctx.close();
 }
 
-TEST_F(TestClsRbd, snapshots_namespaces)
-{
-  cls::rbd::SnapshotNamespace groupSnapNamespace = cls::rbd::GroupSnapshotNamespace(5, "1018643c9869", "3338524f9933");
-  cls::rbd::SnapshotNamespace userSnapNamespace = cls::rbd::UserSnapshotNamespace();
-  librados::IoCtx ioctx;
-  ASSERT_EQ(0, _rados.ioctx_create(_pool_name.c_str(), ioctx));
-
-  string oid = get_temp_image_name();
-
-  ASSERT_EQ(0, create_image(&ioctx, oid, 10, 22, 0, oid, -1));
-
-  vector<cls::rbd::SnapshotNamespace> snap_namespaces;
-  SnapContext snapc;
-
-  ASSERT_EQ(0, get_snapcontext(&ioctx, oid, &snapc));
-  ASSERT_EQ(0u, snapc.snaps.size());
-  ASSERT_EQ(0u, snapc.seq);
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
-  ASSERT_EQ(0u, snap_namespaces.size());
-
-  ASSERT_EQ(0, snapshot_add(&ioctx, oid, 0, "snap1"));
-
-  librados::ObjectWriteOperation op;
-  ::librbd::cls_client::snapshot_add(&op, 1, "snap1", groupSnapNamespace);
-  int r = ioctx.operate(oid, &op);
-  ASSERT_EQ(0, r);
-
-  ASSERT_EQ(0, get_snapcontext(&ioctx, oid, &snapc));
-  ASSERT_EQ(2u, snapc.snaps.size());
-  ASSERT_EQ(1u, snapc.snaps[0]);
-  ASSERT_EQ(0u, snapc.snaps[1]);
-  ASSERT_EQ(1u, snapc.seq);
-  ASSERT_EQ(0, snapshot_namespace_list(&ioctx, oid, snapc.snaps, &snap_namespaces));
-  ASSERT_EQ(groupSnapNamespace, snap_namespaces[0]);
-  ASSERT_EQ(userSnapNamespace, snap_namespaces[1]);
-
-  ioctx.close();
-}
-
 TEST_F(TestClsRbd, snapshots_timestamps)
 {
   librados::IoCtx ioctx;
@@ -1071,7 +1026,6 @@ TEST_F(TestClsRbd, snapshots_timestamps)
   ioctx.close();
 }
 
-
 TEST_F(TestClsRbd, snapid_race)
 {
   librados::IoCtx ioctx;

src/test/encoding/types.h

@@ -421,10 +421,11 @@ TYPE(cls::rbd::MirrorImageMap)
 TYPE(cls::rbd::MirrorImageStatus)
 TYPE(cls::rbd::GroupImageSpec)
 TYPE(cls::rbd::GroupImageStatus)
-TYPE(cls::rbd::GroupSpec)
-TYPE(cls::rbd::SnapshotNamespaceOnDisk)
-TYPE(cls::rbd::ImageSnapshotSpec)
 TYPE(cls::rbd::GroupSnapshot)
+TYPE(cls::rbd::GroupSpec)
+TYPE(cls::rbd::ImageSnapshotSpec)
+TYPE(cls::rbd::SnapshotInfo)
+TYPE(cls::rbd::SnapshotNamespace)
 #endif
 
 #include "cls/lock/cls_lock_types.h"

src/test/librbd/image/test_mock_RefreshRequest.cc

@@ -69,7 +69,6 @@ RefreshParentRequest<MockRefreshImageCtx>* RefreshParentRequest<MockRefreshImage
 
 // template definitions
 #include "librbd/image/RefreshRequest.cc"
-template class librbd::image::RefreshRequest<librbd::MockRefreshImageCtx>;
 
 ACTION_P(TestFeatures, image_ctx) {
   return ((image_ctx->features & arg0) != 0);
@@ -210,8 +209,23 @@ public:
     }
   }
 
-
   void expect_get_snapshots(MockRefreshImageCtx &mock_image_ctx, int r) {
+    auto &expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx),
+                               exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("snapshot_get"), _, _, _));
+    if (r < 0) {
+      expect.WillOnce(Return(r));
+    } else {
+      expect.WillOnce(DoDefault());
+      EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx),
+                  exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("get_parent"), _, _, _))
+                    .WillOnce(DoDefault());
+      EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx),
+                  exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("get_protection_status"), _, _, _))
+                    .WillOnce(DoDefault());
+    }
+  }
+
+  void expect_get_snapshots_legacy(MockRefreshImageCtx &mock_image_ctx, int r) {
     auto &expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx),
                                exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("get_snapshot_name"), _, _, _));
     if (r < 0) {
@@ -240,16 +254,6 @@ public:
     }
   }
 
-  void expect_snap_namespace_list(MockRefreshImageCtx &mock_image_ctx, int r) {
-    auto &expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx),
-                               exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("get_snapshot_namespace"), _, _, _));
-    if (r < 0) {
-      expect.WillOnce(Return(r));
-    } else {
-      expect.WillOnce(DoDefault());
-    }
-  }
-
   void expect_apply_metadata(MockRefreshImageCtx &mock_image_ctx,
 			     int r) {
     EXPECT_CALL(mock_image_ctx, apply_metadata(_, false))
@@ -475,8 +479,42 @@ TEST_F(TestMockImageRefreshRequest, SuccessSnapshotV2) {
   expect_get_flags(mock_image_ctx, 0);
   expect_get_group(mock_image_ctx, 0);
   expect_get_snapshots(mock_image_ctx, 0);
+  expect_refresh_parent_is_required(mock_refresh_parent_request, false);
+  if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) {
+    expect_init_exclusive_lock(mock_image_ctx, mock_exclusive_lock, 0);
+  }
+  expect_add_snap(mock_image_ctx, "snap", ictx->snap_ids.begin()->second);
+
+  C_SaferCond ctx;
+  MockRefreshRequest *req = new MockRefreshRequest(mock_image_ctx, false, false, &ctx);
+  req->send();
+
+  ASSERT_EQ(0, ctx.wait());
+}
+
+TEST_F(TestMockImageRefreshRequest, SuccessLegacySnapshotV2) {
+  REQUIRE_FORMAT_V2();
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+  ASSERT_EQ(0, snap_create(*ictx, "snap"));
+
+  MockRefreshImageCtx mock_image_ctx(*ictx);
+  MockRefreshParentRequest mock_refresh_parent_request;
+  MockExclusiveLock mock_exclusive_lock;
+  expect_op_work_queue(mock_image_ctx);
+  expect_test_features(mock_image_ctx);
+
+  InSequence seq;
+  expect_get_mutable_metadata(mock_image_ctx, ictx->features, 0);
+  expect_get_metadata(mock_image_ctx, 0);
+  expect_apply_metadata(mock_image_ctx, 0);
+  expect_get_flags(mock_image_ctx, 0);
+  expect_get_flags(mock_image_ctx, 0);
+  expect_get_group(mock_image_ctx, 0);
+  expect_get_snapshots(mock_image_ctx, -EOPNOTSUPP);
+  expect_get_snapshots_legacy(mock_image_ctx, 0);
   expect_snap_timestamp_list(mock_image_ctx, 0);
-  expect_snap_namespace_list(mock_image_ctx, 0);
   expect_refresh_parent_is_required(mock_refresh_parent_request, false);
   if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) {
     expect_init_exclusive_lock(mock_image_ctx, mock_exclusive_lock, 0);
@@ -512,8 +550,6 @@ TEST_F(TestMockImageRefreshRequest, SuccessSetSnapshotV2) {
   expect_get_flags(mock_image_ctx, 0);
   expect_get_group(mock_image_ctx, 0);
   expect_get_snapshots(mock_image_ctx, 0);
-  expect_snap_timestamp_list(mock_image_ctx, 0);
-  expect_snap_namespace_list(mock_image_ctx, 0);
   expect_refresh_parent_is_required(mock_refresh_parent_request, false);
   if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
     expect_open_object_map(mock_image_ctx, &mock_object_map, 0);