From owner-freebsd-fs@freebsd.org Fri May 20 03:22:22 2016 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2FA42B43A04 for ; Fri, 20 May 2016 03:22:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 1A74E10B3 for ; Fri, 20 May 2016 03:22:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: by mailman.ysv.freebsd.org (Postfix) id 19AEBB43A03; Fri, 20 May 2016 03:22:22 +0000 (UTC) Delivered-To: fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 170E3B43A02 for ; Fri, 20 May 2016 03:22:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id B6B7110B2 for ; Fri, 20 May 2016 03:22:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c110-21-42-169.carlnfd1.nsw.optusnet.com.au [110.21.42.169]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 9587242AB98; Fri, 20 May 2016 13:22:13 +1000 (AEST) Date: Fri, 20 May 2016 13:22:09 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans cc: Konstantin Belousov , fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs In-Reply-To: <20160520095504.X1527@besplex.bde.org> Message-ID: <20160520120927.V2190@besplex.bde.org> References: <20160517072104.I2137@besplex.bde.org> <20160517084241.GY89104@kib.kiev.ua> <20160518061040.D5948@besplex.bde.org> <20160518070252.F6121@besplex.bde.org> <20160517220055.GF89104@kib.kiev.ua> <20160518084931.T6534@besplex.bde.org> <20160518110834.GJ89104@kib.kiev.ua> <20160519065714.H1393@besplex.bde.org> <20160519094901.O1798@besplex.bde.org> <20160519120557.A2250@besplex.bde.org> <20160519104128.GN89104@kib.kiev.ua> <20160520074427.W1151@besplex.bde.org> <20160520095504.X1527@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=TuMb/2jh c=1 sm=1 tr=0 a=kDyANCGC9fy361NNEb9EQQ==:117 a=kDyANCGC9fy361NNEb9EQQ==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=QWTFWtZIVJHxjm3Yne0A:9 a=CjuIK1q_8ugA:10 X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 May 2016 03:22:22 -0000 PS2: On Fri, 20 May 2016, Bruce Evans wrote: > PS: > > On Fri, 20 May 2016, Bruce Evans wrote: > >> On Thu, 19 May 2016, Konstantin Belousov wrote: >>> diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c >>> index 712fc21..21425f5 100644 >>> --- a/sys/ufs/ffs/ffs_vfsops.c >>> +++ b/sys/ufs/ffs/ffs_vfsops.c >>> @@ -764,24 +764,29 @@ ffs_mountfs(devvp, mp, td) >>> cred = td ? td->td_ucred : NOCRED; >>> ronly = (mp->mnt_flag & MNT_RDONLY) != 0; >>> >>> + KASSERT(devvp->v_type == VCHR, ("reclaimed devvp")); > > I still don't like this. The source code tends to fill up with assertions > (and comments) about simple things. > >>> dev = devvp->v_rdev; >>> dev_ref(dev); >>> + if (!atomic_cmpset_ptr(&dev->si_mountpt, 0, mp)) { > > I used != 0. All file systems need this of course. zfs doesn't use g_vfs_open(), so how can it possibly work to give exclusive access to the device in contention with other mount operations? I think it doesn't even try. Old code used vfs_mountedon() here. vfs_mountedon() was just the above cmp (but not set) in an extern function, with no obvious locking and an a differently bad name for si_mountpt (it was si_mountpoint. The correct name is si_mp). The vnode should be locked, but this was only enough if the old aliasing code gave a unique vnode. Old code also returned EBUSY if vcount(devvp) > 1 && devvp != rootvp. Here rootvp is special to support some old hack involving abusing the swap device for miniroot. This was supposed to have been replaced by g_access() checks in g_vfs_open(), but those aren't exclusive enough, so there is another check in g_vfs_open() but that isn't exclusive enough so we are trying to fix it now. zfs_mount() seems to have no exclusivity check at all, except in the illumos case it has the old vcount() check with rootvp hack (spelled differently as (v_flag & VROOT)). zfs might support multiple mounts, but it can only do that for itself and the vcount() check normally prevents this for the illumos case. vfs_mountedon() is as good an interface as any for checking for exclusive access in a shared way (except it probably can't support multiple mounts like g_vfs_open() is supposed to). It was in 4.4BSD-Lite. 4.4BSD-Lite doesn't have si_mountp[oin]t. It used old alias stuff which is relatively easy to understand for it. It searches the list of aliases and skips ones whose type differs (these are presumably revoked ones). The aliases are vnodes with a common rdev. Each vnode has a V_MOUNTEDON flag. I wonder if current bugs affected this too -- after revoke, the device is still mounted but its vnode is too messed up to show this. vfs_mountedon() in FreeBSD-3 is similar to in 4.4BSD-Lite. In FreeBSD-4, it is an in-between version that looks broken since it doesn't have the alias loop; it depends on vp->v_specmountpoint being the same for all aliases even with only 1 of the aliases locked. Bruce