From owner-freebsd-fs@freebsd.org Tue Jul 7 08:59:04 2015 Return-Path: Delivered-To: freebsd-fs@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 CE7819BB7 for ; Tue, 7 Jul 2015 08:59:04 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 5817419FD; Tue, 7 Jul 2015 08:59:04 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id t678wwIP057032 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Tue, 7 Jul 2015 11:58:59 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua t678wwIP057032 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id t678wvTF057031; Tue, 7 Jul 2015 11:58:57 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 7 Jul 2015 11:58:57 +0300 From: Konstantin Belousov To: Mateusz Guzik Cc: freebsd-fs@freebsd.org, kib@freebsd.org, rwatson@FreeBSD.org, Mateusz Guzik Subject: Re: [PATCH 1/2] vfs: avoid spurious vref/vrele for absolute lookups Message-ID: <20150707085857.GZ2080@kib.kiev.ua> References: <1436152035-12564-1-git-send-email-mjguzik@gmail.com> <1436152035-12564-2-git-send-email-mjguzik@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436152035-12564-2-git-send-email-mjguzik@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Jul 2015 08:59:04 -0000 On Mon, Jul 06, 2015 at 05:07:14AM +0200, Mateusz Guzik wrote: > From: Mateusz Guzik > > 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.