Date: Sat, 21 May 2016 11:48:20 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Bruce Evans <brde@optusnet.com.au>, fs@freebsd.org Subject: Re: fix for per-mount i/o counting in ffs Message-ID: <20160521111424.T1652@besplex.bde.org> In-Reply-To: <20160520153649.GX89104@kib.kiev.ua> References: <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> <20160520092348.GV89104@kib.kiev.ua> <20160520194427.W1170@besplex.bde.org> <20160520142654.GW89104@kib.kiev.ua> <20160520153649.GX89104@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 20 May 2016, Konstantin Belousov wrote:
[This is the version with uintptr_t casts]
> On Fri, May 20, 2016 at 05:26:54PM +0300, Konstantin Belousov wrote:
>> On Fri, May 20, 2016 at 09:22:08PM +1000, Bruce Evans wrote:
>>> On Fri, 20 May 2016, Konstantin Belousov wrote:
>>>
>>>> On Fri, May 20, 2016 at 09:27:38AM +1000, Bruce Evans 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"));
>>>>>> 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);
>>>>>> + }
>[*]
>>> The atomic cmpset now orders things too. Is that enough? It ensures
>>> that an old mount cannot be active. I don't know if v_bufobj is used
>>> for non-mounts.
>> v_bufobj is logically protected against modifications by the vnode lock.
I meant "is it enough if we drop the vnode lock earlier". Also, what protects
fro later accesses and modifications?
>>>> diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c
>>>> index 712fc21..670bb15 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"));
>>>
>>> Hrmph.
>> I want this, it would remove amount of obvious questions.
But it gives negatively useful runtime checking...
>>>> dev = devvp->v_rdev;
>>>> dev_ref(dev);
...this gives better runtime checking. It gives a nice restartable null
pointer trap except in the INVARIANTS case the KASSERT() gives a non-
restartable panic.
>> cmpset_<bar>_ptr() on i386 has cast for old and new parameters to u_int.
>> store_rel_ptr() on i386 does not cast value to u_int. As result, NULL
>> is acceptable for cmpset, but not for store. I spelled it 0 in all cases.
>>
>> Hm, I also should add uintptr_t cast for cmpset, otherwise, I suspect,
>> some arch might be broken.
> Even more casts are needed, updated patch is below.
These are necessary, unfortunately.
Perhaps 32-bit arches need the bogus casts more because of the
difference in pointer sizes. The caller might have a long variable
and expect this to work the same as an int variable on 32-bit arches
because the sizes are the same, but compilers should detect this
type mismatch (for pointers to these types). On 64-bit arches, callers
must be more careful and use only long variables. There are also
signedness problems. Plain int and plain long shouldn't work since
only unsigned variables are supported. Compilers should detect this
too, but traditionally they were sloppier about this, and we apparently
don't usually enable the warning flag for this.
> diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c
> index 712fc21..0487c2f 100644
> --- a/sys/ufs/ffs/ffs_vfsops.c
> +++ b/sys/ufs/ffs/ffs_vfsops.c
> @@ -764,25 +764,31 @@ 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"));
Hrmph.
> dev = devvp->v_rdev;
> - dev_ref(dev);
> + if (atomic_cmpset_acq_ptr((uintptr_t *)&dev->si_mountpt, 0,
> + (uintptr_t)mp) != 0) {
> + 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();
> + if (error != 0) {
> + VOP_UNLOCK(devvp, 0);
> + atomic_store_rel_ptr((uintptr_t *)&dev->si_mountpt, 0);
The store must be before the unlock (since we don't hold a reference to
the dev yet, the dev may go away on revoke after unlock).
Otherwise OK.
> + return (error);
> + }
> + dev_ref(dev);
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160521111424.T1652>