From owner-svn-src-all@freebsd.org Fri Aug 25 10:35:41 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A2504DD47F7; Fri, 25 Aug 2017 10:35:41 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 389AF806E3; Fri, 25 Aug 2017 10:35:41 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id v7PAZYUZ046534 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 25 Aug 2017 13:35:34 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v7PAZYUZ046534 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v7PAZYhx046533; Fri, 25 Aug 2017 13:35:34 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 25 Aug 2017 13:35:34 +0300 From: Konstantin Belousov To: Bryan Drewery Cc: Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r306512 - in head/sys: kern sys Message-ID: <20170825103534.GK1700@kib.kiev.ua> References: <201609301727.u8UHRIgD051373@repo.freebsd.org> <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org> User-Agent: Mutt/1.8.3 (2017-05-23) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 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: Fri, 25 Aug 2017 10:35:41 -0000 On Thu, Aug 24, 2017 at 08:18:03PM -0700, Bryan Drewery wrote: > On 9/30/2016 10:27 AM, Mateusz Guzik wrote: > > Author: mjg > > Date: Fri Sep 30 17:27:17 2016 > > New Revision: 306512 > > URL: https://svnweb.freebsd.org/changeset/base/306512 > > > > Log: > > vfs: batch free vnodes in per-mnt lists > > > > Previously free vnodes would always by directly returned to the global > > LRU list. With this change up to mnt_free_list_batch vnodes are collected > > first. > > > > syncer runs always return the batch regardless of its size. > > > > While vnodes on per-mnt lists are not counted as free, they can be > > returned in case of vnode shortage. > > > > Reviewed by: kib > > Tested by: pho > > > > Modified: > > head/sys/kern/vfs_mount.c > > head/sys/kern/vfs_subr.c > > head/sys/sys/mount.h > > head/sys/sys/vnode.h > > > ... > > @@ -2753,17 +2824,25 @@ _vhold(struct vnode *vp, bool locked) > > * Remove a vnode from the free list, mark it as in use, > > * and put it on the active list. > > */ > > - mtx_lock(&vnode_free_list_mtx); > > - TAILQ_REMOVE(&vnode_free_list, vp, v_actfreelist); > > - freevnodes--; > > - vp->v_iflag &= ~VI_FREE; > > + mp = vp->v_mount; > > + mtx_lock(&mp->mnt_listmtx); > ^^ > > > + if ((vp->v_mflag & VMP_TMPMNTFREELIST) != 0) { > > + TAILQ_REMOVE(&mp->mnt_tmpfreevnodelist, vp, v_actfreelist); > > + mp->mnt_tmpfreevnodelistsize--; > > + vp->v_mflag &= ~VMP_TMPMNTFREELIST; > > + } else { > > + mtx_lock(&vnode_free_list_mtx); > > + TAILQ_REMOVE(&vnode_free_list, vp, v_actfreelist); > > + freevnodes--; > > + mtx_unlock(&vnode_free_list_mtx); > > + } > > KASSERT((vp->v_iflag & VI_ACTIVE) == 0, > > ("Activating already active vnode")); > > + vp->v_iflag &= ~VI_FREE; > > vp->v_iflag |= VI_ACTIVE; > > - mp = vp->v_mount; > > TAILQ_INSERT_HEAD(&mp->mnt_activevnodelist, vp, v_actfreelist); > > mp->mnt_activevnodelistsize++; > > - mtx_unlock(&vnode_free_list_mtx); > > + mtx_unlock(&mp->mnt_listmtx); > > refcount_acquire(&vp->v_holdcnt); > > if (!locked) > > VI_UNLOCK(vp); > > @@ -2819,21 +2898,25 @@ _vdrop(struct vnode *vp, bool locked) > > if ((vp->v_iflag & VI_OWEINACT) == 0) { > > vp->v_iflag &= ~VI_ACTIVE; > > mp = vp->v_mount; > > - mtx_lock(&vnode_free_list_mtx); > > + mtx_lock(&mp->mnt_listmtx); > ^^ > > If code runs getnewvnode() and then immediately runs vhold() or vrele(), > without first running insmntque(vp, mp), then vp->v_mount is NULL here > and the lock/freelist dereferencing just panic. getnewvnode() returns vref-ed vnode, i.e. both hold and use counts are set to 1. What is the use case there which requires vhold-ing vnode without finishing its construction ? > Locking the vnode and then using vgone() and vput() on it avoids the > issue since it marks the vnode VI_DOOMED and instead frees it in > _vdrop() rather than try to re-add it to its NULL per-mount free list. This is the common pattern where insmntque() fails. > > I'm not sure what the right thing here is. Is it a requirement to > insmntque() a new vnode immediately, or to vgone() it before releasing > it if was just returned from getnewvnode()? These are the only uses of a newly allocated vnode which make sense. Do you need this for something that does not happen in the svn tree ? > > Perhaps these cases should assert that v_mount is not NULL or handle the > odd case and just free the vnode instead, or can it still just add to > the global vnode_free_list if mp is NULL? > > The old code handled the case fine since the freelist was global and not > per-mount. 'mp' was only dereferenced below if the vnode had been > active, which was not the case for my example.