From owner-freebsd-fs@freebsd.org Thu May 19 02:26:17 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 37314B40360 for ; Thu, 19 May 2016 02:26:17 +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 22FD31D2F for ; Thu, 19 May 2016 02:26:17 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: by mailman.ysv.freebsd.org (Postfix) id 22403B4035F; Thu, 19 May 2016 02:26:17 +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 1F93CB4035E for ; Thu, 19 May 2016 02:26:17 +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 D75C11D2E for ; Thu, 19 May 2016 02:26:16 +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 mail109.syd.optusnet.com.au (Postfix) with ESMTPS id A6997D689E5; Thu, 19 May 2016 12:20:20 +1000 (AEST) Date: Thu, 19 May 2016 12:20:19 +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: <20160519094901.O1798@besplex.bde.org> Message-ID: <20160519120557.A2250@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> 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=gX_jSoV2WuXo949cYewA: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: Thu, 19 May 2016 02:26:17 -0000 On Thu, 19 May 2016, Bruce Evans wrote: > On Thu, 19 May 2016, Bruce Evans wrote: > >> ... >> I think the following works to prevent multiple mounts via all of the >> known buggy paths: early in every fsmount(): Here is a lightly tested version: X Index: ffs_vfsops.c X =================================================================== X --- ffs_vfsops.c (revision 300160) X +++ ffs_vfsops.c (working copy) X @@ -771,18 +771,24 @@ X error = g_vfs_open(devvp, &cp, "ffs", ronly ? 0 : 1); X g_topology_unlock(); X PICKUP_GIANT(); X + /* XXX: v_bufobj is left set after errors. */ X + devvp->v_bufobj.bo_ops = &ffs_ops; X VOP_UNLOCK(devvp, 0); Since v_bufobj isn't cleaned after later errors, I didn't move the unlock to keep it clean for this error alone. X if (error) X - goto out; X - if (devvp->v_rdev->si_iosize_max != 0) X - mp->mnt_iosize_max = devvp->v_rdev->si_iosize_max; X + goto out1; X + dev_lock(); X + if (dev->si_mountpt != NULL) { X + dev_unlock(); X + error = EBUSY; X + goto out1; X + } X + dev->si_mountpt = mp; X + dev_unlock(); X + if (dev->si_iosize_max != 0) X + mp->mnt_iosize_max = dev->si_iosize_max; X if (mp->mnt_iosize_max > MAXPHYS) X mp->mnt_iosize_max = MAXPHYS; X X - devvp->v_bufobj.bo_ops = &ffs_ops; X - if (devvp->v_type == VCHR) X - devvp->v_rdev->si_mountpt = mp; X - X fs = NULL; X sblockloc = 0; X /* X @@ -1081,10 +1087,14 @@ X #endif /* !UFS_EXTATTR */ X return (0); X out: X + dev_lock(); X + if (dev->si_mountpt == NULL) X + panic("lost si_mountpt in mount"); X + dev->si_mountpt = NULL; X + dev_unlock(); I don't want the debugging panics or KASSERTs in the final version. Explicit locking the stores of NULL is probably not needed. dev_rel() will soon make these stores visible and other locking and ordering makes it very unlikely that they become visible too early. X +out1: X if (bp) X brelse(bp); X - if (devvp->v_type == VCHR && devvp->v_rdev != NULL) X - devvp->v_rdev->si_mountpt = NULL; X if (cp != NULL) { X DROP_GIANT(); X g_topology_lock(); X @@ -1287,8 +1297,11 @@ X g_vfs_close(ump->um_cp); X g_topology_unlock(); X PICKUP_GIANT(); X - if (ump->um_devvp->v_type == VCHR && ump->um_devvp->v_rdev != NULL) X - ump->um_devvp->v_rdev->si_mountpt = NULL; X + dev_lock(); X + if (ump->um_dev->si_mountpt == NULL) X + panic("lost si_mountpt in unmount"); X + ump->um_dev->si_mountpt = NULL; X + dev_unlock(); X vrele(ump->um_devvp); X dev_rel(ump->um_dev); X mtx_destroy(UFS_MTX(ump)); Bruce