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: fix break inside of yield in RGWFetchAllMetaCR #11586

Merged
merged 1 commit into from Mar 10, 2017

Conversation

Projects
None yet
2 participants
@cbodley
Contributor

cbodley commented Oct 20, 2016

the yield macro is implemented with for/switch, so the breaks in
RGWFetchAllMetaCR weren't being applied to the for loop as expected -
so any of these breaks send RGWFetchAllMetaCR into an infinite loop

changed the breaks into returns, so the next call to operate() will
continue with the next iteration of the loop

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

(exposed by unit tests in #11585)

@@ -840,7 +840,7 @@ class RGWFetchAllMetaCR : public RGWCoroutine {
yield {
if (!lease_cr->is_locked()) {
lost_lock = true;
break;
return 0;

This comment has been minimized.

@yehudasa

yehudasa Oct 21, 2016

Member

@cbodley is this the correct handling? this isn't going to take us out of the loop, and that was the original intention here.

This comment has been minimized.

@cbodley

cbodley Oct 21, 2016

Contributor

the yield block currently covers the entire scope of the for loop, so i believe these returns are doing what we want

that said, i'm not actually sure this yield is necessary in the first place - can you confirm?

This comment has been minimized.

@yehudasa

yehudasa Oct 21, 2016

Member

it's needed around entries_index->append()

This comment has been minimized.

@cbodley

cbodley Nov 29, 2016

Contributor

will try replacing the yield {} block with a single yield; statement - then the breaks will apply to the for loop as intended

@ghost

This comment has been minimized.

ghost commented Nov 17, 2016

jenkins test this please (no log)

@ghost

This comment has been minimized.

ghost commented Nov 17, 2016

jenkins test this please (jenkins failure)

@ghost

This comment has been minimized.

ghost commented Nov 18, 2016

jenkins test this please (eio now ignored in master)

rgw: fix break inside of yield in RGWFetchAllMetaCR
the yield macro is implemented with for/switch, so the breaks in
RGWFetchAllMetaCR weren't being applied to the for loop as expected -
so any of these breaks send RGWFetchAllMetaCR into an infinite loop

removed the yield {} block, so that breaks will apply to the for loop as
intended, then added a single yield; statement to allow the
entries_index consumer to run one per iteration

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

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 29, 2016

@yehudasa removed the yield {} block and added a single yield; statement once per iteration of the loop (i put the yield before the local variables so it wouldn't complain about jumping across their declarations)

i rebased the unit tests from #11585 for testing - without this pr, they hit an infinite loop - with this pr, they pass

@yehudasa

lgtm

@yehudasa

This comment has been minimized.

Member

yehudasa commented Nov 29, 2016

@cbodley should also test the multisite suite

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 30, 2016

passes test_multi.py (requires fixes from #12229 and #12105)

@yehudasa

This comment has been minimized.

Member

yehudasa commented Jan 16, 2017

@cbodley need to get this one through teuthology too

@cbodley

This comment has been minimized.

@yehudasa yehudasa merged commit 19f8f77 into ceph:master Mar 10, 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

@cbodley cbodley deleted the cbodley:wip-rgw-fetchallmeta-yield-break branch Mar 10, 2017

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