On Wed, Feb 22, 2012 at 06:53:55PM -0500, Rick Macklem wrote: > John Baldwin wrote: > > On Wednesday, February 22, 2012 2:24:14 pm Konstantin Belousov wrote: > > > On Wed, Feb 22, 2012 at 11:29:40AM -0500, Rick Macklem wrote: > > > > Hiroki Sato wrote: > > > > > Hi, > > > > > > > > > > Just a report, but I got the following panic on an NFS server > > > > > running > > > > > 8.3-PRERELEASE: > > > > > > > > > > ----(from here)---- > > > > > pool.allbsd.org dumped core - see /var/crash/vmcore.0 > > > > > > > > > > Tue Feb 21 10:59:44 JST 2012 > > > > > > > > > > FreeBSD pool.allbsd.org 8.3-PRERELEASE FreeBSD 8.3-PRERELEASE > > > > > #7: Thu > > > > > Feb 16 19:29:19 JST 2012 > > > > > [email protected]:/usr/obj/usr/src/sys/POOL > > > > > amd64 > > > > > > > > > > panic: Assertion lock == sq->sq_lock failed at > > > > > /usr/src/sys/kern/subr_sleepqueue.c:335 > > > > > > > > > Oops, I didn't know that mixing msleep() and tsleep() calls on the > > > > same > > > > event wasn't allowed. > > > > There are two places in the code where it did a: > > > > mtx_unlock(); > > > > tsleep(); > > > > left over from the days when it was written for OpenBSD. > > > This sequence allows to lost the wakeup which is happen right after > > > cache unlock (together with clearing the RC_WANTED flag) but before > > > the thread enters sleep state. The tsleep has a timeout so thread > > > should > > > recover in 10 seconds, but still. > > > > > > Anyway, you should use consistent outer lock for the same wchan, > > > i.e. > > > no lock (tsleep) or mtx (msleep), but not mix them. > > > > Correct. > > > > > > I don't think the mix would actually break anything, except that > > > > the > > > > MPASS() assertion fails, but I've cc'd jhb@ since he seems to have > > > > been > > > > the author of the sleep() stuff. > > > > > > > > Anyhow, please try the attached patch which replaces the > > > > mtx_unlock(); > > tsleep(); with > > > > msleep()s using PDROP. If the attachment gets lost, the patch is > > > > also > > here: > > > > http://people.freebsd.org/~rmacklem/tsleep.patch > > > > > > > > Thanks for reporting this, rick > > > > ps: Is mtx_lock() now preferred over msleep()? > > > What do you mean ? > > > > mtx_sleep() is preferred over msleep(), but I doubt I will remove > > msleep() > > anytime soon. > > > Ok, I'll redo the patch with mtx_sleep() and get one of you guys to > review it. I do not see a need in the changing to mtx_sleep, esp. if other places in nfsd use msleep(). There are more then 570 uses of msleep(9) in the kernel, and undefined number of them in third-party modules.
> > One question. Do you think this is serious enough to worry about for > 8.3? (Just wondering if I need to rush a patch into head with a 1 week > MFC. I realize it would still be up to re@, even if I rush it.) I think it is usual routine bugfix, which is as good to have in release as any other bugfix. 8.3 is in the stabilization period, made exactly for pushing bugfixes.
pgpsgOJ3O3iIR.pgp
Description: PGP signature
