svn commit: r243307 - head/sys/kern
Davide Italiano
davide at freebsd.org
Mon Nov 19 21:08:54 UTC 2012
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.
Thanks
More information about the svn-src-head
mailing list