I can't really comment on the bug itself or the necessity of changing the locking scheme. About the proposed patch: First: that hunk in core-util is formatted very confusingly, but I guess that is just the result of diff being stupid there. If the right solution is to add pa_read_lock_fd, the I think it should be defined unconditionally (i.e. not protected by HAVE_READ_LOCKS) and the implementation should fall back to pa_lock_fd for a write lock if F_SETLKW is not defined, so that the stuff on Windows keeps working (I'll fix it up properly to use a read lock later) Also, the semantics of promoting a read lock to a write lock need to be defined in a comment in core-util.c. From the patch it looks like a promoted lock needs to be unlocked only once, but at least on Windows two unlock operations are needed. Maarten 2011/8/19 David Henningsson <david.henningsson at canonical.com>: > Hi, > > I came across this bug: > > https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/817269 > > Which claims the following (and also offers a patch [1] to fix it). I don't > know much about file locking or NFS, but maybe someone here knows more than > I here. > > Quoting the bug: > > Pulseaudio uses a cookie file (normally ~/.pulse-cookie). This file is > manipulated by the code in src/pulsecore/authkey.c. Currently it does this > to read the key: > 1) Open the file > 2) Acquire a write-lock for the file. > 3) Read the file. > 4) If the file is not a good cookie, generate a new cookie and write it to > the file. > 5) Close the file. > > If more than one thread is opening the cookie at once, then there is > contention at step (2) which is rarely necessary. Instead, step (2) should > acquire only a read-lock, and then step (4) should promote that to a > write-lock before writing the file. > > This is particularly bad in the case where the .pulse-cookie file is on NFS. > In that case, the lock contention forces a thirty-second backoff from > whichever thread gets there second. > > I am happy to work with y'all in engineering a patch for this. It requires > obviously a change to src/pulsecore/authkey.c, and a corresponding one in > src/pulsecore/core-util.c where the actual lock syscall occurs. > > The following script (if ~ is in NFS) will demonstrate the problem. The two > pactl processes both try to acquire the lock on the file simultaneously, and > whichever one loses will take thirty seconds before it wins, because of the > needless contention (and the unfortunate facts of how lockd in NFS works). > > !/bin/bash > > NUM_PROCS=2 > > procs=() > for i in $(seq 1 $NUM_PROCS); do > ?pactl list > /dev/null & > ?procs[$i]=$! > done > for i in $(seq 1 $NUM_PROCS); do > ?wait ${procs[$i]} > done > > > -- > David Henningsson, Canonical Ltd. > http://launchpad.net/~diwic > > [1] https://launchpadlibrarian.net/77550336/use-read-locks > _______________________________________________ > pulseaudio-discuss mailing list > pulseaudio-discuss at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss >
