Date: Fri, 20 May 2016 10:11:38 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: Konstantin Belousov <kostikbel@gmail.com>, fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs Message-ID: <20160520095504.X1527@besplex.bde.org> In-Reply-To: <20160520074427.W1151@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160520095504.X1527>