From owner-svn-src-head@freebsd.org Fri Aug 25 03:18:30 2017 Return-Path: Delivered-To: svn-src-head@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 AAF8FDEE0FE; Fri, 25 Aug 2017 03:18:30 +0000 (UTC) (envelope-from bdrewery@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [96.47.72.132]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 7C35270A0C; Fri, 25 Aug 2017 03:18:30 +0000 (UTC) (envelope-from bdrewery@FreeBSD.org) Received: from mail.xzibition.com (unknown [127.0.1.132]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by freefall.freebsd.org (Postfix) with ESMTPS id C0C8E9D84; Fri, 25 Aug 2017 03:18:29 +0000 (UTC) (envelope-from bdrewery@FreeBSD.org) Received: from mail.xzibition.com (localhost [172.31.3.2]) by mail.xzibition.com (Postfix) with ESMTP id 93906728B; Fri, 25 Aug 2017 03:18:28 +0000 (UTC) X-Virus-Scanned: amavisd-new at mail.xzibition.com Received: from mail.xzibition.com ([172.31.3.2]) by mail.xzibition.com (mail.xzibition.com [172.31.3.2]) (amavisd-new, port 10026) with LMTP id 1ldS6IP9FPuL; Fri, 25 Aug 2017 03:18:25 +0000 (UTC) Subject: Re: svn commit: r306512 - in head/sys: kern sys DKIM-Filter: OpenDKIM Filter v2.9.2 mail.xzibition.com 4E0367286 To: Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Konstantin Belousov References: <201609301727.u8UHRIgD051373@repo.freebsd.org> From: Bryan Drewery Openpgp: id=F9173CB2C3AAEA7A5C8A1F0935D771BB6E4697CF; url=http://www.shatow.net/bryan/bryan2.asc Organization: FreeBSD Message-ID: <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org> Date: Thu, 24 Aug 2017 20:18:03 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <201609301727.u8UHRIgD051373@repo.freebsd.org> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="gf0jINtmXxvW3VvgtjP6UiJJWd4L63TTo" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Aug 2017 03:18:30 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gf0jINtmXxvW3VvgtjP6UiJJWd4L63TTo Content-Type: multipart/mixed; boundary="W4A4iApjpvSIriNGx07PeIPoIwROtm800"; protected-headers="v1" From: Bryan Drewery To: Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Konstantin Belousov Message-ID: <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org> Subject: Re: svn commit: r306512 - in head/sys: kern sys References: <201609301727.u8UHRIgD051373@repo.freebsd.org> In-Reply-To: <201609301727.u8UHRIgD051373@repo.freebsd.org> --W4A4iApjpvSIriNGx07PeIPoIwROtm800 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 >=20 > Log: > vfs: batch free vnodes in per-mnt lists > =20 > Previously free vnodes would always by directly returned to the globa= l > LRU list. With this change up to mnt_free_list_batch vnodes are colle= cted > first. > =20 > syncer runs always return the batch regardless of its size. > =20 > While vnodes on per-mnt lists are not counted as free, they can be > returned in case of vnode shortage. > =20 > Reviewed by: kib > Tested by: pho >=20 > Modified: > head/sys/kern/vfs_mount.c > head/sys/kern/vfs_subr.c > head/sys/sys/mount.h > head/sys/sys/vnode.h >=20 =2E.. > @@ -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 &=3D ~VI_FREE; > + mp =3D vp->v_mount; > + mtx_lock(&mp->mnt_listmtx); ^^ > + if ((vp->v_mflag & VMP_TMPMNTFREELIST) !=3D 0) { > + TAILQ_REMOVE(&mp->mnt_tmpfreevnodelist, vp, v_actfreelist); > + mp->mnt_tmpfreevnodelistsize--; > + vp->v_mflag &=3D ~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) =3D=3D 0, > ("Activating already active vnode")); > + vp->v_iflag &=3D ~VI_FREE; > vp->v_iflag |=3D VI_ACTIVE; > - mp =3D 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) =3D=3D 0) { > vp->v_iflag &=3D ~VI_ACTIVE; > mp =3D 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. 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. 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()? 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. > if (active) { > TAILQ_REMOVE(&mp->mnt_activevnodelist, vp, > v_actfreelist); > mp->mnt_activevnodelistsize--; > } > - TAILQ_INSERT_TAIL(&vnode_free_list, vp, > + TAILQ_INSERT_TAIL(&mp->mnt_tmpfreevnodelist, vp, > v_actfreelist); > - freevnodes++; > + mp->mnt_tmpfreevnodelistsize++; > vp->v_iflag |=3D VI_FREE; > - mtx_unlock(&vnode_free_list_mtx); > + vp->v_mflag |=3D VMP_TMPMNTFREELIST; > + VI_UNLOCK(vp); > + if (mp->mnt_tmpfreevnodelistsize >=3D mnt_free_list_batch) > + vnlru_return_batch_locked(mp); > + mtx_unlock(&mp->mnt_listmtx); > } else { > + VI_UNLOCK(vp); > atomic_add_long(&free_owe_inact, 1); > } > - VI_UNLOCK(vp); > return; > } > /* --=20 Regards, Bryan Drewery --W4A4iApjpvSIriNGx07PeIPoIwROtm800-- --gf0jINtmXxvW3VvgtjP6UiJJWd4L63TTo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBAgAGBQJZn5bxAAoJEDXXcbtuRpfPKMYIAKtRFjDNaq8J+4uSaeptaHBz DK71HOEk+C7vmFm5wWb+Mvhg+bBqlqwO7/ohQT4rYQ3TnxZHEnknR4Gbk0ClzHJa fajS9AdzfdL1qN8mDhfIErNf9UDfcgu4Nc2G8mmaorXvylb29vgv91hf924s1RhX 9CFTqe5tI+v3mbfOO6jj5qvptuIV5MgnaTyb/9M7v81fm1jYo5mUWUx+EXTFBnDL f16f6k3KzdHH2+MjGqstOOokui79WQL5f/URo8NS4qcTDIlqTdNU9IhOEslP65El f1neC87RdTX9WiRfWavAc6TeZzvSI6j95d0y3SYVjvLqpj5oTzjVgqJNWnkLadE= =AwAI -----END PGP SIGNATURE----- --gf0jINtmXxvW3VvgtjP6UiJJWd4L63TTo--