From owner-svn-src-all@FreeBSD.ORG Wed Oct 1 16:37:35 2014 Return-Path: Delivered-To: svn-src-all@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 E3252A95 for ; Wed, 1 Oct 2014 16:37:34 +0000 (UTC) Received: from mail-qc0-f176.google.com (mail-qc0-f176.google.com [209.85.216.176]) (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 A0308192 for ; Wed, 1 Oct 2014 16:37:34 +0000 (UTC) Received: by mail-qc0-f176.google.com with SMTP id r5so625000qcx.35 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=MNeHzcJ+9nzfqvYr75Hj058WLa2GlHe9rlduV4Ud19YGnMjcw6aiTAhNhWSXvxc2qa ccz0wrLaiE3Of1MV5WdaIeZH22RvEmstspWadPlQdgUlbMCvEtsKnFwDhz0A/NIZGslb 9x+PEh8p8NLUI7wP5JxGf/ZCTe7AxobTttZZzVYxonaKlce9L3HRayFTkKsx08CBmB+0 2YFOC108lyLQZCrSOm4RNAzculQPQNoAmVqfJ0czX5OPA9YOwlbP2nr72tKvjuq91zkb miu2rD+/LIDQYBlJPRGpQwuhVg6KdscbxYkX6w6tIE7yjxEQ9FyS3p6PzvwwG2MFdOU8 eRpg== X-Gm-Message-State: ALoCoQkdq/oBETW0sIcoW+bf8/L3XYbYBpNsQ5ZE/1UNOlG5ZGmbvgdSYWbE1gLsPLApuFAlYHKj 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-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Oct 2014 16:37:35 -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