svn commit: r243307 - head/sys/kern
Attilio Rao
attilio at freebsd.org
Mon Nov 19 21:11:42 UTC 2012
On Mon, Nov 19, 2012 at 9:08 PM, Davide Italiano <davide at freebsd.org> wrote:
> On Mon, Nov 19, 2012 at 9:55 PM, Attilio Rao <attilio at freebsd.org> wrote:
>> On Mon, Nov 19, 2012 at 8:53 PM, Davide Italiano <davide at freebsd.org> wrote:
>>> On Mon, Nov 19, 2012 at 9:43 PM, Attilio Rao <attilio at freebsd.org> wrote:
>>>> Author: attilio
>>>> Date: Mon Nov 19 20:43:19 2012
>>>> New Revision: 243307
>>>> URL: http://svnweb.freebsd.org/changeset/base/243307
>>>>
>>>> Log:
>>>> insmntque() is always called with the lock held in exclusive mode,
>>>> then:
>>>> - assume the lock is held in exclusive mode and remove a moot check
>>>> about the lock acquisition.
>>>> - in the destructor remove !MPSAFE specific chunk.
>>>>
>>>> Reviewed by: kib
>>>> MFC after: 2 weeks
>>>>
>>>> Modified:
>>>> head/sys/kern/vfs_subr.c
>>>>
>>>> Modified: head/sys/kern/vfs_subr.c
>>>> ==============================================================================
>>>> --- head/sys/kern/vfs_subr.c Mon Nov 19 19:31:55 2012 (r243306)
>>>> +++ head/sys/kern/vfs_subr.c Mon Nov 19 20:43:19 2012 (r243307)
>>>> @@ -1111,10 +1111,6 @@ insmntque_stddtr(struct vnode *vp, void
>>>>
>>>> vp->v_data = NULL;
>>>> vp->v_op = &dead_vnodeops;
>>>> - /* XXX non mp-safe fs may still call insmntque with vnode
>>>> - unlocked */
>>>> - if (!VOP_ISLOCKED(vp))
>>>> - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
>>>> vgone(vp);
>>>> vput(vp);
>>>> }
>>>> @@ -1126,7 +1122,6 @@ int
>>>> insmntque1(struct vnode *vp, struct mount *mp,
>>>> void (*dtr)(struct vnode *, void *), void *dtr_arg)
>>>> {
>>>> - int locked;
>>>>
>>>> KASSERT(vp->v_mount == NULL,
>>>> ("insmntque: vnode already on per mount vnode list"));
>>>> @@ -1144,18 +1139,15 @@ insmntque1(struct vnode *vp, struct moun
>>>> */
>>>> MNT_ILOCK(mp);
>>>> VI_LOCK(vp);
>>>> - if ((mp->mnt_kern_flag & MNTK_NOINSMNTQ) != 0 &&
>>>> + if (((mp->mnt_kern_flag & MNTK_NOINSMNTQ) != 0 &&
>>>> ((mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0 ||
>>>> - mp->mnt_nvnodelistsize == 0)) {
>>>> - locked = VOP_ISLOCKED(vp);
>>>> - if (!locked || (locked == LK_EXCLUSIVE &&
>>>> - (vp->v_vflag & VV_FORCEINSMQ) == 0)) {
>>>> - VI_UNLOCK(vp);
>>>> - MNT_IUNLOCK(mp);
>>>> - if (dtr != NULL)
>>>> - dtr(vp, dtr_arg);
>>>> - return (EBUSY);
>>>> - }
>>>> + mp->mnt_nvnodelistsize == 0)) &&
>>>> + (vp->v_vflag & VV_FORCEINSMQ) == 0) {
>>>> + VI_UNLOCK(vp);s
>>>> + MNT_IUNLOCK(mp);
>>>> + if (dtr != NULL)
>>>> + dtr(vp, dtr_arg);
>>>> + return (EBUSY);
>>>> }
>>>> vp->v_mount = mp;
>>>> MNT_REF(mp);
>>>
>>> Thanks for doing this.
>>> Attilio, I don't know if this really could help, but what do you think
>>> about adding an assertion to check if the vnode is locked?
>>> This could help in some cases, e.g. it might be useful to discover the
>>> violation of this assumption for a developer which wants to port a new
>>> fs into the source tree.
>>
>> Exactly where? insmntque1() already has this.
>>
>> Attilio
>>
>>
>> --
>> Peace can only be achieved by understanding - A. Einstein
>
> I was talking about the destructor code, instead of the vn_lock() call
> which you removed.
> I was in doubt so I asked, but now after closely looking at the code I
> see the destructor function is called only within insmntque1 and the
> check I suggest is probably redundant/useless.
I've discussed this with kib privately, the thing is that asserts in
insmntque1() and ones in the destructors implicitely (like the one in
vgone()) should give enough protection already.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
More information about the svn-src-all
mailing list