From owner-freebsd-current@FreeBSD.ORG Wed Jan 25 18:35:44 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EA7611065670; Wed, 25 Jan 2012 18:35:44 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id AF9DD8FC13; Wed, 25 Jan 2012 18:35:44 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 50C5846B06; Wed, 25 Jan 2012 13:35:44 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id D2095B926; Wed, 25 Jan 2012 13:35:43 -0500 (EST) From: John Baldwin To: Kostik Belousov Date: Wed, 25 Jan 2012 13:35:43 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) References: <20120123013642.GB10149@sirius.xvoid.org> <4F1D74B9.8010800@FreeBSD.org> <20120125121528.GJ2726@deviant.kiev.zoral.com.ua> In-Reply-To: <20120125121528.GJ2726@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201201251335.43387.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 25 Jan 2012 13:35:43 -0500 (EST) Cc: Yuri Pankov , Rick Macklem , freebsd-current@freebsd.org Subject: Re: panic: No NCF_TS X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Jan 2012 18:35:45 -0000 On Wednesday, January 25, 2012 7:15:28 am Kostik Belousov wrote: > diff --git a/sys/fs/nfsclient/nfs_clvnops.c b/sys/fs/nfsclient/nfs_clvnops.c > index 2747191..709cd8d 100644 > --- a/sys/fs/nfsclient/nfs_clvnops.c > +++ b/sys/fs/nfsclient/nfs_clvnops.c > @@ -1431,8 +1431,6 @@ nfs_mknodrpc(struct vnode *dvp, struct vnode **vpp, struct componentname *cnp, > } > } > if (!error) { > - if ((cnp->cn_flags & MAKEENTRY)) > - cache_enter(dvp, newvp, cnp); > *vpp = newvp; > } else if (NFS_ISV4(dvp)) { > error = nfscl_maperr(cnp->cn_thread, error, vap->va_uid, This is good. > @@ -1591,7 +1589,7 @@ again: > } > if (!error) { > if (cnp->cn_flags & MAKEENTRY) > - cache_enter(dvp, newvp, cnp); > + cache_enter_time(dvp, newvp, cnp, &vap->va_ctime); > *ap->a_vpp = newvp; > } else if (NFS_ISV4(dvp)) { > error = nfscl_maperr(cnp->cn_thread, error, vap->va_uid, This should use the post-op attrs instead. Maybe this: if (cnp->cn_flags & MAKEENTRY && attrflag) cache_enter_time(dvp, newvp, cnp, &nfsva.na_ctime); > @@ -1966,8 +1964,9 @@ nfs_link(struct vop_link_args *ap) > * must care about lookup caching hit rate, so... > */ > if (VFSTONFS(vp->v_mount)->nm_negnametimeo != 0 && > - (cnp->cn_flags & MAKEENTRY)) > - cache_enter(tdvp, vp, cnp); > + (cnp->cn_flags & MAKEENTRY) && dattrflag) { > + cache_enter_time(tdvp, vp, cnp, &dnfsva.na_mtime); > + } > if (error && NFS_ISV4(vp)) > error = nfscl_maperr(cnp->cn_thread, error, (uid_t)0, > (gid_t)0); Looks good. > @@ -2023,15 +2022,6 @@ nfs_symlink(struct vop_symlink_args *ap) > error = nfscl_maperr(cnp->cn_thread, error, > vap->va_uid, vap->va_gid); > } else { > - /* > - * If negative lookup caching is enabled, I might as well > - * add an entry for this node. Not necessary for correctness, > - * but if negative caching is enabled, then the system > - * must care about lookup caching hit rate, so... > - */ > - if (VFSTONFS(dvp->v_mount)->nm_negnametimeo != 0 && > - (cnp->cn_flags & MAKEENTRY)) > - cache_enter(dvp, newvp, cnp); > *ap->a_vpp = newvp; > } > @@ -2041,6 +2031,16 @@ nfs_symlink(struct vop_symlink_args *ap) > if (dattrflag != 0) { > mtx_unlock(&dnp->n_mtx); > (void) nfscl_loadattrcache(&dvp, &dnfsva, NULL, NULL, 0, 1); > + /* > + * If negative lookup caching is enabled, I might as well > + * add an entry for this node. Not necessary for correctness, > + * but if negative caching is enabled, then the system > + * must care about lookup caching hit rate, so... > + */ > + if (VFSTONFS(dvp->v_mount)->nm_negnametimeo != 0 && > + (cnp->cn_flags & MAKEENTRY)) { > + cache_enter_time(dvp, newvp, cnp, &dnfsva.na_mtime); > + } > } else { > dnp->n_attrstamp = 0; > mtx_unlock(&dnp->n_mtx); Good. > @@ -2116,8 +2116,9 @@ nfs_mkdir(struct vop_mkdir_args *ap) > * must care about lookup caching hit rate, so... > */ > if (VFSTONFS(dvp->v_mount)->nm_negnametimeo != 0 && > - (cnp->cn_flags & MAKEENTRY)) > - cache_enter(dvp, newvp, cnp); > + (cnp->cn_flags & MAKEENTRY) && dattrflag) { > + cache_enter_time(dvp, newvp, cnp, &dnfsva.na_mtime); > + } > *ap->a_vpp = newvp; > } > return (error); Correct (I'm still not sure it is really best to be adding these extra entries, but that's a separate issue). > diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c > index 647dcac..4562ebc 100644 > --- a/sys/kern/vfs_cache.c > +++ b/sys/kern/vfs_cache.c > @@ -237,13 +237,9 @@ static void > cache_out_ts(struct namecache *ncp, struct timespec *tsp, int *ticksp) > { > > - if ((ncp->nc_flag & NCF_TS) == 0) { > - if (tsp != NULL) > - bzero(tsp, sizeof(*tsp)); > - if (ticksp != NULL) > - *ticksp = 0; > - return; > - } > + KASSERT((ncp->nc_flag & NCF_TS) != 0 || > + (tsp == NULL && ticksp == NULL), > + ("No NCF_TS")); > > if (tsp != NULL) > *tsp = ((struct namecache_ts *)ncp)->nc_time; > @@ -791,8 +787,8 @@ cache_enter_time(dvp, vp, cnp, tsp) > n2->nc_nlen == cnp->cn_namelen && > !bcmp(nc_get_name(n2), cnp->cn_nameptr, n2->nc_nlen)) { > if (tsp != NULL) { > - if ((n2->nc_flag & NCF_TS) == 0) > - continue; > + KASSERT((n2->nc_flag & NCF_TS) != 0, > + ("no NCF_TS")); > n3 = (struct namecache_ts *)n2; > n3->nc_time = > ((struct namecache_ts *)ncp)->nc_time; Good. > diff --git a/sys/nfsclient/nfs_vnops.c b/sys/nfsclient/nfs_vnops.c > index a39b29b..c2dfd97 100644 > --- a/sys/nfsclient/nfs_vnops.c > +++ b/sys/nfsclient/nfs_vnops.c > @@ -1530,8 +1530,6 @@ nfsmout: > if (newvp) > vput(newvp); > } else { > - if (cnp->cn_flags & MAKEENTRY) > - cache_enter(dvp, newvp, cnp); > *vpp = newvp; > } > mtx_lock(&(VTONFS(dvp))->n_mtx); > @@ -1670,8 +1668,6 @@ nfsmout: > vput(newvp); > } > if (!error) { > - if (cnp->cn_flags & MAKEENTRY) > - cache_enter(dvp, newvp, cnp); > *ap->a_vpp = newvp; > } > mtx_lock(&(VTONFS(dvp))->n_mtx); This is fine. If we really cared about these, we could add a variant of nfsm_wcc_data() that returned the post-op attributes, but it's not really worth doing I think. -- John Baldwin