From owner-freebsd-fs@freebsd.org Fri May 20 00:27:12 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 5DF15B415A1 for ; Fri, 20 May 2016 00:27:12 +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 48E3517E6 for ; Fri, 20 May 2016 00:27:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: by mailman.ysv.freebsd.org (Postfix) id 482FCB4159F; Fri, 20 May 2016 00:27:12 +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 47D62B4159E for ; Fri, 20 May 2016 00:27:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id F33AD17E5 for ; Fri, 20 May 2016 00:27:11 +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 mail109.syd.optusnet.com.au (Postfix) with ESMTPS id 36948D6F26C; Fri, 20 May 2016 10:11:39 +1000 (AEST) Date: Fri, 20 May 2016 10:11:38 +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: <20160520074427.W1151@besplex.bde.org> Message-ID: <20160520095504.X1527@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> 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=c+ZWOkJl 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=_iGiZFj0rCtMW9D2ktwA: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 00:27:12 -0000 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. >> @@ -1083,8 +1088,6 @@ ffs_mountfs(devvp, mp, td) >> out: >> if (bp) >> brelse(bp); >> - if (devvp->v_type == VCHR && devvp->v_rdev != NULL) >> - devvp->v_rdev->si_mountpt = NULL; >> if (cp != NULL) { >> DROP_GIANT(); >> g_topology_lock(); >> @@ -1102,6 +1105,7 @@ out: >> free(ump, M_UFSMNT); >> mp->mnt_data = NULL; >> } >> + dev->si_mountpt = NULL; > > This should remain before the vnode unlock. Otherwise a new mount can > fail unnecessarily. > >> dev_rel(dev); >> return (error); >> } Oops. The vnode lock is not held here, so the order in ffs_mount() cannot be duplicated. I moved the resetting of si_mountpt down to here too. Not locking here and elsewhere makes the locking for v_bufobj even more obscure. v_bufobj is a whole struct living in the vnode. It has no locking annotation, but has a style bug (a stray '*') where its locking annotation should be. g_vfs_open() sets sc->sc_bo to &vp->v_bufobj and I think most uses of this don't lock the vnode or check if has been revoked. Perhaps ro accesses are OK (revoke() must not clean v_bufobj). Cleaning v_bufobj on mount failure without the vnode lock would be a bug. I think it is just not cleaned or used until the next mount changes it. Bruce