Date: Wed, 31 Oct 2012 04:57:14 +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=-Gid4M5jgsTmZATvb2i-_=NoXGOQX2AmUUy=fVd62kqKg@mail.gmail.com> In-Reply-To: <201210310355.q9V3tYu7042357@svn.freebsd.org> References: <201210310355.q9V3tYu7042357@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 to take a look at the patch to notify if there's something wrong, I'll be more than happy. Thanks Davide
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACYV=-Gid4M5jgsTmZATvb2i-_=NoXGOQX2AmUUy=fVd62kqKg>