Date: Thu, 9 Jul 2015 13:17:42 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: rwatson@FreeBSD.org, freebsd-fs@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 2/4] vfs: avoid spurious vref/vrele for absolute lookups Message-ID: <20150709101742.GN2080@kib.kiev.ua> In-Reply-To: <1436393231-5831-3-git-send-email-mjguzik@gmail.com> References: <20150707085857.GZ2080@kib.kiev.ua> <1436393231-5831-1-git-send-email-mjguzik@gmail.com> <1436393231-5831-3-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 09, 2015 at 12:07:09AM +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.
>
> Check for absolute lookup and vref the right vnode the first time.
> ---
> sys/kern/vfs_lookup.c | 70 +++++++++++++++++++++++++++++++++------------------
> 1 file changed, 46 insertions(+), 24 deletions(-)
>
> diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
> index 20f8e96..e434464 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;
Do not put initialization into declaration.
Otherwise, the patch looks fine.
> +
> + 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.
> *
> @@ -222,7 +243,18 @@ namei(struct nameidata *ndp)
> AUDIT_ARG_UPATH2(td, ndp->ni_dirfd, cnp->cn_pnbuf);
>
> dp = NULL;
> - if (cnp->cn_pnbuf[0] != '/') {
> + cnp->cn_nameptr = cnp->cn_pnbuf;
> + if (cnp->cn_pnbuf[0] == '/') {
> + error = namei_handle_root(ndp, &dp);
> + FILEDESC_SUNLOCK(fdp);
> + if (error != 0) {
> + vrele(ndp->ni_rootdir);
> + if (ndp->ni_startdir != NULL)
> + vrele(ndp->ni_startdir);
> + namei_cleanup_cnp(cnp);
> + return (error);
> + }
> + } else {
> if (ndp->ni_startdir != NULL) {
> dp = ndp->ni_startdir;
> error = 0;
> @@ -276,29 +308,6 @@ namei(struct nameidata *ndp)
> SDT_PROBE(vfs, namei, lookup, entry, dp, cnp->cn_pnbuf,
> cnp->cn_flags, 0, 0);
> for (;;) {
> - /*
> - * Check if root directory should replace current directory.
> - * Done at start of translation and after symbolic link.
> - */
> - cnp->cn_nameptr = cnp->cn_pnbuf;
> - if (*(cnp->cn_nameptr) == '/') {
> - vrele(dp);
> - if (ndp->ni_strictrelative != 0) {
> -#ifdef KTRACE
> - if (KTRPOINT(curthread, KTR_CAPFAIL))
> - ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL);
> -#endif
> - vrele(ndp->ni_rootdir);
> - namei_cleanup_cnp(cnp);
> - return (ENOTCAPABLE);
> - }
> - while (*(cnp->cn_nameptr) == '/') {
> - cnp->cn_nameptr++;
> - ndp->ni_pathlen--;
> - }
> - dp = ndp->ni_rootdir;
> - VREF(dp);
> - }
> ndp->ni_startdir = dp;
> error = lookup(ndp);
> if (error) {
> @@ -375,6 +384,19 @@ namei(struct nameidata *ndp)
> ndp->ni_pathlen += linklen;
> vput(ndp->ni_vp);
> dp = ndp->ni_dvp;
> + /*
> + * Check if root directory should replace current directory.
> + */
> + cnp->cn_nameptr = cnp->cn_pnbuf;
> + if (*(cnp->cn_nameptr) == '/') {
> + vrele(dp);
> + error = namei_handle_root(ndp, &dp);
> + if (error != 0) {
> + vrele(ndp->ni_rootdir);
> + namei_cleanup_cnp(cnp);
> + return (error);
> + }
> + }
> }
> vrele(ndp->ni_rootdir);
> namei_cleanup_cnp(cnp);
> --
> 2.4.5
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150709101742.GN2080>