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

rbd, librbd: migrate atomic_t to std::atomic (rodan) #14656

Merged
merged 3 commits into from May 8, 2017

Conversation

Projects
None yet
4 participants
@chardan
Contributor

chardan commented Apr 20, 2017

refer-to: #14655

@@ -600,8 +600,89 @@ void Replayer::handle_update(const std::string &mirror_uuid,
{{mirror_uuid, image_id.id}},
gather_ctx->new_sub());
}
}
void Replayer::start_image_replayer(unique_ptr<ImageReplayer<> > &image_replayer)

This comment has been minimized.

@chardan

chardan Apr 20, 2017

Contributor

hmm... did I inadvertently merge this in somehow, or is it supposed to be here?

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 25, 2017

looks like this is based on top of something other than master

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 27, 2017

Jenkins re-test this please

@cbodley

This comment has been minimized.

Contributor

cbodley commented Apr 27, 2017

the rgw bits look pretty good, i'll get this into a testing branch for the rgw suite

i was a bit confused by the commits called migrate std::atomic<unsigned> to std::atomic<bool>, because their changes are from atomic_t -> atomic<bool>. and there's still some inconsistency between atomic<bool> and atomic<unsigned> for boolean flags. could we stick with atomic<bool> for those?

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 27, 2017

@cbodley I am sorry for the inconsistency-- I had originally done this whole thing as a single giant change and then made some separate commits for things like the flags. Unfortunately, I didn't base my changes on master and things became a bit confused during all the rebasing and shuffling, partly because stuff stopped compiling and I finally wound up just changing my mind on doing that as a separate change since it was causing me to also get confused.

I agree that ultimately since I'm already in here fiddling with the types, it makes sense to just move stuff used as a Boolean flag to type atomic. In the "final" PR (gojira), I lost a commit and had to do a large change that does exactly that.

I'm hoping to avoid it, but if this stuff is altogether still too confusing I can try to rework the commits again. Another thought is that I could perhaps take that single commit out of this PR and make sure it's in "gojira".

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 27, 2017

@cbodley @liewegas: Another idea is that I could gather the RGW stuff here and from gojira and put them into their own PR. We're not out of kaiju names, yet... ;-)

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 27, 2017

@cbodley Ok, new PR in the series, "ebirah": #14839

@chardan chardan changed the title from migrate atomic_t to std::atomic (rodan) to rbd, librbd: migrate atomic_t to std::atomic (rodan) Apr 28, 2017

@chardan

This comment has been minimized.

Contributor

chardan commented Apr 28, 2017

Jenkins re-test this please

@dillaman

This comment has been minimized.

Contributor

dillaman commented May 1, 2017

@chardan These commits seem to be intermingled where the changes don't reflect the commit message (i.e. changing RGW things in a commit re: RBD tools).

@@ -57,7 +57,7 @@ class RGWAsyncRadosRequest : public RefCountedObject {
class RGWAsyncRadosProcessor {
deque<RGWAsyncRadosRequest *> m_req_queue;
atomic_t going_down;
std::atomic<unsigned> going_down = { 0 };

This comment has been minimized.

@cbodley

cbodley May 4, 2017

Contributor

please rebase - this change was already merged in #14839

@@ -56,10 +56,10 @@ struct RGWLoadGenRequest : public RGWRequest {
string method;
string resource;
int content_length;
atomic_t* fail_flag;
std::atomic<bool>* fail_flag = { nullptr };

This comment has been minimized.

@cbodley

cbodley May 4, 2017

Contributor

please rebase - these changes were already merged in #14839

@chardan

This comment has been minimized.

Contributor

chardan commented May 4, 2017

Jenkins retest please

@@ -60,7 +60,6 @@ struct RGWLoadGenRequest : public RGWRequest {
string resource;
int content_length;
std::atomic<int64_t>* fail_flag = nullptr;

This comment has been minimized.

@dillaman

dillaman May 4, 2017

Contributor

Nit: yank from this PR as it's unnecessary and unrelated

This comment has been minimized.

@dillaman

dillaman May 8, 2017

Contributor

... still needs to be addressed

@@ -59,9 +59,10 @@ struct RGWLoadGenRequest : public RGWRequest {
string method;
string resource;
int content_length;
std::atomic<int64_t>* fail_flag = nullptr;
std::atomic<bool>* fail_flag = { nullptr };

This comment has been minimized.

@dillaman

dillaman May 4, 2017

Contributor

Nit: yank from this PR as it's unrelated

This comment has been minimized.

@chardan

chardan May 4, 2017

Contributor

In other conversation, it was agreed that we might as well convert "flags" to atomic while we're in here.

This comment has been minimized.

@dillaman

dillaman May 4, 2017

Contributor

@chardan This PR is for RBD-related stuff but this change is RGW. It would be easier to just open a new PR against RGW instead of having two different groups of reviewers on the same PR.

@@ -16,12 +22,6 @@
#include "ImageDeleter.h"
#include "types.h"
#include <set>

This comment has been minimized.

@dillaman

dillaman May 4, 2017

Contributor

Why?

This comment has been minimized.

@chardan

chardan May 4, 2017

Contributor

I don't see this in my local clone... it should now be whatever is in master.

This comment has been minimized.

@dillaman

dillaman May 4, 2017

Contributor

@chardan If it's already in master, it shouldn't show up as a change in this PR. Make sure you have properly rebased your branch.

@dillaman

NAKing PR until comments are addressed

@chardan

This comment has been minimized.

Contributor

chardan commented May 8, 2017

@dillaman Ok, this PR should now be rbd* only. New PR for rgw coming. Thank you!

@@ -60,7 +60,6 @@ struct RGWLoadGenRequest : public RGWRequest {
string resource;
int content_length;
std::atomic<int64_t>* fail_flag = nullptr;

This comment has been minimized.

@dillaman

dillaman May 8, 2017

Contributor

... still needs to be addressed

@@ -297,7 +297,7 @@ class Journal {
uint64_t m_event_tid;
Events m_events;
atomic_t m_op_tid;
std::atomic<unsigned> m_op_tid = { 0 };

This comment has been minimized.

@dillaman

dillaman May 8, 2017

Contributor

Nit: this should be in the subsequent commit for librbd

This comment has been minimized.

@dillaman

dillaman May 8, 2017

Contributor

Needs to be unint64_t -- not just unsigned (int)

chardan added some commits Apr 24, 2017

tools (rbd*): migrate atomic_t to std::atomic<>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
librbd: migrate atomic_t to std::atomic<>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
rbd_mirror: migrate std::atomic<unsigned> to std::atomic<bool>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@chardan

This comment has been minimized.

Contributor

chardan commented May 8, 2017

Jenkins retest this please.

@dillaman

lgtm

@dillaman dillaman merged commit a5c36ce into ceph:master May 8, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment