Skip to content
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

common/LogClient: assign seq and queue atomically #16828

Merged
merged 3 commits into from Aug 9, 2017

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Aug 4, 2017

@gregsfortytwo
Copy link
Member

jenkins test this please

gregsfortytwo
gregsfortytwo previously approved these changes Aug 4, 2017
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@gregsfortytwo gregsfortytwo dismissed their stale review August 4, 2017 20:40

We should make last_log a non-atomic

Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last_log is an atomic variable, but we're only accessing it under the mutex and it's clearly not really safe to grab them without that so we should turn it back into a normal int.

The rest of this looks good.

Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coolio

@gregsfortytwo
Copy link
Member

jenkins test this please

@liewegas
Copy link
Member Author

liewegas commented Aug 6, 2017

retest this please

@gregsfortytwo
Copy link
Member

retest this please

The _get_mon_log_message() assumes that log_last and log_queue
are in sync, but it was previously possible to increment log_last
setting e.seq in do_log(), and only later queue it.  If a racing
thread ran get_mon_log_message() in the meantime it would fail
an assertion.

Fix by assigning the seq and queueing it atomically.  If the
cluster log is not enabled, use the get_next_seq() helper so that
graylog or syslog messages still have a seq assigned.

Fixes: http://tracker.ceph.com/issues/18209
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

liewegas commented Aug 9, 2017

@liewegas liewegas deleted the wip-18209 branch August 9, 2017 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants