From owner-freebsd-fs@freebsd.org Sat May 21 01:48:23 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 91E78B430AF for ; Sat, 21 May 2016 01:48:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 7CA4E1739 for ; Sat, 21 May 2016 01:48:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: by mailman.ysv.freebsd.org (Postfix) id 78465B430AE; Sat, 21 May 2016 01:48:23 +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 77ED7B430AD for ; Sat, 21 May 2016 01:48:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 278E01738 for ; Sat, 21 May 2016 01:48:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id E7CAE784D81; Sat, 21 May 2016 11:48:20 +1000 (AEST) Date: Sat, 21 May 2016 11:48:20 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Bruce Evans , fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs In-Reply-To: <20160520153649.GX89104@kib.kiev.ua> Message-ID: <20160521111424.T1652@besplex.bde.org> References: <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> <20160520092348.GV89104@kib.kiev.ua> <20160520194427.W1170@besplex.bde.org> <20160520142654.GW89104@kib.kiev.ua> <20160520153649.GX89104@kib.kiev.ua> 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=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=WlGRK_V-j6VVmBx-EXEA: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: Sat, 21 May 2016 01:48:23 -0000 On Fri, 20 May 2016, Konstantin Belousov wrote: [This is the version with uintptr_t casts] > On Fri, May 20, 2016 at 05:26:54PM +0300, Konstantin Belousov wrote: >> On Fri, May 20, 2016 at 09:22:08PM +1000, Bruce Evans wrote: >>> On Fri, 20 May 2016, Konstantin Belousov wrote: >>> >>>> On Fri, May 20, 2016 at 09:27:38AM +1000, Bruce Evans 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")); >>>>>> dev = devvp->v_rdev; >>>>>> dev_ref(dev); >>>>>> + if (!atomic_cmpset_ptr(&dev->si_mountpt, 0, mp)) { >>>>>> + dev_rel(dev); >>>>>> + VOP_UNLOCK(devvp, 0); >>>>>> + return (EBUSY); >>>>>> + } >[*] >>> The atomic cmpset now orders things too. Is that enough? It ensures >>> that an old mount cannot be active. I don't know if v_bufobj is used >>> for non-mounts. >> v_bufobj is logically protected against modifications by the vnode lock. I meant "is it enough if we drop the vnode lock earlier". Also, what protects fro later accesses and modifications? >>>> diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c >>>> index 712fc21..670bb15 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")); >>> >>> Hrmph. >> I want this, it would remove amount of obvious questions. But it gives negatively useful runtime checking... >>>> dev = devvp->v_rdev; >>>> dev_ref(dev); ...this gives better runtime checking. It gives a nice restartable null pointer trap except in the INVARIANTS case the KASSERT() gives a non- restartable panic. >> cmpset__ptr() on i386 has cast for old and new parameters to u_int. >> store_rel_ptr() on i386 does not cast value to u_int. As result, NULL >> is acceptable for cmpset, but not for store. I spelled it 0 in all cases. >> >> Hm, I also should add uintptr_t cast for cmpset, otherwise, I suspect, >> some arch might be broken. > Even more casts are needed, updated patch is below. These are necessary, unfortunately. Perhaps 32-bit arches need the bogus casts more because of the difference in pointer sizes. The caller might have a long variable and expect this to work the same as an int variable on 32-bit arches because the sizes are the same, but compilers should detect this type mismatch (for pointers to these types). On 64-bit arches, callers must be more careful and use only long variables. There are also signedness problems. Plain int and plain long shouldn't work since only unsigned variables are supported. Compilers should detect this too, but traditionally they were sloppier about this, and we apparently don't usually enable the warning flag for this. > diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c > index 712fc21..0487c2f 100644 > --- a/sys/ufs/ffs/ffs_vfsops.c > +++ b/sys/ufs/ffs/ffs_vfsops.c > @@ -764,25 +764,31 @@ 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")); Hrmph. > dev = devvp->v_rdev; > - dev_ref(dev); > + if (atomic_cmpset_acq_ptr((uintptr_t *)&dev->si_mountpt, 0, > + (uintptr_t)mp) != 0) { > + VOP_UNLOCK(devvp, 0); > + return (EBUSY); > + } > DROP_GIANT(); > g_topology_lock(); > error = g_vfs_open(devvp, &cp, "ffs", ronly ? 0 : 1); > g_topology_unlock(); > PICKUP_GIANT(); > + if (error != 0) { > + VOP_UNLOCK(devvp, 0); > + atomic_store_rel_ptr((uintptr_t *)&dev->si_mountpt, 0); The store must be before the unlock (since we don't hold a reference to the dev yet, the dev may go away on revoke after unlock). Otherwise OK. > + return (error); > + } > + dev_ref(dev); Bruce