New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
librbd: add SnapshotNamespace to ImageCtx #12970
Conversation
src/librbd/ImageCtx.cc
Outdated
{ | ||
assert(snap_lock.is_locked()); | ||
map<string, snap_t>::const_iterator it = | ||
snap_ids.find(in_snap_name); | ||
map<pair<cls::rbd::SnapshotNamespace, string>, snap_t>::const_iterator it = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: feel free to use auto
here to avoid having very long typedef types
src/librbd/ImageCtx.cc
Outdated
map<string, snap_t>::const_iterator it = | ||
snap_ids.find(in_snap_name); | ||
map<pair<cls::rbd::SnapshotNamespace, string>, snap_t>::const_iterator it = | ||
snap_ids.find(make_pair(in_snap_namespace, in_snap_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: not a big deal, but you can also just use {in_snap_namespace, in_snap_name}
to auto-create the pair.
src/librbd/ImageCtx.cc
Outdated
@@ -552,15 +555,19 @@ struct C_InvalidateCache : public Context { | |||
SnapInfo info(in_snap_name, in_snap_namespace, | |||
in_size, parent, protection_status, flags); | |||
snap_info.insert(pair<snap_t, SnapInfo>(id, info)); | |||
snap_ids.insert(pair<string, snap_t>(in_snap_name, id)); | |||
snap_ids.insert(pair<pair<cls::rbd::SnapshotNamespace, string>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: good example of using C++11 brace initialization to make the code look cleaner
bcacaf6
to
3b58e62
Compare
@dillaman Could you please take a look at internal.cc in this PR request. Basically every method that was supposed to look up snapshots by their names now receives an additional argument - snapshot namespace. I presume CLI code will be using UserSnapshotNamespace by default. As I understand we don't want to add more parameters to command line tool in order to allow users to specify what snap shot namespace they are using. Please clarify if I'm moving into the right direction. |
src/librbd/ImageCtx.cc
Outdated
{ | ||
assert(snap_lock.is_wlocked()); | ||
snaps.erase(std::remove(snaps.begin(), snaps.end(), id), snaps.end()); | ||
snap_info.erase(id); | ||
snap_ids.erase(in_snap_name); | ||
snap_ids.erase(make_pair(in_snap_namespace, in_snap_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: feel free to use brace initialization here as well
@@ -428,11 +430,11 @@ struct C_InvalidateCache : public Context { | |||
data_ctx.snap_set_read(snap_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snap_namespace = {};
src/librbd/DiffIterate.cc
Outdated
@@ -268,7 +268,8 @@ int DiffIterate::execute() { | |||
r = 0; | |||
if (m_image_ctx.parent && overlap > 0) { | |||
ldout(cct, 10) << " first getting parent diff" << dendl; | |||
DiffIterate diff_parent(*m_image_ctx.parent, NULL, 0, overlap, | |||
DiffIterate diff_parent(*m_image_ctx.parent, cls::rbd::UnknownSnapshotNamespace(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: replace cls::rbd::UnknownSnapshotNamespace
with {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume in order to use this I should reorder Namespaces in the definition of Variant. So that UnknownSnapshotNamespace is the first one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it compiles, it doesn't really matter since the name is null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I see
src/librbd/DiffIterate.cc
Outdated
@@ -268,7 +268,8 @@ int DiffIterate::execute() { | |||
r = 0; | |||
if (m_image_ctx.parent && overlap > 0) { | |||
ldout(cct, 10) << " first getting parent diff" << dendl; | |||
DiffIterate diff_parent(*m_image_ctx.parent, NULL, 0, overlap, | |||
DiffIterate diff_parent(*m_image_ctx.parent, cls::rbd::UnknownSnapshotNamespace(), | |||
NULL, 0, overlap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: while you are here, can you chance NULL
to nullptr
src/librbd/internal.cc
Outdated
@@ -677,13 +677,14 @@ void filter_out_mirror_watchers(ImageCtx *ictx, | |||
return 0; | |||
} | |||
|
|||
int flatten_children(ImageCtx *ictx, const char* snap_name, ProgressContext& pctx) | |||
int flatten_children(ImageCtx *ictx, const cls::rbd::SnapshotNamespace& snap_namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are safe to assume this is for an ImageSnapshotNamespace
only
src/librbd/internal.cc
Outdated
|
||
int snap_is_protected(ImageCtx *ictx, const char *snap_name, | ||
bool *is_protected) | ||
int snap_is_protected(ImageCtx *ictx, const cls::rbd::SnapshotNamespace& snap_namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are safe to assume this is for an ImageSnapshotNamespace
only
src/librbd/internal.cc
Outdated
return 0; | ||
} | ||
|
||
int snap_remove(ImageCtx *ictx, const char *snap_name, uint32_t flags, ProgressContext& pctx) | ||
int snap_remove(ImageCtx *ictx, const cls::rbd::SnapshotNamespace& snap_namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are safe to assume this is for an ImageSnapshotNamespace
only
src/librbd/internal.h
Outdated
@@ -184,7 +182,8 @@ namespace librbd { | |||
int64_t read_iterate(ImageCtx *ictx, uint64_t off, uint64_t len, | |||
int (*cb)(uint64_t, size_t, const char *, void *), | |||
void *arg); | |||
int diff_iterate(ImageCtx *ictx, const char *fromsnapname, uint64_t off, | |||
int diff_iterate(ImageCtx *ictx, const cls::rbd::SnapshotNamespace& fromsnapnamespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can you add underscores between words: fromsnapnamespace
-> from_snap_namespace
src/librbd/internal.cc
Outdated
@@ -2343,7 +2317,8 @@ void filter_out_mirror_watchers(ImageCtx *ictx, | |||
return total_read; | |||
} | |||
|
|||
int diff_iterate(ImageCtx *ictx, const char *fromsnapname, uint64_t off, | |||
int diff_iterate(ImageCtx *ictx, const cls::rbd::SnapshotNamespace fromsnapnamespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same underscore comment here
baf59c7
to
3f0b60e
Compare
@dillaman Could you please take a look at these changes. So far I changed all required files in librbd directory, unit tests are still to be fixed. |
src/librbd/ImageState.h
Outdated
@@ -73,6 +74,7 @@ class ImageState { | |||
ActionType action_type; | |||
uint64_t refresh_seq = 0; | |||
std::string snap_name; | |||
cls::rbd::SnapshotNamespace snap_namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to update ImageState::set_snap
to take a snapshot namespace and assign the snapshot namespace to this param.
src/librbd/WatchNotifyTypes.h
Outdated
|
||
void encode(bufferlist &bl) const; | ||
void decode(__u8 version, bufferlist::iterator &iter); | ||
void dump(Formatter *f) const; | ||
|
||
protected: | ||
SnapPayloadBase() {} | ||
SnapPayloadBase(const std::string &name) : snap_name(name) {} | ||
SnapPayloadBase(const std::string &name, const cls::rbd::SnapshotNamespace& _snap_namespace) : snap_name(name), snap_namespace(_snap_namespace) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: avoid going past 80 characters in a single line. This comment applies to all changes in this file.
@@ -165,11 +165,17 @@ void RefreshParentRequest<I>::send_set_parent_snap() { | |||
return; | |||
} | |||
|
|||
cls::rbd::SnapshotNamespace snap_namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge this logic w/ the logic on line 159. This implies we need a new helper method ImageCtx::get_snap
which will populate both the snap name and namespace. This avoids weird errors where you drop the lock and the snapshot disappears. Also fixes the case where the potential error isn't currently handled below.
src/librbd/image/SetSnapRequest.cc
Outdated
Context *on_finish) | ||
: m_image_ctx(image_ctx), m_snap_name(snap_name), m_on_finish(on_finish), | ||
: m_image_ctx(image_ctx), m_snap_name(snap_name), m_snap_namespace(snap_namespace), m_on_finish(on_finish), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: max line length
src/librbd/internal.cc
Outdated
int diff_iterate(ImageCtx *ictx, const cls::rbd::SnapshotNamespace from_snap_namespace, | ||
const char *fromsnapname, uint64_t off, | ||
int diff_iterate(ImageCtx *ictx, const char *fromsnapname, | ||
const cls::rbd::SnapshotNamespace from_snap_namespace, uint64_t off, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: pass by ref
src/librbd/journal/Types.h
Outdated
|
||
protected: | ||
SnapEventBase() { | ||
} | ||
SnapEventBase(uint64_t op_tid, const std::string &_snap_name) | ||
: OpEventBase(op_tid), snap_name(_snap_name) { | ||
SnapEventBase(uint64_t op_tid, const std::string &_snap_name, const cls::rbd::SnapshotNamespace& _snap_namespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: max line length throughout the changes to this file
src/librbd/journal/Types.h
Outdated
@@ -121,12 +121,13 @@ struct OpFinishEvent : public OpEventBase { | |||
|
|||
struct SnapEventBase : public OpEventBase { | |||
std::string snap_name; | |||
cls::rbd::SnapshotNamespace snap_namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only snap create and remove would require the namespace -- all other ops imply the user snapshot namespace. Either create a new SnapNamespaceBase for those two structs to inherit from or just add the snap namespace to those two ops. This comment applies to the notifications in WatchNotifyTypes as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... also, I don't see the namespace being encoded/decoded. Ensure that you bump the struct version when you do add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that only user facing functions assume UserSnapshotNamespace by default. Internally we may need to flatten image snapshots, protect and unprotect them. For example when we clone group snapshots into another consistency group). As far as I understand these types are required for those internal operations. Am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, my impression that as maximum - rename op doesn't need this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough -- although I'm still not sure how cloning is going to work for groups since OpenStack allows you to remove images from the group and yet still create group clones from a group snapshot. Lots of holes in the design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my conversation with Xing: http://git.net/ml/openstack-dev/2016-12/msg00323.html
When we clone a consistency group we basically create a new image from every snapshot in this group snapshot. Then we make a group of those images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the creation of new images should be a clone operation (that's RBD's core design win for OpenStack). That implies that there will be lots of edge cases to handle OpenStack since OpenStack won't treat the images as dependent. We can tackle that issue later.
src/librbd/image/RefreshRequest.cc
Outdated
@@ -1008,7 +1008,7 @@ void RefreshRequest<I>::apply() { | |||
m_image_ctx.snapc = m_snapc; | |||
|
|||
if (m_image_ctx.snap_id != CEPH_NOSNAP && | |||
m_image_ctx.get_snap_id(m_image_ctx.snap_name) != m_image_ctx.snap_id) { | |||
m_image_ctx.get_snap_id(m_image_ctx.snap_namespace, m_image_ctx.snap_name) != m_image_ctx.snap_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
78f614c
to
1cb5c9e
Compare
@VictorDenisov The PR doesn't compile right now |
yeah, I'm working on compiling unittest_rbd_mirror target. I hope it's the last one. |
0c8b865
to
091316d
Compare
Now I need to fix the tests and it's done. |
ae100ef
to
d424a64
Compare
@dillaman done |
@@ -247,7 +248,7 @@ void SnapshotCopyRequest<I>::send_snap_remove() { | |||
SnapshotCopyRequest<I>, &SnapshotCopyRequest<I>::handle_snap_remove>( | |||
this); | |||
RWLock::RLocker owner_locker(m_local_image_ctx->owner_lock); | |||
m_local_image_ctx->operations->execute_snap_remove(m_snap_name.c_str(), ctx); | |||
m_local_image_ctx->operations->execute_snap_remove(m_snap_name.c_str(), cls::rbd::UserSnapshotNamespace(), ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: 80 character line limit
@@ -27,11 +27,11 @@ const std::string &get_snapshot_name(I *image_ctx, librados::snap_t snap_id) { | |||
auto snap_it = std::find_if(image_ctx->snap_ids.begin(), | |||
image_ctx->snap_ids.end(), | |||
[snap_id]( | |||
const std::pair<std::string, librados::snap_t> &pair) { | |||
const std::pair<std::pair<cls::rbd::SnapshotNamespace, std::string>, librados::snap_t> &pair) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: perhaps just switch to an auto &
here to avoid the long line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like auto in lambdas produces a warning unless I specify parameter -std=c++1y. Do you prefer the warning or just wrap the lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to leave it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, by "leave it" you mean wrap the lines. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No warnings, no switching to C++14 standard -- just try to avoid >80 characters if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
src/librbd/ImageCtx.cc
Outdated
{ | ||
assert(snap_lock.is_wlocked()); | ||
snap_t in_snap_id = get_snap_id(in_snap_name); | ||
snap_t in_snap_id = get_snap_id(in_snap_name, in_snap_namespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: still missing a consistent calling convention for snapshot namespace + name. I would just suggest always using namespace followed by name as the convention and modify where appropriate since that follows the map key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to have name and then namespace? I think this is the only place where I confused this order. I was going for name, namespace order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine -- just want to keep it consistent. The rationale for <namespace, name> is because that is how it's actually stored in ImageCtx
right now (but it could also be switched to <name, namespace>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like I can't change the order of namespace and name in snap_ids map. I use function: lower_bound when I look for a snapshot by its namespace when I don't know its name. Therefore namespace should go first. I'll need to change everything to be in the order: name, namespace. Unless you can change my view as you often do)
src/librbd/ImageWatcher.cc
Outdated
assert(m_image_ctx.owner_lock.is_locked()); | ||
assert(m_image_ctx.exclusive_lock && | ||
!m_image_ctx.exclusive_lock->is_lock_owner()); | ||
|
||
notify_lock_owner(SnapRenamePayload(src_snap_id, dst_snap_name), on_finish); | ||
notify_lock_owner(SnapRenamePayload(src_snap_id, dst_snap_name, cls::rbd::UserSnapshotNamespace()), on_finish); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
src/librbd/journal/Types.h
Outdated
@@ -173,7 +178,9 @@ struct SnapRenameEvent : public SnapEventBase { | |||
SnapRenameEvent(uint64_t op_tid, uint64_t src_snap_id, | |||
const std::string &src_snap_name, | |||
const std::string &dest_snap_name) | |||
: SnapEventBase(op_tid, dest_snap_name), snap_id(src_snap_id), | |||
: SnapEventBase(op_tid, dest_snap_name, | |||
cls::rbd::UserSnapshotNamespace()), // Only snapshots from user namespace are allowed to be renamed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: indentation
EXPECT_CALL(mock_image_ctx, get_snap_id(_)) | ||
.WillRepeatedly(Invoke([&mock_image_ctx](std::string snap_name) { | ||
EXPECT_CALL(mock_image_ctx, get_snap_id(_, _)) | ||
.WillRepeatedly(Invoke([&mock_image_ctx](std::string snap_name, cls::rbd::SnapshotNamespace snap_namespace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
@@ -313,7 +317,7 @@ TEST_F(TestMockImageSyncImageCopyRequest, SnapshotSubset) { | |||
ASSERT_EQ(0, create_snap("snap1")); | |||
ASSERT_EQ(0, create_snap("snap2")); | |||
ASSERT_EQ(0, create_snap("snap3")); | |||
m_client_meta.sync_points = {{"snap3", "snap2", boost::none}}; | |||
m_client_meta.sync_points = {{"snap3", "snap2", boost::none, cls::rbd::UserSnapshotNamespace()}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
@@ -383,7 +387,7 @@ TEST_F(TestMockImageSyncImageCopyRequest, RestartCatchup) { | |||
|
|||
TEST_F(TestMockImageSyncImageCopyRequest, RestartPartialSync) { | |||
ASSERT_EQ(0, create_snap("snap1")); | |||
m_client_meta.sync_points = {{"snap1", librbd::journal::MirrorPeerSyncPoint::ObjectNumber{0U}}}; | |||
m_client_meta.sync_points = {{"snap1", librbd::journal::MirrorPeerSyncPoint::ObjectNumber{0U}, cls::rbd::UserSnapshotNamespace()}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
@@ -131,7 +131,7 @@ TestPoolWatcher() : m_lock("TestPoolWatcherLock"), | |||
ictx->state->open(false); | |||
EXPECT_EQ(0, ictx->operations->snap_create(snap_name.c_str(), | |||
cls::rbd::UserSnapshotNamespace())); | |||
EXPECT_EQ(0, ictx->operations->snap_protect(snap_name.c_str())); | |||
EXPECT_EQ(0, ictx->operations->snap_protect(snap_name.c_str(), cls::rbd::UserSnapshotNamespace())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
@@ -197,8 +197,8 @@ class TestMockImageSync : public TestMockFixture { | |||
EXPECT_CALL(mock_sync_point_create_request, send()) | |||
.WillOnce(Invoke([this, &mock_local_image_ctx, &mock_sync_point_create_request, r]() { | |||
if (r == 0) { | |||
mock_local_image_ctx.snap_ids["snap1"] = 123; | |||
m_client_meta.sync_points.emplace_back("snap1", boost::none); | |||
mock_local_image_ctx.snap_ids[{cls::rbd::UserSnapshotNamespace(), "snap1"}] = 123; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: line length
@VictorDenisov I would recommend just squashing these three commits into a single commit since they don't compile independently. |
0dd9226
to
e04700f
Compare
@dillaman it's ready |
retest this please |
@@ -435,11 +437,11 @@ struct C_InvalidateCache : public Context { | |||
data_ctx.snap_set_read(snap_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: snap_namespace = {};
@@ -218,6 +218,7 @@ struct SnapPayloadBase { | |||
public: | |||
static const bool CHECK_FOR_REFRESH = true; | |||
|
|||
cls::rbd::SnapshotNamespace snap_namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't encoded/decoded/dumped (in the respective methods). It's in SnapCreatePayload -- you can probably do some magic where if the version == 5, you decode it in SnapCreatePayload -- otherwise let the base class handle it.
src/librbd/WatchNotifyTypes.h
Outdated
SnapProtectPayload(const std::string &name) : SnapPayloadBase(name) {} | ||
SnapProtectPayload(const cls::rbd::SnapshotNamespace& snap_namespace, | ||
const std::string &name) | ||
: SnapPayloadBase(snap_namespace, name) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: reduce indentation
@@ -140,13 +140,17 @@ struct OpFinishEvent : public OpEventBase { | |||
}; | |||
|
|||
struct SnapEventBase : public OpEventBase { | |||
cls::rbd::SnapshotNamespace snap_namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to encode/decode/dump -- since SnapCreateEvent already expects it at version >= 3 && < 6, it should decode it directly. Otherwise, if version >= 6, let the base class decode it (and encode it unconditionally). Also -- bump the version to 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more minor nits (and version encoding/decoding issues) and it should be ready for testing
e04700f
to
4a6ca09
Compare
@VictorDenisov rebase is required |
7b4b5f5
to
d6d1747
Compare
@dillaman ready |
src/librbd/journal/Types.cc
Outdated
boost::apply_visitor(EncodeVisitor(bl), client_meta); | ||
ENCODE_FINISH(bl); | ||
} | ||
|
||
void ClientData::decode(bufferlist::iterator& it) { | ||
DECODE_START(1, it); | ||
DECODE_START(2, it); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this shouldn't change -- it should remain at version 1 to indicate we can successfully still decode version 1 structures
src/librbd/journal/Types.h
Outdated
@@ -194,8 +199,10 @@ struct SnapRenameEvent : public SnapEventBase { | |||
SnapRenameEvent(uint64_t op_tid, uint64_t src_snap_id, | |||
const std::string &src_snap_name, | |||
const std::string &dest_snap_name) | |||
: SnapEventBase(op_tid, dest_snap_name), snap_id(src_snap_id), | |||
src_snap_name(src_snap_name) { | |||
: SnapEventBase(op_tid, cls::rbd::UserSnapshotNamespace(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since SnapRenameEvent
encodes a snap id and source snap name, it can no longer inherit from SnapEventBase
since SnapEventBase
encode function will inject new parameters into the struct between SnaRenameEvent
's parameters.
The essence of this huge change is to have two keys for indexing snap ids in ImageCtx. It used to be a map of (snap_name -> snap_id) now it's (snap_namespace, snap_name) -> snap_id. Therefore now snapshots can have the same name if they are in different namespaces. All the remaining changes are a consequece of this change of ImageCtx.snap_ids field. The only exception is: we assume that you can't rename snapshots from GroupSnapshotNamespaces. So rename operation always assumes UserSnapshotNamespace. Signed-off-by: Victor Denisov <denisovenator@gmail.com>
d6d1747
to
de85e00
Compare
@dillaman i dropped the inheritance from SnapEventBase for SnapRenameEvent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Manually merged to master branch |
librbd: add SnapshotNamespace to ImageCtx Reviewed-by: Jason Dillaman <dillaman@redhat.com>
This PR is for preliminary review. PR #11544 is supposed to be rebased on top of this PR.