maria-developers team mailing list archive
Mailing list archive
Missing memory barrier in parallel replication error handler in wait_for_prior_commit()?
Hi Andrei (Cc: Sujatha),
I noticed another thing with the wait_for_commit error handling while
looking at the MDEV-18648 patch.
We have code like this:
// Wakeup code in wait_for_commit::wakeup():
// Wait code in wait_for_prior_commit():
So the waiter code runs a "fast path" without locks. It is ok if we race on
the assignment of NULL to wait_for_commit::waitee variable, because then the
waiter will take the slow path and do proper locking.
But it looks like there is a race as follows:
1. wakeup() sets waitee= NULL
2. wait_for_prior_commit() sees waitee==NULL _and_ wakeup_error==0, and
incorrectly returns without error.
3. wakeup() too late sets wait_for_commit::wakeup_error.
It is not enough of course to swap the assignments in wakeup(). A
write-write memory barrier is needed between them in wakeup(), and a
corresponding read-read barrier is needed in wait_for_prior_commit().
With proper barriers, the waiter cannot see the write of waitee=NULL without
also seeing the write to wakeup_error. So it will either return with
non-zero wakeup_error or take the slow path with proper locking. Both of
which are fine.
Andrei, what do you think? Can you see something in the above analysis that
I missed? It seems like such an obvious miss in the code that I wonder how
it remained undetected for so long (or how I could have written that in the
first place...). But I suppose the race is very unlikely to hit in practice,
especially in a code path that only triggers in the error case...