From owner-svn-src-all@FreeBSD.ORG Mon Nov 19 21:11:42 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B0411FD6; Mon, 19 Nov 2012 21:11:42 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 936BF8FC12; Mon, 19 Nov 2012 21:11:41 +0000 (UTC) Received: by mail-lb0-f182.google.com with SMTP id go10so2422619lbb.13 for ; Mon, 19 Nov 2012 13:11:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=ZkfB8AzPOwu6BUrEJ39oIoCTdiwicjfG54f+pSvYZAA=; b=yu2uEYqCPLavQBlCWVwc/vfSmQtmgakk599sqFhtIrng5it01KV0P+zSugJjCgfH9L N50kGoXureSzbffXmiMj7aFvbh/XkcvS1xtaPhGaNqTvOqmbqPNnc0UwIXAdQSHwS+Yj OYR34zWwmlKX1WqF+8FcKjnvgbcY4qZ6FIgJ6FvTS4NEHDpEZ4Uk/E4VZ4r7zFnkK7e8 Zmuo39yr7sCU4Iqbx7ZphgCiOLgxl54R4+EtauvJP5lKWYmGB4xaWk/Gypcvs4VryN8V TItfhW/dm9+G9Qi45JbXI4GFdez0eiLVZeCuQIJzhQK27uqajqjEc27BQgcvyAmQ48sr 389w== MIME-Version: 1.0 Received: by 10.152.129.197 with SMTP id ny5mr12786964lab.43.1353359500489; Mon, 19 Nov 2012 13:11:40 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.112.134.5 with HTTP; Mon, 19 Nov 2012 13:11:40 -0800 (PST) In-Reply-To: References: <201211192043.qAJKhJ9i038016@svn.freebsd.org> Date: Mon, 19 Nov 2012 21:11:40 +0000 X-Google-Sender-Auth: CdB99dSwCkzxnGiltrPl4fyZjbk Message-ID: Subject: Re: svn commit: r243307 - head/sys/kern From: Attilio Rao To: Davide Italiano Content-Type: text/plain; charset=UTF-8 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Nov 2012 21:11:42 -0000 On Mon, Nov 19, 2012 at 9:08 PM, Davide Italiano wrote: > On Mon, Nov 19, 2012 at 9:55 PM, Attilio Rao wrote: >> On Mon, Nov 19, 2012 at 8:53 PM, Davide Italiano wrote: >>> On Mon, Nov 19, 2012 at 9:43 PM, Attilio Rao 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