From owner-freebsd-fs@FreeBSD.ORG Tue Sep 18 13:34:56 2012 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EDE3D106566B for ; Tue, 18 Sep 2012 13:34:55 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-annu.mail.uoguelph.ca (esa-annu.mail.uoguelph.ca [131.104.91.36]) by mx1.freebsd.org (Postfix) with ESMTP id AB9BA8FC0C for ; Tue, 18 Sep 2012 13:34:55 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Ap8EAEp3WFCDaFvO/2dsb2JhbAA+BxaFcbc0giABAQUjBFIbDgoCAg0ZAlkGiBMLpxuTFIEhigAhhTWBEgOVYoEUjw2DAoE+Ihs X-IronPort-AV: E=Sophos;i="4.80,443,1344225600"; d="scan'208";a="182105131" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-annu-pri.mail.uoguelph.ca with ESMTP; 18 Sep 2012 09:34:54 -0400 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 5C44DB4017; Tue, 18 Sep 2012 09:34:54 -0400 (EDT) Date: Tue, 18 Sep 2012 09:34:54 -0400 (EDT) From: Rick Macklem To: Konstantin Belousov Message-ID: <21418398.765673.1347975294365.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20120918085941.GZ37286@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.202] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) Cc: FS List Subject: Re: testing/review of atomic export update patch X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Sep 2012 13:34:56 -0000 Konstantin Belousov wrote: > On Mon, Sep 17, 2012 at 05:32:44PM -0400, Rick Macklem wrote: > > Konstantin Belousov wrote: > > > On Sun, Sep 16, 2012 at 05:41:25PM -0400, Rick Macklem wrote: > > > > Hi, > > > > > > > > There is a simple patch at: > > > > http://people.freebsd.org/~rmacklem/atomic-export.patch > > > > that can be applied to a kernel + mountd, so that the new > > > > nfsd can be suspended by mountd while the exports are being > > > > reloaded. It adds a new "-S" flag to mountd to enable this. > > > > (This avoids the long standing bug where clients receive ESTALE > > > > replies to RPCs while mountd is reloading exports.) > > > > > > This looks simple, but also somewhat worrisome. What would happen > > > if the mountd crashes after nfsd suspension is requested, but > > > before > > > resume was performed ? > > > > > > Might be, mountd should check for suspended nfsd on start and > > > unsuspend > > > it, if some flag is specified ? > > Well, I think that happens with the patch as it stands. > > > > suspend is done if the "-S" option is specified, but that is a no op > > if it is already suspended. The resume is done no matter what flags > > are provided, so mountd will always try and do a "resume". > > --> get_exportlist() is always called when mountd is started up and > > it does the resume unconditionally when it completes. > > If mountd repeatedly crashes before completing get_exportlist() > > when it is started up, the exports will be all messed up, so > > having the nfsd threads suspended doesn't seem so bad for this > > case (which hopefully never happens;-). > > > > Both suspend and resume are just no ops for unpatched kernels. > > > > Maybe the comment in front of "resume" should explicitly explain > > this, instead of saying resume is harmless to do under all > > conditions? > > > > Thanks for looking at it, rick > I see. > > My another note is that there is no any protection against parallel > instances of suspend/resume happen. For instance, one thread could set > suspend_nfsd = 1 and be descheduled, while another executes resume > code sequence meantime. Then it would see suspend_nfsd != 0, while > nfsv4rootfs_lock not held, and tries to unlock it. It seems that > nfsv4_unlock would silently exit. The suspending thread resumes, > and obtains the lock. You end up with suspend_nfsd == 0 but lock held. Yes. I had assumed that mountd would be the only thing using these syscalls and it is single threaded. (The syscalls can only be done by root for the obvious reasons.;-) Maybe the following untested version of the syscalls would be better, since they would allow multiple concurrent calls to either suspend or resume. (There would still be an indeterminate case if one thread called resume concurrently with another few calling suspend, but that is unavoidable, I think?) Again, thanks for the comments, rick --- untested version of syscalls --- } else if ((uap->flag & NFSSVC_SUSPENDNFSD) != 0) { NFSLOCKV4ROOTMUTEX(); if (suspend_nfsd == 0) { /* Lock out all nfsd threads */ igotlock = 0; while (igotlock == 0 && suspend_nfsd == 0) { igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL, NFSV4ROOTLOCKMUTEXPTR, NULL); } suspend_nfsd = 1; } NFSUNLOCKV4ROOTMUTEX(); error = 0; } else if ((uap->flag & NFSSVC_RESUMENFSD) != 0) { NFSLOCKV4ROOTMUTEX(); if (suspend_nfsd != 0) { nfsv4_unlock(&nfsv4rootfs_lock, 0); suspend_nfsd = 0; } NFSUNLOCKV4ROOTMUTEX(); error = 0; }