Date: Wed, 1 Oct 2014 10:37:33 -0600 From: Will Andrews <will@firepipe.net> To: Davide Italiano <davide@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: svn commit: r272366 - head/sys/kern Message-ID: <CADBaqmh4=4z6ridUBbMtvfSHKkcVer8Mb5bymrdvpdBc55SMbg@mail.gmail.com> In-Reply-To: <CACYV=-F-sKkBW8xu-1tTk7famf9U_jeBV9Y=2gZuQQ6iYhpdPg@mail.gmail.com> References: <201410011532.s91FWTZL050853@svn.freebsd.org> <CACYV=-F-sKkBW8xu-1tTk7famf9U_jeBV9Y=2gZuQQ6iYhpdPg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I think it depends on what kind of safety you're concerned about. There's only one syncer; none of its state can be reclaimed behind its back. If additional work is added to its queue in the interim, it will be processed as if it was already there at the start of the loop. We've been running with this change for 2 years; I don't think any issues have been reported with it. I realize that's not a guarantee that there isn't a problem. However, this change does fix an issue where the system could deadlock on shutdown because it's calling a watchdog callback that mallocs, while holding sync_mtx. --Will. On Wed, Oct 1, 2014 at 9:40 AM, Davide Italiano <davide@freebsd.org> wrote: > On Wed, Oct 1, 2014 at 8:32 AM, Will Andrews <will@freebsd.org> wrote: >> Author: will >> Date: Wed Oct 1 15:32:28 2014 >> New Revision: 272366 >> URL: https://svnweb.freebsd.org/changeset/base/272366 >> >> Log: >> In the syncer, drop the sync mutex while patting the watchdog. >> >> Some watchdog drivers (like ipmi) need to sleep while patting the watchdog. >> See sys/dev/ipmi/ipmi.c:ipmi_wd_event(), which calls malloc(M_WAITOK). >> >> Submitted by: asomers >> MFC after: 1 month >> Sponsored by: Spectra Logic >> MFSpectraBSD: 637548 on 2012/10/04 >> >> Modified: >> head/sys/kern/vfs_subr.c >> >> Modified: head/sys/kern/vfs_subr.c >> ============================================================================== >> --- head/sys/kern/vfs_subr.c Wed Oct 1 15:23:23 2014 (r272365) >> +++ head/sys/kern/vfs_subr.c Wed Oct 1 15:32:28 2014 (r272366) >> @@ -1863,8 +1863,15 @@ sched_sync(void) >> continue; >> } >> >> - if (first_printf == 0) >> + if (first_printf == 0) { >> + /* >> + * Drop the sync mutex, because some watchdog >> + * drivers need to sleep while patting >> + */ >> + mtx_unlock(&sync_mtx); >> wdog_kern_pat(WD_LASTVAL); >> + mtx_lock(&sync_mtx); >> + } >> >> } >> if (syncer_state == SYNCER_FINAL_DELAY && syncer_final_iter > 0) >> > > I introduced something like this back in the days when I was at iX but > never convinced myself this is completely safe -- I assume you > investigated about possible races opened by releasing the syncer > mutex, and I would be happy if you can elaborate a bit more. > > -- > Davide > > "There are no solved problems; there are only problems that are more > or less solved" -- Henri Poincare
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CADBaqmh4=4z6ridUBbMtvfSHKkcVer8Mb5bymrdvpdBc55SMbg>