From owner-freebsd-fs@freebsd.org Tue May 17 22:01:05 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 DAE69B401E6 for ; Tue, 17 May 2016 22:01:05 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id C4AD21D50 for ; Tue, 17 May 2016 22:01:05 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: by mailman.ysv.freebsd.org (Postfix) id C3FEAB401E5; Tue, 17 May 2016 22:01:05 +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 C39D5B401E4 for ; Tue, 17 May 2016 22:01:05 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 6D5F21D4F for ; Tue, 17 May 2016 22:01:05 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u4HM0tdA016113 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Wed, 18 May 2016 01:00:56 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u4HM0tdA016113 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u4HM0t7E016110; Wed, 18 May 2016 01:00:55 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 18 May 2016 01:00:55 +0300 From: Konstantin Belousov To: Bruce Evans Cc: fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs Message-ID: <20160517220055.GF89104@kib.kiev.ua> References: <20160517072104.I2137@besplex.bde.org> <20160517084241.GY89104@kib.kiev.ua> <20160518061040.D5948@besplex.bde.org> <20160518070252.F6121@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160518070252.F6121@besplex.bde.org> User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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: Tue, 17 May 2016 22:01:06 -0000 On Wed, May 18, 2016 at 07:30:25AM +1000, Bruce Evans wrote: > Further cleanups: > - the null pointer check is bogus since we already dereferenced > devvp->v_rdev. We also assigned devvp->v_rdev to the variable > dev but spelled out devvp->v_rdev in a couple of other places. > - the VCHR check is bogus since we only work for VCHR and have > already checked for VCHR in vn_isdisk(). No, these are not bogus. The checks are incorrect because they are racy, but they are needed with the proper locking. I intended to look at this tomorrow, since the fixes are not related to the current changes, but you forced me. VCHR check ensures that the devvp vnode is not reclaimed. I do not want to remove the check and rely on the caller of ffs_mountfs() to always do the right thing for it without unlocking devvp, this is too subtle. We are safe from devvp being reclaimed when io is in progress, since our reference prevents cdev memory from free, which ensures that v_rdev is valid if non-NULL. Unmount is not supposed to finish until all io is finished (but we had bugs there). > > Similarly in ffs_umount() except there is no dev variable there. There is ump->um_dev. diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c index 712fc21..da61c15 100644 --- a/sys/ufs/ffs/ffs_vfsops.c +++ b/sys/ufs/ffs/ffs_vfsops.c @@ -771,17 +771,18 @@ ffs_mountfs(devvp, mp, td) error = g_vfs_open(devvp, &cp, "ffs", ronly ? 0 : 1); g_topology_unlock(); PICKUP_GIANT(); - VOP_UNLOCK(devvp, 0); - if (error) + if (error) { + VOP_UNLOCK(devvp, 0); goto out; - if (devvp->v_rdev->si_iosize_max != 0) + } + if (dev->si_iosize_max != 0) mp->mnt_iosize_max = devvp->v_rdev->si_iosize_max; if (mp->mnt_iosize_max > MAXPHYS) mp->mnt_iosize_max = MAXPHYS; - devvp->v_bufobj.bo_ops = &ffs_ops; if (devvp->v_type == VCHR) - devvp->v_rdev->si_mountpt = mp; + dev->si_mountpt = mp; + VOP_UNLOCK(devvp, 0); fs = NULL; sblockloc = 0; @@ -1083,8 +1084,10 @@ ffs_mountfs(devvp, mp, td) out: if (bp) brelse(bp); + VOP_LOCK(devvp, LK_EXCLUSIVE | LK_RETRY); if (devvp->v_type == VCHR && devvp->v_rdev != NULL) devvp->v_rdev->si_mountpt = NULL; + VOP_UNLOCK(devvp, 0); if (cp != NULL) { DROP_GIANT(); g_topology_lock(); @@ -1287,9 +1290,11 @@ ffs_unmount(mp, mntflags) g_vfs_close(ump->um_cp); g_topology_unlock(); PICKUP_GIANT(); - if (ump->um_devvp->v_type == VCHR && ump->um_devvp->v_rdev != NULL) - ump->um_devvp->v_rdev->si_mountpt = NULL; - vrele(ump->um_devvp); + VOP_LOCK(ump->um_devvp, LK_EXCLUSIVE | LK_RETRY); + if (ump->um_devvp->v_type == VCHR && + ump->um_devvp->v_rdev == ump->um_dev) + ump->um_dev->si_mountpt = NULL; + vput(ump->um_devvp); dev_rel(ump->um_dev); mtx_destroy(UFS_MTX(ump)); if (mp->mnt_gjprovider != NULL) {