From owner-svn-src-all@FreeBSD.ORG Mon Nov 19 21:08:54 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 D6A3CBE5; Mon, 19 Nov 2012 21:08:54 +0000 (UTC) (envelope-from davide.italiano@gmail.com) Received: from mail-vc0-f182.google.com (mail-vc0-f182.google.com [209.85.220.182]) by mx1.freebsd.org (Postfix) with ESMTP id 3C1F48FC0C; Mon, 19 Nov 2012 21:08:54 +0000 (UTC) Received: by mail-vc0-f182.google.com with SMTP id fo13so7287755vcb.13 for ; Mon, 19 Nov 2012 13:08:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=Jlz5O1ts5Vj0d0SWVRrY+srq607OCQZcg/Q4iTR+Fio=; b=u4eeD3lqIStNIeBT/k8jFk9cYULfOHWX33N55T8Rq63McX9K3cGrZR/ho0z4OuGwaX 4i/YXl7ofAgFxCJm5k/uBGX8o0PG1NlRxCGwBiRYy8CNiCQbxyhr/JL8BBtBUMzEGQJ2 K084OVn3EK3kY28YtU8LMPVhLGAd6LRydpErBQ4pZ3jLmaKGdbZbt2LaEo/y0OWxoqRH c4uBxSRHPi5YH0g7KHp8TPx8bxVtkF2SD8tCByEa+B3r8TGAzsYy3wOGvJkIf76YWhtB XQrYk6wwsjZaZSls60TSu+wEavdymUkYHw4DHSO+FGtY+YCrwljqwLrm6rYsLcbpMbR6 NQsQ== MIME-Version: 1.0 Received: by 10.52.89.235 with SMTP id br11mr18292783vdb.77.1353359333638; Mon, 19 Nov 2012 13:08:53 -0800 (PST) Sender: davide.italiano@gmail.com Received: by 10.58.247.132 with HTTP; Mon, 19 Nov 2012 13:08:53 -0800 (PST) In-Reply-To: References: <201211192043.qAJKhJ9i038016@svn.freebsd.org> Date: Mon, 19 Nov 2012 22:08:53 +0100 X-Google-Sender-Auth: M4I7k7jnF3DOajPl20ZJe2dfdQs Message-ID: Subject: Re: svn commit: r243307 - head/sys/kern From: Davide Italiano To: attilio@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 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 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:08:54 -0000 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. Thanks