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

rgw:multisite: fix RGWRadosRemoveOmapKeysCR and change cn to intrusive_ptr #16197

Merged
merged 2 commits into from Jul 12, 2017

Conversation

Projects
None yet
4 participants
@shashalu
Contributor

shashalu commented Jul 7, 2017

RGWRadosRemoveOmapKeysCR::request_complete return val is wrong. The return val should get from completion. Some member variables is not used, clear up those variables.
change raw 'RGWAioCompletionNotifier*' to intrusive_ptr for rgw_cr_rados

Fixes:http://tracker.ceph.com/issues/20539

Signed-off-by: Shasha Lu lu.shasha@eisoo.com

@mattbenjamin mattbenjamin requested a review from cbodley Jul 7, 2017

@mattbenjamin mattbenjamin added bug fix rgw cleanup and removed bug fix labels Jul 7, 2017

@cbodley

cbodley approved these changes Jul 7, 2017

@shashalu

This comment has been minimized.

Contributor

shashalu commented Jul 10, 2017

@cbodley @yuriw Sorry, forget cn->get(); in send_request, which will result in crash in request_complete. Revised it.

@@ -283,6 +283,7 @@ int RGWRadosGetOmapKeysCR::send_request() {
op.omap_get_vals2(marker, max_entries, entries, nullptr, &rval);
cn = stack->create_completion_notifier();
cn->get();

This comment has been minimized.

@cbodley

cbodley Jul 10, 2017

Contributor

was this meant to go in RGWRadosRemoveOmapKeysCR::send_request()?

it looks like this reference on cn is missing from a lot of these coroutines. could you please go through and change these raw RGWAioCompletionNotifier* pointers to boost::intrusive_ptr<RGWAioCompletionNotifier>? see RGWRadosRemoveCR as an example

@yuriw

This comment has been minimized.

rgw:multisite: fix RGWRadosRemoveOmapKeysCR
RGWRadosRemoveOmapKeysCR::request_complete return val is wrong. The return val should get from completion.  Some member variables is not used, clear up those variables.

Fixes:http://tracker.ceph.com/issues/20539

Signed-off-by: Shasha Lu <lu.shasha@eisoo.com>

@shashalu shashalu changed the title from rgw:multisite: fix RGWRadosRemoveOmapKeysCR to rgw:multisite: fix RGWRadosRemoveOmapKeysCR and change cn to intrusive_ptr Jul 11, 2017

@shashalu

This comment has been minimized.

Contributor

shashalu commented Jul 11, 2017

@cbodley already revised.

@@ -222,9 +222,6 @@ RGWRadosSetOmapKeysCR::RGWRadosSetOmapKeysCR(RGWRados *_store,
RGWRadosSetOmapKeysCR::~RGWRadosSetOmapKeysCR()
{
if (cn) {
cn->put();
}

This comment has been minimized.

@cbodley

cbodley Jul 11, 2017

Contributor

thanks. can you remove these destructors now that they're empty? (~RGWRadosTimelogAddCR() and ~RGWRadosTimelogTrimCR() also)

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jul 11, 2017

tested the version with intrusive_ptr under valgrind and found no leaks 👍

rgw:multisite: change raw 'RGWAioCompletionNotifier*' to intrusive_pt…
…r for rgw_cr_rados

Signed-off-by: Shasha Lu <lu.shasha@eisoo.com>
@shashalu

This comment has been minimized.

Contributor

shashalu commented Jul 12, 2017

@cbodley updated

@cbodley cbodley merged commit 86d985f into ceph:master Jul 12, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@shashalu shashalu deleted the shashalu:fix-RGWRadosRemoveOmapKeysCR branch Jul 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment