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: lock is not released when set sync marker is failed. #12197

Merged
merged 1 commit into from Jul 13, 2017

Conversation

Projects
None yet
4 participants
@zhangsw
Contributor

zhangsw commented Nov 28, 2016

Signed-off-by: Zhang Shaowen zhangshaowen@cmss.chinamobile.com

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 28, 2016

the fix looks correct, but i'd prefer not to include the whitespace changes

@zhangsw

This comment has been minimized.

Contributor

zhangsw commented Nov 29, 2016

@cbodley done.

@cbodley

I ran some tests with the following code to inject errors, and it's leading to an unexpected segfault.

+#if 1
+          retcode = -ENOENT; // trigger early return
+#else
          using WriteMarkerCR = RGWSimpleRadosWriteCR<rgw_meta_sync_marker>;
          yield call(new WriteMarkerCR(sync_env->async_rados, sync_env->store,
                                       pool, sync_env->shard_obj_name(shard_id),
                                       sync_marker));
+#endif

When RGWMetaSyncShardCR::full_sync() yields for the new yield lease_cr->go_down();, it has already set sync_marker.state = rgw_meta_sync_marker::IncrementalSync. This means that when RGWMetaSyncShardCR::operate() resumes, it calls into RGWMetaSyncShardCR::incremental_sync() instead - so nothing after the yield lease_cr->go_down(); gets executed. (without this error injection, the same is true for the yield call(new WriteMarkerCR(...));)

I think we'll want to update and write a temporary sync marker here, and only apply the changes to our sync_marker member variable once full_sync() returns successfully. I'll introduce a separate PR to do that.

In the meantime, this PR can merge as is.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 29, 2016

opened #12223 for the related fix

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 29, 2016

I created a tracker issue for this so we can get it backported to jewel - can you please add this line to your commit message?

Fixes: http://tracker.ceph.com/issues/18077
rgw: lock is not released when set sync marker is failed.
Fixes: http://tracker.ceph.com/issues/18077

Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
@zhangsw

This comment has been minimized.

Contributor

zhangsw commented Nov 30, 2016

@cbodley your analysis is pretty right~I have added that line to my commit message.

@ghost

This comment has been minimized.

ghost commented Dec 2, 2016

jenkins test this please (https://jenkins.ceph.com/job/ceph-pull-requests/15086/ has readable.sh error)

@yuriw

This comment has been minimized.

@yehudasa yehudasa merged commit 790f913 into ceph:master Jul 13, 2017

3 of 4 checks passed

make check (arm64) Build triggered. sha1 is merged.
Details
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