osdc: Fix race condition with tick_event and shutdown #7151
Conversation
| if (tick_event) { | ||
| if (timer.cancel_event(tick_event)) { | ||
| if (tick_event.read()) { | ||
| if (timer.cancel_event(tick_event.read())) { |
There was a problem hiding this comment.
Hmm. We're holding Objecter::rwlock here, and the timer is potentially in the Objecter::tick callback, blocking on acquiring rwlock. If we cancel the event here, that's a no-op because timer has already removed the event from the queue. We'll go ahead and zero it, and eventually give up rwlock. Objecter::tick will acquire rwlock, and immediately assert out on the condition that tick_event is nonzero. Probably the assertion inside Objecter::tick needs to move the other side of the initialized check so that it never gets hit in this case.
There was a problem hiding this comment.
Thank you for catching that. I think it's better just to drop that assert in tick() entirely. It's fragile and doesn't really guard against anything except someone calling 'tick' directly.
- Clear the tick_event whether it was in the timer queue or not.
- Make sure we don't schedule a new tick_event if someone calls shutdown while
tick() is running
- Get rid of some assertions that aren't relevant
Fixes #14256
| @@ -4795,7 +4794,6 @@ Objecter::~Objecter() | |||
| assert(check_latest_map_ops.empty()); | |||
| assert(check_latest_map_commands.empty()); | |||
|
|
|||
| assert(!tick_event); | |||
There was a problem hiding this comment.
if we clear tick_event in tick() and when we cancel both, can't we keep this?
There was a problem hiding this comment.
Arguably yes. I got rid of this since getting rid of the asserts was the suggested way to solve the problem of not having a lock on the class when reading them without making them atomic.
osdc: Fix race condition with tick_event and shutdown Reviewed-by: John Spray <jspray@redhat.com> Reviewed-by: Sage Weil <sage@redhat.com>
tick() is running
a read lock
Fixes #14256