Date: Thu, 19 May 2016 13:41:28 +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: <20160519104128.GN89104@kib.kiev.ua> In-Reply-To: <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> <20160519120557.A2250@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 19, 2016 at 12:20:19PM +1000, Bruce Evans wrote:
> 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:
There is no need to protect the si_mountpt with any locking, the field
itself serves as a lock good enough, also preventing the parallel mounts
of the same devices. I changed the assignement to atomic_cmpset, which
is enough there. It is somewhat pity that this would reliably disable
multiple ro mounts of the same volume.
There is no need to move assignment of NULL to dev->si_mountpt later
in ffs_unmount(), the moment where the assignment is performed is safe
for other thread to start another mount.
I still want to keep devvp locked for long enough to cover the bufobj
hacking, and I do not want to move bufobj.bo_ops change before
g_vfs_open() succeed.
I also wanted to remove GIANT dances, but this requires geom patch,
which I will mail separately.
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);
+ }
DROP_GIANT();
g_topology_lock();
error = g_vfs_open(devvp, &cp, "ffs", ronly ? 0 : 1);
g_topology_unlock();
PICKUP_GIANT();
- VOP_UNLOCK(devvp, 0);
- if (error)
+ if (error != 0) {
+ VOP_UNLOCK(devvp, 0);
goto out;
- if (devvp->v_rdev->si_iosize_max != 0)
- mp->mnt_iosize_max = devvp->v_rdev->si_iosize_max;
+ }
+ if (dev->si_iosize_max != 0)
+ mp->mnt_iosize_max = dev->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;
+ VOP_UNLOCK(devvp, 0);
fs = NULL;
sblockloc = 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;
dev_rel(dev);
return (error);
}
@@ -1287,8 +1291,7 @@ 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;
+ ump->um_dev->si_mountpt = NULL;
vrele(ump->um_devvp);
dev_rel(ump->um_dev);
mtx_destroy(UFS_MTX(ump));
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160519104128.GN89104>