-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Wip rgw sync fixes 3 #8170
Wip rgw sync fixes 3 #8170
Conversation
all_reqs.insert(iter.second); /* can't call _finish_request() on iter.second, it will modify reqs */ | ||
} | ||
for (auto iter : all_reqs) { | ||
_finish_request(iter, -ECANCELED); |
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.
good catch! i'll pull this into my testing branch
though it might be simpler to do something like this:
while (!reqs.empty()) {
_finish_request(reqs.begin()->second, -ECANCELED);
}
alternatively, you could move reqs into a temporary and loop over that:
auto all_reqs = std::move(reqs); // prevent _finish_request() from removing reqs during the loop
for (auto iter : all_reqs) {
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.
@cbodley yeah, seems more elegant. I wouldn't go for the while (!reqs.empty())
solutions, because it relies on _finish_request() to do the right thing (which doesn't remove it directly, but calls _complete_request()).
With these fixes, I've been unable to reproduce the segfaults and multisite tests are passing consistently. |
4fa4f0b
to
60429c3
Compare
It doesn't make sense to update it earlier, the zone that follows will not sync before it sees the completion. Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
can't call _finish_request() on iter->second when iterating, it clobbers the map. Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
and drop the cr when worker is done. The cr can be cleaned before worker is done, so we should have it stick around as long as the worker is alive. Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
@cbodley reworked commit following your comment, added another fix to a separate issue |
lgtm |
Wip rgw sync fixes 3 Reviewed-by: Casey Bodley <cbodley@redhat.com>
No description provided.