Date: Tue, 7 Jul 2015 11:58:57 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-fs@freebsd.org, kib@freebsd.org, rwatson@FreeBSD.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 1/2] vfs: avoid spurious vref/vrele for absolute lookups Message-ID: <20150707085857.GZ2080@kib.kiev.ua> In-Reply-To: <1436152035-12564-2-git-send-email-mjguzik@gmail.com> References: <1436152035-12564-1-git-send-email-mjguzik@gmail.com> <1436152035-12564-2-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 06, 2015 at 05:07:14AM +0200, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg@freebsd.org>
>
> namei used to vref fd_cdir, which was immediatley vrele'd on entry to
> the loop.
Does it make sense to do this, if the other patch, for interlock-less
vref/vrele on holdcount > 0, is in progress ?
>
> Simplify error handling and remove type checking for ni_startdir vnode.
> It is only set by nfs which does the check on its own. Assert the
> correct type instead.
> ---
> sys/kern/vfs_lookup.c | 92 ++++++++++++++++++++++++++++-----------------------
> 1 file changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
> index 5dc07dc..c5218ec 100644
> --- a/sys/kern/vfs_lookup.c
> +++ b/sys/kern/vfs_lookup.c
> @@ -109,6 +109,27 @@ namei_cleanup_cnp(struct componentname *cnp)
> #endif
> }
>
> +static int
> +namei_handle_root(struct nameidata *ndp, struct vnode **dpp)
> +{
> + struct componentname *cnp = &ndp->ni_cnd;
> +
> + if (ndp->ni_strictrelative != 0) {
> +#ifdef KTRACE
> + if (KTRPOINT(curthread, KTR_CAPFAIL))
> + ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL);
> +#endif
> + return (ENOTCAPABLE);
> + }
> + while (*(cnp->cn_nameptr) == '/') {
> + cnp->cn_nameptr++;
> + ndp->ni_pathlen--;
> + }
> + *dpp = ndp->ni_rootdir;
> + VREF(*dpp);
> + return (0);
> +}
> +
> /*
> * Convert a pathname into a pointer to a locked vnode.
> *
> @@ -148,6 +169,8 @@ namei(struct nameidata *ndp)
> ("namei: nameiop contaminated with flags"));
> KASSERT((cnp->cn_flags & OPMASK) == 0,
> ("namei: flags contaminated with nameiops"));
> + if (ndp->ni_startdir != NULL)
> + MPASS(ndp->ni_startdir->v_type == VDIR);
ni_startdir is not locked, am I correct ? If yes, the assert is not safe.
> if (!lookup_shared)
> cnp->cn_flags &= ~LOCKSHARED;
> fdp = p->p_fd;
Could this patch be further split ? E.g. could the introduction of the
namei_handle_root() and its use twice be done in the first patch, while
the loop logic reorganization come into the follow-up ?
As it is now, the patch is almost impossible to review without rewriting
the logic independently.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150707085857.GZ2080>