Date: Wed, 18 May 2016 01:00:55 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs Message-ID: <20160517220055.GF89104@kib.kiev.ua> In-Reply-To: <20160518070252.F6121@besplex.bde.org> References: <20160517072104.I2137@besplex.bde.org> <20160517084241.GY89104@kib.kiev.ua> <20160518061040.D5948@besplex.bde.org> <20160518070252.F6121@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160517220055.GF89104>