Date: Fri, 20 May 2016 13:22:09 +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: <20160520120927.V2190@besplex.bde.org> In-Reply-To: <20160520095504.X1527@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> <20160520095504.X1527@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
PS2:
On Fri, 20 May 2016, Bruce Evans wrote:
> 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.
All file systems need this of course.
zfs doesn't use g_vfs_open(), so how can it possibly work to give
exclusive access to the device in contention with other mount
operations? I think it doesn't even try.
Old code used vfs_mountedon() here. vfs_mountedon() was just the above cmp
(but not set) in an extern function, with no obvious locking and an a
differently bad name for si_mountpt (it was si_mountpoint. The correct
name is si_mp). The vnode should be locked, but this was only enough if
the old aliasing code gave a unique vnode. Old code also returned EBUSY
if vcount(devvp) > 1 && devvp != rootvp. Here rootvp is special to support
some old hack involving abusing the swap device for miniroot. This was
supposed to have been replaced by g_access() checks in g_vfs_open(), but
those aren't exclusive enough, so there is another check in g_vfs_open()
but that isn't exclusive enough so we are trying to fix it now.
zfs_mount() seems to have no exclusivity check at all, except in the
illumos case it has the old vcount() check with rootvp hack (spelled
differently as (v_flag & VROOT)).
zfs might support multiple mounts, but it can only do that for itself
and the vcount() check normally prevents this for the illumos case.
vfs_mountedon() is as good an interface as any for checking for exclusive
access in a shared way (except it probably can't support multiple mounts
like g_vfs_open() is supposed to). It was in 4.4BSD-Lite. 4.4BSD-Lite
doesn't have si_mountp[oin]t. It used old alias stuff which is relatively
easy to understand for it. It searches the list of aliases and skips ones
whose type differs (these are presumably revoked ones). The aliases are
vnodes with a common rdev. Each vnode has a V_MOUNTEDON flag. I wonder
if current bugs affected this too -- after revoke, the device is still
mounted but its vnode is too messed up to show this.
vfs_mountedon() in FreeBSD-3 is similar to in 4.4BSD-Lite. In FreeBSD-4,
it is an in-between version that looks broken since it doesn't have the
alias loop; it depends on vp->v_specmountpoint being the same for all
aliases even with only 1 of the aliases locked.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160520120927.V2190>