From owner-svn-src-head@FreeBSD.ORG Wed Oct 1 16:37:40 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A30B5ADC for ; Wed, 1 Oct 2014 16:37:40 +0000 (UTC) Received: from mail-qg0-f41.google.com (mail-qg0-f41.google.com [209.85.192.41]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 63641196 for ; Wed, 1 Oct 2014 16:37:40 +0000 (UTC) Received: by mail-qg0-f41.google.com with SMTP id f51so590388qge.0 for ; Wed, 01 Oct 2014 09:37:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=3hKavqNqkQxN14T8665jX4DBKFHL2mrMdWN+jF4eKk8=; b=IB5DF7N9JQF2nbIWfyX/7NaG9/Sf+36vVgRKfWws1jq94crf1eTbGzCbhejuJxKZ/U YgKOdiOpfKZ2t4SFtIrKkEs+uYJN8U3yECRVEH8GhqMJ1r5HQv1TywRvIdT0ZygtLXWH Ss/lc3osWXjDG+VH8RCV1IydZeeWLpBzZP5aN12y6EOj88PnDLSDd7VU7OlK8Vol0MYO wNv2nf1W2P9KfxFW34KLYknU+6fnr4Ib2Hf27Z/Ab8T9hOUmreZzC6XzstW77o4l+8Oi qxPG0so0xE6gH3azaxAQxJGl0eNvGmHmwALkVYRlz6Yu0zkQxnLDivqIX1NZ1/iBwZFK ck/w== X-Gm-Message-State: ALoCoQlE7oZaUlD65v4pAY6rGehRIMQAUx9XggDP5k75J/BIyDFKA5NqG+gutyzlJvFgIijmrtz3 MIME-Version: 1.0 X-Received: by 10.224.32.138 with SMTP id c10mr64383266qad.1.1412181453110; Wed, 01 Oct 2014 09:37:33 -0700 (PDT) Received: by 10.140.31.36 with HTTP; Wed, 1 Oct 2014 09:37:33 -0700 (PDT) In-Reply-To: References: <201410011532.s91FWTZL050853@svn.freebsd.org> Date: Wed, 1 Oct 2014 10:37:33 -0600 Message-ID: Subject: Re: svn commit: r272366 - head/sys/kern From: Will Andrews To: Davide Italiano Content-Type: text/plain; charset=UTF-8 Cc: "svn-src-head@freebsd.org" , "svn-src-all@freebsd.org" , "src-committers@freebsd.org" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Oct 2014 16:37:40 -0000 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 wrote: > On Wed, Oct 1, 2014 at 8:32 AM, Will Andrews 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