Date: Thu, 24 Aug 2017 20:18:03 -0700 From: Bryan Drewery <bdrewery@FreeBSD.org> To: Mateusz Guzik <mjg@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Konstantin Belousov <kib@FreeBSD.org> Subject: Re: svn commit: r306512 - in head/sys: kern sys Message-ID: <6ea06065-ea5c-b83b-3854-224626be0d77@FreeBSD.org> In-Reply-To: <201609301727.u8UHRIgD051373@repo.freebsd.org> References: <201609301727.u8UHRIgD051373@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gf0jINtmXxvW3VvgtjP6UiJJWd4L63TTo Content-Type: multipart/mixed; boundary="W4A4iApjpvSIriNGx07PeIPoIwROtm800"; protected-headers="v1" From: Bryan Drewery <bdrewery@FreeBSD.org> To: Mateusz Guzik <mjg@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Konstantin Belousov <kib@FreeBSD.org> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6ea06065-ea5c-b83b-3854-224626be0d77>