Date: Mon, 5 Jul 2010 20:06:13 GMT From: Ilya Putsikau <ilya@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 180499 for review Message-ID: <201007052006.o65K6DTk060411@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@180499?ac=10 Change 180499 by ilya@ilya_triton on 2010/07/05 20:05:57 Fix bugs related to reference counting and resource deallocation Affected files ... .. //depot/projects/soc2010/ilya_fsnotify/src/sys/kern/vfs_notify.c#5 edit Differences ... ==== //depot/projects/soc2010/ilya_fsnotify/src/sys/kern/vfs_notify.c#5 (text+ko) ==== @@ -162,10 +162,14 @@ static struct fnnode* node_lookupex(struct vnode *vp, ino_t *inop, int flags); static struct fnnode* node_alloc(struct vnode *vp, ino_t ino); +static void node_hold(struct fnnode *node); static void node_drop(struct fnnode *node); +static void node_watchhold(struct fnnode *node); +static void node_watchdrop(struct fnnode *node); static void event_copypath(struct fnevent *event, char *path, int *pathlen); static int event_pathlen(struct fnevent *event); static int event_nextcookie(void); +static void watch_free(struct fnwatch *watch); #define NODE_ISVALID(a) ((a) != NULL && (a) != FNNODE_INVAL) @@ -175,7 +179,9 @@ static int fsnotify_modevent(struct module *module, int cmd, void *arg) { - int hashsize; + struct fnnode_hashhead *hashhead; + struct fnnode *node; + int i; int error = 0; switch (cmd) { @@ -195,8 +201,8 @@ mtx_init(&fsnotify_queue_mtx, "fsnotify_queue", NULL, MTX_DEF); mtx_init(&fnnode_hashmtx, "fsnotify_hash", NULL, MTX_DEF); - hashsize = MAX(desiredvnodes / 32, 16); - fnnode_inohashtbl = hashinit(hashsize, M_FSNOTIFYHASH, + i = MAX(desiredvnodes / 32, 16); + fnnode_inohashtbl = hashinit(i, M_FSNOTIFYHASH, &fnnode_hashmask); TASK_INIT(&fsnotify_task, 0, process_queue, NULL); @@ -230,6 +236,28 @@ destroy_dev(fsnotify_dev); taskqueue_drain(fsnotify_tq, &fsnotify_task); taskqueue_free(fsnotify_tq); + for (i = 0; i <= fnnode_hashmask; i++) { + hashhead = &fnnode_inohashtbl[i]; + while (!LIST_EMPTY(hashhead)) { + node = LIST_FIRST(hashhead); + mtx_lock(&node->nd_mtx); + if (node->nd_vnode != NULL) { + VI_LOCK(node->nd_vnode); + node->nd_vnode->v_fnnode = NULL; + printf("fsnotify unload: deref vp: node %p vp %p\n", + node, node->nd_vnode); + VI_UNLOCK(node->nd_vnode); + node->nd_vnode = NULL; + mtx_unlock(&node->nd_mtx); + node_drop(node); + } else + mtx_unlock(&node->nd_mtx); + KASSERT(LIST_FIRST(hashhead)->nd_refcnt == 1, + ("Invalid node reference count: %d", + LIST_FIRST(hashhead)->nd_refcnt)); + node_drop(LIST_FIRST(hashhead)); + } + } free(fnnode_inohashtbl, M_FSNOTIFYHASH); mtx_destroy(&fsnotify_queue_mtx); mtx_destroy(&fnnode_hashmtx); @@ -262,17 +290,27 @@ fsnotify_session_dtor(void *data) { struct fnsession *ss = data; + struct fnwatch *watch; struct fneventhandle *eh; + printf("session_dtor: %p\n", ss); + mtx_lock(&ss->ss_mtx); while (!TAILQ_EMPTY(&ss->ss_queue)) { eh = TAILQ_FIRST(&ss->ss_queue); session_drophandle(ss, eh); } + while (!TAILQ_EMPTY(&ss->ss_watchlist)) { + watch = TAILQ_FIRST(&ss->ss_watchlist); + watch_free(watch); + } + mtx_unlock(&ss->ss_mtx); + cv_destroy(&ss->ss_queuecv); mtx_destroy(&ss->ss_mtx); free(ss, M_FSNOTIFY); + printf("session free: %p\n", ss); } static int @@ -281,6 +319,7 @@ struct fnsession *ss; ss = malloc(sizeof(struct fnsession), M_FSNOTIFY, M_WAITOK | M_ZERO); + printf("session alloc: %p\n", ss); mtx_init(&ss->ss_mtx, "fnsession_queue", NULL, MTX_DEF); cv_init(&ss->ss_queuecv, "fnsession_queuecv"); @@ -289,6 +328,8 @@ devfs_set_cdevpriv(ss, fsnotify_session_dtor); + printf("fsnotify_open: session %p\n", ss); + return (0); } @@ -300,9 +341,11 @@ struct fnsession *ss; struct fnwatch *watch; struct fsnotify_event *fe; - int len, error; + int destroy, len, error; char user_buf[sizeof(struct fsnotify_event) + MAXPATHLEN]; + printf("fsnotify_read: offset %jd\n", uio->uio_offset); + if (uio->uio_resid == 0) return (0); @@ -310,8 +353,6 @@ if (error != 0) return (error); - printf("fsnotify_read: offset %jd\n", uio->uio_offset); - mtx_lock(&ss->ss_mtx); if (TAILQ_EMPTY(&ss->ss_queue)) { @@ -336,8 +377,8 @@ fe->fe_fileno = event->ev_node->nd_ino; fe->fe_cookie = event->ev_cookie; event_copypath(event, fe->fe_name, &fe->fe_namelen); - fe->fe_namelen += 1; len = fe->fe_namelen + sizeof(struct fsnotify_event); + destroy = event->ev_mask & FE_DESTROY; mtx_unlock(&ss->ss_mtx); @@ -348,6 +389,8 @@ uio->uio_offset = 0; mtx_lock(&ss->ss_mtx); session_drophandle(ss, eh); + if (destroy != 0) + watch_free(watch); mtx_unlock(&ss->ss_mtx); } @@ -391,6 +434,8 @@ vfslocked = VFS_LOCK_GIANT(vp->v_mount); node = node_lookupex(vp, &ino, LOOKUP_IGNINVAL); if (node != NULL) { + node_watchhold(node); + mtx_unlock(&node->nd_mtx); VFS_UNLOCK_GIANT(vfslocked); } else { error = vn_fullpath(td, vp, &path, &pathfree); @@ -401,8 +446,11 @@ node->nd_path = path; node->nd_pathlen = strlen(path); node->nd_pathfree = pathfree; + node_watchhold(node); + mtx_unlock(&node->nd_mtx); } error = session_addwatch(ss, node, add_args->fa_mask, &watch); + node_drop(node); if (error == 0) add_args->fa_wd = watch->wt_wd; /* If error != 0 node will be removed by hook_reclaim */ @@ -465,8 +513,14 @@ vp->v_fnnode = NULL; VI_UNLOCK(vp); - if (NODE_ISVALID(node)) + if (NODE_ISVALID(node)) { + printf("node reclaim: deref vnode: node %p vp %p\n", + node, node->nd_vnode); + mtx_lock(&node->nd_mtx); + node->nd_vnode = NULL; + mtx_unlock(&node->nd_mtx); node_drop(node); + } } static __inline int @@ -489,6 +543,7 @@ int cookie; cookie = event_nextcookie(); + printf("hook_generic_remove: %s\n", cnp->cn_nameptr); node = node_lookup(vp); if (node != NULL) @@ -502,6 +557,7 @@ static int hook_create(struct vop_create_args *ap) { + printf("hook_create: %s\n", ap->a_cnp->cn_nameptr); hook_generic_create(ap->a_dvp, *ap->a_vpp, ap->a_cnp); return (0); } @@ -509,6 +565,7 @@ static int hook_mkdir(struct vop_mkdir_args *ap) { + printf("hook_mkdir: %s\n", ap->a_cnp->cn_nameptr); hook_generic_create(ap->a_dvp, *ap->a_vpp, ap->a_cnp); return (0); } @@ -516,6 +573,7 @@ static int hook_link(struct vop_link_args *ap) { + printf("hook_link: %s\n", ap->a_cnp->cn_nameptr); hook_generic_create(ap->a_tdvp, ap->a_vp, ap->a_cnp); return (0); } @@ -523,6 +581,7 @@ static int hook_symlink(struct vop_symlink_args *ap) { + printf("hook_symlink: %s\n", ap->a_cnp->cn_nameptr); hook_generic_create(ap->a_dvp, *ap->a_vpp, ap->a_cnp); return (0); } @@ -557,6 +616,7 @@ } fnode = node_lookupex(ap->a_fvp, NULL, 0); if (fnode != NULL) { + mtx_unlock(&fnode->nd_mtx); /* TODO */ /* mark path stale */ } @@ -571,13 +631,6 @@ return (0); } -static void -watch_tryfree(struct fnwatch *watch) -{ - if (watch->wt_session == NULL && watch->wt_node == NULL) - free(watch, M_FSNOTIFY); -} - static int watch_nextwd(void) { @@ -595,26 +648,99 @@ return (nwd); } +static void +watch_detachnode(struct fnwatch *watch) +{ + struct fnnode *node = watch->wt_node; + + mtx_assert(&node->nd_mtx, MA_OWNED); + + TAILQ_REMOVE(&node->nd_watchlist, watch, wt_nodeentry); + node->nd_watchcount--; + watch->wt_node = NULL; + MPASS(watch->wt_session != NULL); +} + +static void +watch_free(struct fnwatch *watch) +{ + struct fnsession *ss; + struct fnnode *node; + struct fneventhandle *eh; + + ss = watch->wt_session; + node = watch->wt_node; + printf("watch_free: watch %p: session %p node %p\n", watch, ss, node); + mtx_assert(&ss->ss_mtx, MA_OWNED); + + if (node != NULL) { + mtx_lock(&node->nd_mtx); + if (watch->wt_node != NULL) { + MPASS(watch->wt_node == node); + watch_detachnode(watch); + node_watchdrop(node); + } else + mtx_unlock(&node->nd_mtx); + } + + TAILQ_REMOVE(&ss->ss_watchlist, watch, wt_sessionentry); + watch->wt_session = NULL; + + TAILQ_FOREACH(eh, &ss->ss_queue, eh_queueentry) { + MPASS(eh->eh_watch != watch); + } + + MPASS(watch->wt_session == NULL && watch->wt_node == NULL); + printf("watch_free: free %p\n", watch); + free(watch, M_FSNOTIFY); +} + static __inline void node_hold(struct fnnode *node) { refcount_acquire(&node->nd_refcnt); } +static __inline void +node_watchhold(struct fnnode *node) +{ + mtx_assert(&node->nd_mtx, MA_OWNED); + node_hold(node); + if (TAILQ_EMPTY(&node->nd_watchlist)) + node_hold(node); +} + +static __inline void +node_watchdrop(struct fnnode *node) +{ + mtx_assert(&node->nd_mtx, MA_OWNED); + if (TAILQ_EMPTY(&node->nd_watchlist)) { + MPASS(node->nd_watchcount == 0); + node->nd_supermask = 0; + mtx_unlock(&node->nd_mtx); + node_drop(node); + } else + mtx_unlock(&node->nd_mtx); +} + static void node_drop(struct fnnode *node) { + mtx_assert(&node->nd_mtx, MA_NOTOWNED); if (refcount_release(&node->nd_refcnt) != 0) { - KASSERT(node->nd_watchcount == 0 && node->nd_supermask == 0 && - TAILQ_EMPTY(&node->nd_watchlist), ("Invalid reference count")); + MPASS(node->nd_vnode == NULL); + KASSERT(node->nd_watchcount == 0 && + TAILQ_EMPTY(&node->nd_watchlist), + ("Invalid watch count: %d", node->nd_watchcount)); if (node->nd_ino != 0) { mtx_lock(&fnnode_hashmtx); LIST_REMOVE(node, nd_hashentry); mtx_unlock(&fnnode_hashmtx); } mtx_destroy(&node->nd_mtx); - free(node->nd_path, M_TEMP); + free(node->nd_pathfree, M_TEMP); free(node, M_FSNOTIFY); + printf("node free: %p\n", node); } } @@ -636,14 +762,23 @@ MPASS(ino != 0); node = malloc(sizeof(struct fnnode), M_FSNOTIFY, M_WAITOK | M_ZERO); + printf("node alloc: node %p vp %p\n", node, vp); - refcount_init(&node->nd_refcnt, 1); + refcount_init(&node->nd_refcnt, 2); mtx_init(&node->nd_mtx, "fsnotify_node", NULL, MTX_DEF); TAILQ_INIT(&node->nd_watchlist); node->nd_ino = ino; + mtx_lock(&node->nd_mtx); + + mtx_lock(&fnnode_hashmtx); + LIST_INSERT_HEAD(node_inohashhead(vp->v_mount, ino), + node, nd_hashentry); + mtx_unlock(&fnnode_hashmtx); + VI_LOCK(vp); + node->nd_vnode = vp; vp->v_fnnode = node; VI_UNLOCK(vp); @@ -651,19 +786,21 @@ } static void -node_detachwatches(struct fnnode *node) +node_detachallwatches(struct fnnode *node) { - struct fnwatch *watch; + struct fnwatch *watch = NULL; mtx_assert(&node->nd_mtx, MA_OWNED); - node->nd_watchcount = 0; - node->nd_supermask = 0; while (!TAILQ_EMPTY(&node->nd_watchlist)) { watch = TAILQ_FIRST(&node->nd_watchlist); - TAILQ_REMOVE(&node->nd_watchlist, watch, wt_nodeentry); - watch->wt_node = NULL; - watch_tryfree(watch); + watch_detachnode(watch); } + node->nd_supermask = 0; + MPASS(node->nd_watchcount == 0); + if (watch != NULL) + node_watchdrop(node); + else + mtx_unlock(&node->nd_mtx); } static int @@ -696,7 +833,7 @@ { struct fnnode *node, *rv; ino_t ino; - int error, watchcount; + int error; rv = NULL; VI_LOCK(vp); @@ -708,10 +845,11 @@ goto done; } else if (node != NULL) { mtx_lock(&node->nd_mtx); - watchcount = node->nd_watchcount; - mtx_unlock(&node->nd_mtx); - if (watchcount == 0) + MPASS(node->nd_vnode == vp); + if (node->nd_watchcount == 0) { + mtx_unlock(&node->nd_mtx); node = NULL; + } goto done; } @@ -729,18 +867,21 @@ node->nd_mount != vp->v_mount) continue; mtx_lock(&node->nd_mtx); + mtx_unlock(&fnnode_hashmtx); VI_LOCK(vp); if (!NODE_ISVALID(vp->v_fnnode)) { + MPASS(node->nd_vnode == NULL); node_hold(node); + printf("node lookup: ref vnode: node %p vp %p\n", node, vp); vp->v_fnnode = node; + node->nd_vnode = vp; } else - MPASS(vp->v_fnnode == node); + MPASS(vp->v_fnnode == node && vp == node->nd_vnode); VI_UNLOCK(vp); - watchcount = node->nd_watchcount; - mtx_unlock(&node->nd_mtx); - if (watchcount == 0) + if (node->nd_watchcount == 0) { + mtx_unlock(&node->nd_mtx); node = NULL; - mtx_unlock(&fnnode_hashmtx); + } goto done; } mtx_unlock(&fnnode_hashmtx); @@ -751,6 +892,8 @@ VI_UNLOCK(vp); done: + if (node != NULL) + mtx_assert(&node->nd_mtx, MA_OWNED); return (node); } @@ -776,6 +919,8 @@ * thread */ vp = node->nd_vnode; + printf("node_updatepath: node %p vp %p %s\n", + node, vp, node->nd_path); if ((vp->v_iflag & VI_DOOMED) != 0) return (ENOENT); @@ -845,6 +990,7 @@ event->ev_pathpos = MAXPATHLEN - 1 - namelen; memcpy(event->ev_pathfree + event->ev_pathpos, name, namelen); event->ev_pathfree[MAXPATHLEN - 1] = '\0'; + printf("event alloc: %p\n", event); return (event); } @@ -855,6 +1001,7 @@ node_drop(event->ev_node); uma_zfree(namei_zone, event->ev_pathfree); free(event, M_FSNOTIFY); + printf("event free: %p\n", event); } static __inline int @@ -866,8 +1013,9 @@ static __inline void event_copypath(struct fnevent *event, char *path, int *pathlen) { - *pathlen = event_pathlen(event); - memcpy(path, event->ev_pathfree + *pathlen, *pathlen); + /* Count last zero byte */ + *pathlen = event_pathlen(event) + 1; + memcpy(path, event->ev_pathfree + event->ev_pathpos, *pathlen); } static void @@ -921,12 +1069,14 @@ { struct fnwatch *watch; + printf("session_addwatch: %s: session=%p\n", node->nd_path, ss); + watch = malloc(sizeof(struct fnwatch), M_FSNOTIFY, M_WAITOK | M_ZERO); + printf("watch alloc: %p\n", watch); watch->wt_wd = watch_nextwd(); watch->wt_mask = mask; watch->wt_session = ss; - node_hold(node); watch->wt_node = node; mtx_lock(&ss->ss_mtx); @@ -948,15 +1098,19 @@ static int session_rmwatch(struct fnsession *ss, int wd) { - struct fnwatch *watch, *tmp; + struct fnwatch *watch; + struct fneventhandle *eh, *ehtmp; mtx_lock(&ss->ss_mtx); - TAILQ_FOREACH_SAFE(watch, &ss->ss_watchlist, wt_sessionentry, tmp) { + TAILQ_FOREACH(watch, &ss->ss_watchlist, wt_sessionentry) { if (watch->wt_wd != wd) continue; - TAILQ_REMOVE(&ss->ss_watchlist, watch, wt_sessionentry); - watch->wt_session = NULL; - watch_tryfree(watch); + TAILQ_FOREACH_SAFE(eh, &ss->ss_queue, eh_queueentry, ehtmp) { + if (eh->eh_watch != watch) + continue; + session_drophandle(ss, eh); + } + watch_free(watch); break; } mtx_unlock(&ss->ss_mtx); @@ -981,15 +1135,12 @@ ss = eh->eh_watch->wt_session; - if (ss == NULL) + if (ss == NULL) { + eventhandle_drop(eh); return; + } mtx_lock(&ss->ss_mtx); - if (eh->eh_watch->wt_session != NULL) { - mtx_unlock(&ss->ss_mtx); - eventhandle_drop(eh); - return; - } TAILQ_INSERT_TAIL(&ss->ss_queue, eh, eh_queueentry); ss->ss_queuesize++; mtx_unlock(&ss->ss_mtx); @@ -1004,7 +1155,6 @@ struct fnwatch *watch; struct fnevent *event; struct fneventhandle *eh; - struct vnode *vp; int i, handle_count; while (1) { @@ -1028,6 +1178,8 @@ event->ev_handlemaxsize); break; } + printf("process_queue: handle event %p in session %p\n", + event, watch->wt_session); eh = &event->ev_handlebuf[event->ev_handlecount++]; eh->eh_event = event; eh->eh_watch = watch; @@ -1039,17 +1191,18 @@ event_free(event); continue; } - vp = node->nd_vnode; - if (vp != NULL) + /* FIXME */ + if (0 && node->nd_vnode != NULL) node_updatepath(node); else printf("fsnotify: vnode not found, reusing cached path: %s\n", node->nd_path); + event_prependpath(event, node); + if (event->ev_mask & FE_DESTROY) - node_detachwatches(node); - - event_prependpath(event, node); - mtx_unlock(&node->nd_mtx); + node_detachallwatches(node); + else + mtx_unlock(&node->nd_mtx); for (i = 0; i < handle_count; i++) session_enqueue(&event->ev_handlebuf[i]); @@ -1060,12 +1213,12 @@ enqueue_direvent(struct fnnode *dirnode, struct componentname *cnp, int cookie, int mask) { struct fnevent *event; - int supermask, watch_count; + int supermask, watchcount; printf("enqueue_direvent: %s %x\n", cnp->cn_nameptr, mask); mtx_assert(&dirnode->nd_mtx, MA_OWNED); - watch_count = dirnode->nd_watchcount; + watchcount = dirnode->nd_watchcount; supermask = dirnode->nd_supermask & mask; node_hold(dirnode); mtx_unlock(&dirnode->nd_mtx); @@ -1075,10 +1228,10 @@ return; } - KASSERT(watch_count > 0, ("No watchers found")); + KASSERT(watchcount > 0, ("No watchers found")); event = event_alloc(dirnode, cnp->cn_nameptr, cnp->cn_namelen, - watch_count + 1, mask, cookie); + watchcount + 1, mask, cookie); mtx_lock(&fsnotify_queue_mtx); TAILQ_INSERT_TAIL(&fsnotify_queue, event, ev_queueentry);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201007052006.o65K6DTk060411>