Date: Fri, 25 Aug 2017 13:35:34 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Bryan Drewery <bdrewery@FreeBSD.org> Cc: Mateusz Guzik <mjg@FreeBSD.org>, 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> In-Reply-To: <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org> References: <201609301727.u8UHRIgD051373@repo.freebsd.org> <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170825103534.GK1700>