svn commit: r243307 - head/sys/kern

[Attilio Rao]

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