From owner-freebsd-fs@FreeBSD.ORG Wed Feb 20 14:53:45 2013 Return-Path: Delivered-To: fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id C229D86C for ; Wed, 20 Feb 2013 14:53:45 +0000 (UTC) (envelope-from jamie@FreeBSD.org) Received: from m2.gritton.org (gritton.org [199.192.164.235]) by mx1.freebsd.org (Postfix) with ESMTP id 751BB92E for ; Wed, 20 Feb 2013 14:53:44 +0000 (UTC) Received: from guppy.corp.verio.net (fw.oremut02.us.wh.verio.net [198.65.168.24]) (authenticated bits=0) by m2.gritton.org (8.14.5/8.14.5) with ESMTP id r1KErhdq029382; Wed, 20 Feb 2013 07:53:43 -0700 (MST) (envelope-from jamie@FreeBSD.org) Message-ID: <5124E372.1000009@FreeBSD.org> Date: Wed, 20 Feb 2013 07:53:38 -0700 From: Jamie Gritton User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120126 Thunderbird/9.0 MIME-Version: 1.0 To: Konstantin Belousov Subject: Re: mount/kldload race References: <51244A13.8030907@FreeBSD.org> <20130220054309.GD2598@kib.kiev.ua> In-Reply-To: <20130220054309.GD2598@kib.kiev.ua> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: fs@FreeBSD.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Feb 2013 14:53:45 -0000 On 02/19/13 22:43, Konstantin Belousov wrote: > On Tue, Feb 19, 2013 at 08:59:15PM -0700, Jamie Gritton wrote: >> Perhaps most people don't try to mount a bunch of filesystems at the >> same time, at least not those that depend on kernel modules. But it >> turns out that's going to be a pretty common situation with jails and >> nullfs. And I found that when attempting such a feat will cause most of >> these simultaneous mounts to fail with ENODEV. >> >> It turns out that the problem is a race in vfs_byname_kld(). First it'll >> see if the fstype is loaded, and if it isn't then it will load the >> module. But if the module is loaded by a different process between those >> two points, the resulting EEXIST from kern_kldload() will make >> vfs_byname_kld() error out. >> >> The fix is pretty simple: don't treat EEXIST as an error. By going on, >> and rechecking for the fstype, the filesystem can be mounted while still >> allowing any "real" error to be caught. I'm including a small patch that >> will accomplish this, and I'd appreciate a quick look by anyone who's >> familiar with this part of things before I commit it. >> >> - Jamie > >> Index: sys/kern/vfs_init.c >> =================================================================== >> --- sys/kern/vfs_init.c (revision 247000) >> +++ sys/kern/vfs_init.c (working copy) >> @@ -130,13 +130,18 @@ >> >> /* Try to load the respective module. */ >> *error = kern_kldload(td, fstype,&fileid); >> + if (*error == EEXIST) { >> + *error = 0; >> + fileid = 0; > Why do you clear fileid ? Is this to prevent an attempt to kldunload() > the module which was not loaded by the current thread ? > > If yes, I would suggest to use the separate flag to track this, > which is cleared on EEXIST error. IMHO it is cleaner and less puzzling. Yes, that's why. As a side note, I clear *error ostensibly for the sake of the callers, but it turns out none of the callers actually look at the returned error. Here's a new patch with an added flag: Index: sys/kern/vfs_init.c =================================================================== --- sys/kern/vfs_init.c (revision 247000) +++ sys/kern/vfs_init.c (working copy) @@ -122,7 +122,7 @@ vfs_byname_kld(const char *fstype, struct thread *td, int *error) { struct vfsconf *vfsp; - int fileid; + int fileid, loaded; vfsp = vfs_byname(fstype); if (vfsp != NULL) @@ -130,13 +130,17 @@ /* Try to load the respective module. */ *error = kern_kldload(td, fstype, &fileid); + loaded = (*error == 0); + if (*error == EEXIST) + *error = 0; if (*error) return (NULL); /* Look up again to see if the VFS was loaded. */ vfsp = vfs_byname(fstype); if (vfsp == NULL) { - (void)kern_kldunload(td, fileid, LINKER_UNLOAD_FORCE); + if (loaded) + (void)kern_kldunload(td, fileid, LINKER_UNLOAD_FORCE); *error = ENODEV; return (NULL); }