Re: Snapshots of consistency groups

[Victor Denisov <vdenisov@xxxxxxxxxxxx>]

Hi Jason,

I have a question about implementation of snapshot namespaces in cls_rbd.

What if I implement encode and decode functions for SnapshotNamespace
types as follows:

inline void encode(const SnapshotNamespace &c, bufferlist &bl,
uint64_t features=0)
{
  ENCODE_DUMP_PRE();
  boost::apply_visitor(EncodeSnapshotTypeVisitor(bl), c);
  ENCODE_DUMP_POST(SnapshotNamespace);
}

inline void decode(SnapshotNamespace &c, bufferlist::iterator &p)
{
  uint32_t snap_type;
  ::decode(snap_type, p);
  switch (snap_type) {
    case cls::rbd::SNAPSHOT_NAMESPACE_TYPE_USER:
      c = UserSnapshotNamespace();
      break;
    case cls::rbd::SNAPSHOT_NAMESPACE_TYPE_GROUP:
      c = GroupSnapshotNamespace();
      break;
    default:
      c = UnknownSnapshotNamespace();
      break;
  }
  boost::apply_visitor(DecodeSnapshotTypeVisitor(p), c);
}

https://github.com/VictorDenisov/ceph/blob/consistency_groups_namespaces/src/cls/rbd/cls_rbd_types.h#L314

and then instead of


if (struct_v >= 5) {
      uint32_t snap_type;
      ::decode(snap_type, p);
      snapshot_namespace_type =
static_cast<cls::rbd::SnapshotNamespaceType>(snap_type);
      switch (snap_type) {
        case cls::rbd::SNAPSHOT_NAMESPACE_TYPE_USER:
          snapshot_namespace = cls::rbd::UserSnapshotNamespace();
          break;
        case cls::rbd::SNAPSHOT_NAMESPACE_TYPE_GROUP:
          snapshot_namespace = cls::rbd::GroupSnapshotNamespace();
          break;
        default:
          snapshot_namespace = cls::rbd::UnknownSnapshotNamespace();
          break;
      }
      boost::apply_visitor(cls::rbd::DecodeSnapshotTypeVisitor(p),
snapshot_namespace);
    } else {
      snapshot_namespace = cls::rbd::UserSnapshotNamespace();
    }


in cls_rbd_snap decoding I would write just:

if (struct_v >= 5) {
      ::decode(snapshot_namespace, p);
    } else {
      snapshot_namespace = cls::rbd::UserSnapshotNamespace();
    }

then code for ::encode function of cls_rbd_snap would change accordingly:

instead of

boost::apply_visitor(cls::rbd::EncodeSnapshotTypeVisitor(bl),
snapshot_namespace);

I would do:
::encode(snapshot_namespace, bl);

The reason I would like to have encode and decode for
SnapshotNamespace implemented like this is because when I implement
get_snap_namespace method in cls_rbd.cc it's very convenient to have
conventional encode and decode functions. get_snap_namespace has to
serialize and deserialize results.

https://github.com/VictorDenisov/ceph/blob/consistency_groups_namespaces/src/cls/rbd/cls_rbd.cc#L1492


What do you think?

Thanks,
V.

On Tue, Sep 13, 2016 at 4:41 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote:
> Published a pull request for extracting Group.cc
>
> https://github.com/ceph/ceph/pull/11070
>
> Please review.
>
>
> On Tue, Sep 13, 2016 at 5:33 AM, Jason Dillaman <jdillama@xxxxxxxxxx> wrote:
>> On Mon, Sep 12, 2016 at 7:09 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote:
>>> I also think, probably, moving existing group api to Group.cc can be
>>> done in a separate pull request.
>>
>> Agreed -- no worries.
>>
>>> On Mon, Sep 12, 2016 at 4:06 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote:
>>>> Another quick question.
>>>> Do you think it makes sense to introduce snapshot namespaces in a pull
>>>> request and review it first?
>>>> It looks like a self sufficient change that we can merge before
>>>> introducing snapshots.
>>
>> Yes, I think that would be a good idea to simplify the PR and get it
>> merged potentially quicker.
>>
>> --
>> Jason
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html