Date: Wed, 25 Jan 2012 13:35:43 -0500 From: John Baldwin <jhb@freebsd.org> To: Kostik Belousov <kostikbel@gmail.com> Cc: Yuri Pankov <yuri.pankov@gmail.com>, Rick Macklem <rmacklem@freebsd.org>, freebsd-current@freebsd.org Subject: Re: panic: No NCF_TS Message-ID: <201201251335.43387.jhb@freebsd.org> In-Reply-To: <20120125121528.GJ2726@deviant.kiev.zoral.com.ua> References: <20120123013642.GB10149@sirius.xvoid.org> <4F1D74B9.8010800@FreeBSD.org> <20120125121528.GJ2726@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201251335.43387.jhb>