On 2/14/22 15:01, Daniel P. Berrangé wrote:
> On Mon, Feb 14, 2022 at 02:32:55PM +0100, Michal Privoznik wrote:
>> I've rewritten our virMutexes to check for failures [1] and saw couple
>> of problems. In most cases we are destroying a locked mutex which these
>> patches aim to fix. Then we have virLogLock() which is locked in
>> virFork() and then unlocked from child (a different process) which is
>> problematic. Pthread doesn't like it when one thread locks a mutex only
>> so that another one unlocks it. Semaphores don't care.
>
> Despite behaviuor being undefined, in practice it works with any
> impl we have in supported platforms.
>
Indeed, that's why I haven't sent the patch that rewrites virLogMutex.
> The key problem we're addressing is that the child process needs
> to inherit a known state for the mutex. Whether that state is
> locked or unlocked doesn't matter, as long as it has a bulletproof
> guarantee of what that status is.
>
> We call virLogLock before fork() to guarantee a locked state,
> by blocking on any other threads in the parent that might be
> temporarily holding it.
Again, I understand that. It's just that the following code returns an
error:
#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <string.h>
int main(int argc, char *argv[])
{
pthread_mutex_t l;
pthread_mutexattr_t attr;
pthread_mutexattr_init(&attr);
pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
pthread_mutex_init(&l, &attr);
pthread_mutex_lock(&l);
if (fork() == 0) {
/* child */
int rc;
if ((rc = pthread_mutex_unlock(&l)) != 0) {
fprintf(stderr, "unlock error: %s\n", strerror(rc));
abort();
}
}
return 0;
}
Switching to a semaphore works again. I'm not advocating for that
though, it's needless because apparently pthread does the right thing on
all platforms we care about.
>
> The pthread_atfork() handler can be used todo the same trick
> if you don't control the place where fork() is called. The
> man page explicitly says
>
> https://linux.die.net/man/3/pthread_atfork
>
> "The expected usage is that the prepare handler acquires
> all mutex locks and the other two fork handlers release
> them."
>
Which is a terrible advice, let me say. This assumes that there is a
global list of mutexes (which there certainly is not in projects of our
size), but more importantly - this is so fragile to lock ordering than
anything else.
Michal
