Date: Wed, 31 Oct 2012 04:57:54 +0100 From: Davide Italiano <davide@freebsd.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r242387 - head/sys/fs/smbfs Message-ID: <CACYV=-GUo2GH7NLkXU7V1V-rnsobkXpU9b_NFG1X9F8Ahwix6A@mail.gmail.com> In-Reply-To: <CACYV=-Gid4M5jgsTmZATvb2i-_=NoXGOQX2AmUUy=fVd62kqKg@mail.gmail.com> References: <201210310355.q9V3tYu7042357@svn.freebsd.org> <CACYV=-Gid4M5jgsTmZATvb2i-_=NoXGOQX2AmUUy=fVd62kqKg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Oct 31, 2012 at 4:57 AM, Davide Italiano <davide@freebsd.org> wrote: > On Wed, Oct 31, 2012 at 4:55 AM, Davide Italiano <davide@freebsd.org> wrote: >> Author: davide >> Date: Wed Oct 31 03:55:33 2012 >> New Revision: 242387 >> URL: http://svn.freebsd.org/changeset/base/242387 >> >> Log: >> - Do not put in the mntqueue half-constructed vnodes. >> - Change the code so that it relies on vfs_hash rather than on a >> home-made hashtable. >> - There's no need to inline fnv_32_buf(). >> >> Reviewed by: delphij >> Tested by: pho >> Sponsored by: iXsystems inc. >> >> Modified: >> head/sys/fs/smbfs/smbfs.h >> head/sys/fs/smbfs/smbfs_node.c >> head/sys/fs/smbfs/smbfs_node.h >> head/sys/fs/smbfs/smbfs_vfsops.c >> >> Modified: head/sys/fs/smbfs/smbfs.h >> ============================================================================== >> --- head/sys/fs/smbfs/smbfs.h Wed Oct 31 03:34:07 2012 (r242386) >> +++ head/sys/fs/smbfs/smbfs.h Wed Oct 31 03:55:33 2012 (r242387) >> @@ -82,9 +82,6 @@ struct smbmount { >> /* struct simplelock sm_npslock;*/ >> struct smbnode * sm_npstack[SMBFS_MAXPATHCOMP]; >> int sm_caseopt; >> - struct sx sm_hashlock; >> - LIST_HEAD(smbnode_hashhead, smbnode) *sm_hash; >> - u_long sm_hashlen; >> int sm_didrele; >> }; >> >> >> Modified: head/sys/fs/smbfs/smbfs_node.c >> ============================================================================== >> --- head/sys/fs/smbfs/smbfs_node.c Wed Oct 31 03:34:07 2012 (r242386) >> +++ head/sys/fs/smbfs/smbfs_node.c Wed Oct 31 03:55:33 2012 (r242387) >> @@ -27,6 +27,7 @@ >> */ >> #include <sys/param.h> >> #include <sys/systm.h> >> +#include <sys/fnv_hash.h> >> #include <sys/kernel.h> >> #include <sys/lock.h> >> #include <sys/malloc.h> >> @@ -52,54 +53,15 @@ >> #include <fs/smbfs/smbfs_node.h> >> #include <fs/smbfs/smbfs_subr.h> >> >> -#define SMBFS_NOHASH(smp, hval) (&(smp)->sm_hash[(hval) & (smp)->sm_hashlen]) >> -#define smbfs_hash_lock(smp) sx_xlock(&smp->sm_hashlock) >> -#define smbfs_hash_unlock(smp) sx_xunlock(&smp->sm_hashlock) >> - >> extern struct vop_vector smbfs_vnodeops; /* XXX -> .h file */ >> >> static MALLOC_DEFINE(M_SMBNODE, "smbufs_node", "SMBFS vnode private part"); >> static MALLOC_DEFINE(M_SMBNODENAME, "smbufs_nname", "SMBFS node name"); >> >> -int smbfs_hashprint(struct mount *mp); >> - >> -#if 0 >> -#ifdef SYSCTL_DECL >> -SYSCTL_DECL(_vfs_smbfs); >> -#endif >> -SYSCTL_PROC(_vfs_smbfs, OID_AUTO, vnprint, CTLFLAG_WR|CTLTYPE_OPAQUE, >> - NULL, 0, smbfs_hashprint, "S,vnlist", "vnode hash"); >> -#endif >> - >> -#define FNV_32_PRIME ((u_int32_t) 0x01000193UL) >> -#define FNV1_32_INIT ((u_int32_t) 33554467UL) >> - >> -u_int32_t >> +u_int32_t __inline >> smbfs_hash(const u_char *name, int nmlen) >> { >> - u_int32_t v; >> - >> - for (v = FNV1_32_INIT; nmlen; name++, nmlen--) { >> - v *= FNV_32_PRIME; >> - v ^= (u_int32_t)*name; >> - } >> - return v; >> -} >> - >> -int >> -smbfs_hashprint(struct mount *mp) >> -{ >> - struct smbmount *smp = VFSTOSMBFS(mp); >> - struct smbnode_hashhead *nhpp; >> - struct smbnode *np; >> - int i; >> - >> - for(i = 0; i <= smp->sm_hashlen; i++) { >> - nhpp = &smp->sm_hash[i]; >> - LIST_FOREACH(np, nhpp, n_hash) >> - vprint("", SMBTOV(np)); >> - } >> - return 0; >> + return (fnv_32_buf(name, nmlen, FNV1_32_INIT)); >> } >> >> static char * >> @@ -149,6 +111,20 @@ smbfs_name_free(u_char *name) >> #endif >> } >> >> +static int __inline >> +smbfs_vnode_cmp(struct vnode *vp, void *_sc) >> +{ >> + struct smbnode *np; >> + struct smbcmp *sc; >> + >> + np = (struct smbnode *) vp; >> + sc = (struct smbcmp *) _sc; >> + if (np->n_parent != sc->n_parent || np->n_nmlen != sc->n_nmlen || >> + bcmp(sc->n_name, np->n_name, sc->n_nmlen) != 0) >> + return 1; >> + return 0; >> +} >> + >> static int >> smbfs_node_alloc(struct mount *mp, struct vnode *dvp, >> const char *name, int nmlen, struct smbfattr *fap, struct vnode **vpp) >> @@ -156,12 +132,14 @@ smbfs_node_alloc(struct mount *mp, struc >> struct vattr vattr; >> struct thread *td = curthread; /* XXX */ >> struct smbmount *smp = VFSTOSMBFS(mp); >> - struct smbnode_hashhead *nhpp; >> - struct smbnode *np, *np2, *dnp; >> - struct vnode *vp; >> - u_long hashval; >> + struct smbnode *np, *dnp; >> + struct vnode *vp, *vp2; >> + struct smbcmp sc; >> int error; >> >> + sc.n_parent = dvp; >> + sc.n_nmlen = nmlen; >> + sc.n_name = name; >> *vpp = NULL; >> if (smp->sm_root != NULL && dvp == NULL) { >> SMBERROR("do not allocate root vnode twice!\n"); >> @@ -184,38 +162,33 @@ smbfs_node_alloc(struct mount *mp, struc >> vprint("smbfs_node_alloc: dead parent vnode", dvp); >> return EINVAL; >> } >> - hashval = smbfs_hash(name, nmlen); >> -retry: >> - smbfs_hash_lock(smp); >> -loop: >> - nhpp = SMBFS_NOHASH(smp, hashval); >> - LIST_FOREACH(np, nhpp, n_hash) { >> - vp = SMBTOV(np); >> - if (np->n_parent != dvp || >> - np->n_nmlen != nmlen || bcmp(name, np->n_name, nmlen) != 0) >> - continue; >> - VI_LOCK(vp); >> - smbfs_hash_unlock(smp); >> - if (vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, td) != 0) >> - goto retry; >> + *vpp = NULL; >> + error = vfs_hash_get(mp, smbfs_hash(name, nmlen), LK_EXCLUSIVE, td, >> + vpp, smbfs_vnode_cmp, &sc); >> + if (error) >> + return (error); >> + if (*vpp) { >> + np = VTOSMB(*vpp); >> /* Force cached attributes to be refreshed if stale. */ >> - (void)VOP_GETATTR(vp, &vattr, td->td_ucred); >> + (void)VOP_GETATTR(*vpp, &vattr, td->td_ucred); >> /* >> * If the file type on the server is inconsistent with >> * what it was when we created the vnode, kill the >> * bogus vnode now and fall through to the code below >> * to create a new one with the right type. >> */ >> - if ((vp->v_type == VDIR && (np->n_dosattr & SMB_FA_DIR) == 0) || >> - (vp->v_type == VREG && (np->n_dosattr & SMB_FA_DIR) != 0)) { >> - vgone(vp); >> - vput(vp); >> - break; >> + if (((*vpp)->v_type == VDIR && >> + (np->n_dosattr & SMB_FA_DIR) == 0) || >> + ((*vpp)->v_type == VREG && >> + (np->n_dosattr & SMB_FA_DIR) != 0)) { >> + vgone(*vpp); >> + vput(*vpp); >> + } >> + else { >> + SMBVDEBUG("vnode taken from the hashtable\n"); >> + return (0); >> } >> - *vpp = vp; >> - return 0; >> } >> - smbfs_hash_unlock(smp); >> /* >> * If we don't have node attributes, then it is an explicit lookup >> * for an existing vnode. >> @@ -223,15 +196,13 @@ loop: >> if (fap == NULL) >> return ENOENT; >> >> - error = getnewvnode("smbfs", mp, &smbfs_vnodeops, &vp); >> - if (error != 0) >> - return (error); >> - error = insmntque(vp, mp); /* XXX: Too early for mpsafe fs */ >> - if (error != 0) >> + error = getnewvnode("smbfs", mp, &smbfs_vnodeops, vpp); >> + if (error) >> return (error); >> - >> + vp = *vpp; >> np = malloc(sizeof *np, M_SMBNODE, M_WAITOK | M_ZERO); >> - >> + lockmgr(vp->v_vnlock, LK_EXCLUSIVE, NULL); >> + /* Vnode initialization */ >> vp->v_type = fap->fa_attr & SMB_FA_DIR ? VDIR : VREG; >> vp->v_data = np; >> np->n_vnode = vp; >> @@ -239,7 +210,6 @@ loop: >> np->n_nmlen = nmlen; >> np->n_name = smbfs_name_alloc(name, nmlen); >> np->n_ino = fap->fa_ino; >> - >> if (dvp) { >> ASSERT_VOP_LOCKED(dvp, "smbfs_node_alloc"); >> np->n_parent = dvp; >> @@ -249,24 +219,18 @@ loop: >> } >> } else if (vp->v_type == VREG) >> SMBERROR("new vnode '%s' born without parent ?\n", np->n_name); >> - >> - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); >> - VN_LOCK_AREC(vp); >> - >> - smbfs_hash_lock(smp); >> - LIST_FOREACH(np2, nhpp, n_hash) { >> - if (np2->n_parent != dvp || >> - np2->n_nmlen != nmlen || bcmp(name, np2->n_name, nmlen) != 0) >> - continue; >> - vput(vp); >> -/* smb_name_free(np->n_name); >> - free(np, M_SMBNODE);*/ >> - goto loop; >> + error = insmntque(vp, mp); >> + if (error) { >> + free(np, M_SMBNODE); >> + return (error); >> } >> - LIST_INSERT_HEAD(nhpp, np, n_hash); >> - smbfs_hash_unlock(smp); >> - *vpp = vp; >> - return 0; >> + error = vfs_hash_insert(vp, smbfs_hash(name, nmlen), LK_EXCLUSIVE, >> + td, &vp2, smbfs_vnode_cmp, &sc); >> + if (error) >> + return (error); >> + if (vp2 != NULL) >> + *vpp = vp2; >> + return (0); >> } >> >> int >> @@ -307,26 +271,21 @@ smbfs_reclaim(ap) >> >> KASSERT((np->n_flag & NOPEN) == 0, ("file not closed before reclaim")); >> >> - smbfs_hash_lock(smp); >> /* >> * Destroy the vm object and flush associated pages. >> */ >> vnode_destroy_vobject(vp); >> - >> dvp = (np->n_parent && (np->n_flag & NREFPARENT)) ? >> np->n_parent : NULL; >> - >> - if (np->n_hash.le_prev) >> - LIST_REMOVE(np, n_hash); >> - if (smp->sm_root == np) { >> - SMBVDEBUG("root vnode\n"); >> - smp->sm_root = NULL; >> - } >> - vp->v_data = NULL; >> - smbfs_hash_unlock(smp); >> + >> + /* >> + * Remove the vnode from its hash chain. >> + */ >> + vfs_hash_remove(vp); >> if (np->n_name) >> smbfs_name_free(np->n_name); >> free(np, M_SMBNODE); >> + vp->v_data = NULL; >> if (dvp != NULL) { >> vrele(dvp); >> /* >> >> Modified: head/sys/fs/smbfs/smbfs_node.h >> ============================================================================== >> --- head/sys/fs/smbfs/smbfs_node.h Wed Oct 31 03:34:07 2012 (r242386) >> +++ head/sys/fs/smbfs/smbfs_node.h Wed Oct 31 03:55:33 2012 (r242387) >> @@ -63,6 +63,12 @@ struct smbnode { >> LIST_ENTRY(smbnode) n_hash; >> }; >> >> +struct smbcmp { >> + struct vnode * n_parent; >> + int n_nmlen; >> + const char * n_name; >> +}; >> + >> #define VTOSMB(vp) ((struct smbnode *)(vp)->v_data) >> #define SMBTOV(np) ((struct vnode *)(np)->n_vnode) >> >> >> Modified: head/sys/fs/smbfs/smbfs_vfsops.c >> ============================================================================== >> --- head/sys/fs/smbfs/smbfs_vfsops.c Wed Oct 31 03:34:07 2012 (r242386) >> +++ head/sys/fs/smbfs/smbfs_vfsops.c Wed Oct 31 03:55:33 2012 (r242387) >> @@ -58,8 +58,6 @@ SYSCTL_NODE(_vfs, OID_AUTO, smbfs, CTLFL >> SYSCTL_INT(_vfs_smbfs, OID_AUTO, version, CTLFLAG_RD, &smbfs_version, 0, ""); >> SYSCTL_INT(_vfs_smbfs, OID_AUTO, debuglevel, CTLFLAG_RW, &smbfs_debuglevel, 0, ""); >> >> -static MALLOC_DEFINE(M_SMBFSHASH, "smbfs_hash", "SMBFS hash table"); >> - >> static vfs_init_t smbfs_init; >> static vfs_uninit_t smbfs_uninit; >> static vfs_cmount_t smbfs_cmount; >> @@ -170,10 +168,6 @@ smbfs_mount(struct mount *mp) >> >> smp = malloc(sizeof(*smp), M_SMBFSDATA, M_WAITOK | M_ZERO); >> mp->mnt_data = smp; >> - smp->sm_hash = hashinit(desiredvnodes, M_SMBFSHASH, &smp->sm_hashlen); >> - if (smp->sm_hash == NULL) >> - goto bad; >> - sx_init(&smp->sm_hashlock, "smbfsh"); >> smp->sm_share = ssp; >> smp->sm_root = NULL; >> if (1 != vfs_scanopt(mp->mnt_optnew, >> @@ -243,12 +237,6 @@ smbfs_mount(struct mount *mp) >> smbfs_free_scred(scred); >> return error; >> bad: >> - if (smp) { >> - if (smp->sm_hash) >> - free(smp->sm_hash, M_SMBFSHASH); >> - sx_destroy(&smp->sm_hashlock); >> - free(smp, M_SMBFSDATA); >> - } >> if (ssp) >> smb_share_put(ssp, scred); >> smbfs_free_scred(scred); >> @@ -291,10 +279,6 @@ smbfs_unmount(struct mount *mp, int mntf >> goto out; >> smb_share_put(smp->sm_share, scred); >> mp->mnt_data = NULL; >> - >> - if (smp->sm_hash) >> - free(smp->sm_hash, M_SMBFSHASH); >> - sx_destroy(&smp->sm_hashlock); >> free(smp, M_SMBFSDATA); >> MNT_ILOCK(mp); >> mp->mnt_flag &= ~MNT_LOCAL; > > Alan, > I committed this because I'm going to commit in a while some changes > which depended on this commit. > If you want still tot t take a look at the patch to notify if there's > something wrong, I'll be more than happy. > > Thanks > > Davide I apologize, this was meant to be only for alc@.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACYV=-GUo2GH7NLkXU7V1V-rnsobkXpU9b_NFG1X9F8Ahwix6A>