kernel - TMPFS - Bug fixing pass - directory hierarchy
authorMatthew Dillon <dillon@apollo.backplane.com>
Sat, 13 Feb 2010 06:16:35 +0000 (22:16 -0800)
committerMatthew Dillon <dillon@apollo.backplane.com>
Sat, 13 Feb 2010 06:16:35 +0000 (22:16 -0800)
* Adjust parent linkages for directories.  The root node's parent
  linkage is now set to NULL instead of a self-reference.  The
  root node is given just one extra link count to hold it instead
  of two.

* List operations on tmpfs_mount->tn_entries are now protected
  by TMPFS_LOCK.

* Hold a node locked when calling tmpfs_free_node().  The function
  will eat the lock.

* Do a better job cleaning up dirent and node structures when freeing.
  The ctor function is only called on backing store allocations, not
  on frontend objcache allocations.

  Misinitialized previously freed structures created some amount of
  havoc.

* Remove unnecessary critical sections.

* Refactor the umount code to properly clean up all nodes, and in
  the correct order.  Remove tmpfs_free_node() hacks that tried
  to allow out-of-order removal.

sys/vfs/tmpfs/tmpfs.h
sys/vfs/tmpfs/tmpfs_subr.c
sys/vfs/tmpfs/tmpfs_vfsops.c
sys/vfs/tmpfs/tmpfs_vnops.c

index 22b3e57..647c0dd 100644 (file)
@@ -425,8 +425,7 @@ int tmpfs_alloc_node(struct tmpfs_mount *, enum vtype,
 void   tmpfs_free_node(struct tmpfs_mount *, struct tmpfs_node *);
 int    tmpfs_alloc_dirent(struct tmpfs_mount *, struct tmpfs_node *,
            const char *, uint16_t, struct tmpfs_dirent **);
-void   tmpfs_free_dirent(struct tmpfs_mount *, struct tmpfs_dirent *,
-           boolean_t);
+void   tmpfs_free_dirent(struct tmpfs_mount *, struct tmpfs_dirent *);
 int    tmpfs_alloc_vp(struct mount *, struct tmpfs_node *, int,
            struct vnode **);
 void   tmpfs_free_vp(struct vnode *);
@@ -453,6 +452,7 @@ void        tmpfs_itimes(struct vnode *, const struct timespec *,
 
 void   tmpfs_update(struct vnode *);
 int    tmpfs_truncate(struct vnode *, off_t);
+int    tmpfs_node_ctor(void *obj, void *privdata, int flags);
 
 /* --------------------------------------------------------------------- */
 
index fa4968a..8e7005b 100644 (file)
@@ -134,14 +134,16 @@ tmpfs_alloc_node(struct tmpfs_mount *tmp, enum vtype type,
                TAILQ_INIT(&nnode->tn_dir.tn_dirhead);
                KKASSERT(parent != nnode);
                KKASSERT(IMPLIES(parent == NULL, tmp->tm_root == NULL));
-               nnode->tn_dir.tn_parent = (parent == NULL) ? nnode : parent;
+               nnode->tn_dir.tn_parent = parent;
                nnode->tn_dir.tn_readdir_lastn = 0;
                nnode->tn_dir.tn_readdir_lastp = NULL;
                nnode->tn_links++;
                nnode->tn_size = 0;
-               TMPFS_NODE_LOCK(nnode->tn_dir.tn_parent);
-               nnode->tn_dir.tn_parent->tn_links++;
-               TMPFS_NODE_UNLOCK(nnode->tn_dir.tn_parent);
+               if (parent) {
+                       TMPFS_NODE_LOCK(parent);
+                       parent->tn_links++;
+                       TMPFS_NODE_UNLOCK(parent);
+               }
                break;
 
        case VFIFO:
@@ -170,8 +172,8 @@ tmpfs_alloc_node(struct tmpfs_mount *tmp, enum vtype type,
        }
 
        TMPFS_NODE_LOCK(nnode);
-       LIST_INSERT_HEAD(&tmp->tm_nodes_used, nnode, tn_entries);
        TMPFS_LOCK(tmp);
+       LIST_INSERT_HEAD(&tmp->tm_nodes_used, nnode, tn_entries);
        tmp->tm_nodes_inuse++;
        TMPFS_UNLOCK(tmp);
        TMPFS_NODE_UNLOCK(nnode);
@@ -204,18 +206,17 @@ tmpfs_free_node(struct tmpfs_mount *tmp, struct tmpfs_node *node)
 {
        size_t pages = 0;
 
-       TMPFS_NODE_LOCK(node);
-
 #ifdef INVARIANTS
        TMPFS_ASSERT_ELOCKED(node);
        KKASSERT(node->tn_vnode == NULL);
        KKASSERT((node->tn_vpstate & TMPFS_VNODE_ALLOCATING) == 0);
 #endif
 
-       LIST_REMOVE(node, tn_entries);
        TMPFS_LOCK(tmp);
+       LIST_REMOVE(node, tn_entries);
        tmp->tm_nodes_inuse--;
        TMPFS_UNLOCK(tmp);
+       TMPFS_NODE_UNLOCK(node);
 
        switch (node->tn_type) {
        case VNON:
@@ -229,12 +230,38 @@ tmpfs_free_node(struct tmpfs_mount *tmp, struct tmpfs_node *node)
                /* FALLTHROUGH */
                break;
        case VDIR:
+               /*
+                * The parent link can be NULL if this is the root
+                * node.
+                */
                node->tn_links--;
                node->tn_size = 0;
-               KKASSERT(node->tn_dir.tn_parent != NULL);
-               TMPFS_NODE_LOCK(node->tn_dir.tn_parent);
-               node->tn_dir.tn_parent->tn_links--;
-               TMPFS_NODE_UNLOCK(node->tn_dir.tn_parent);
+               KKASSERT(node->tn_dir.tn_parent || node == tmp->tm_root);
+               if (node->tn_dir.tn_parent) {
+                       TMPFS_NODE_LOCK(node->tn_dir.tn_parent);
+                       node->tn_dir.tn_parent->tn_links--;
+
+                       /*
+                        * If the parent directory has no more links and
+                        * no vnode ref nothing is going to come along
+                        * and clean it up unless we do it here.
+                        */
+                       if (node->tn_dir.tn_parent->tn_links == 0 &&
+                           node->tn_dir.tn_parent->tn_vnode == NULL) {
+                               tmpfs_free_node(tmp, node->tn_dir.tn_parent);
+                               /* eats parent lock */
+                       } else {
+                               TMPFS_NODE_UNLOCK(node->tn_dir.tn_parent);
+                       }
+                       node->tn_dir.tn_parent = NULL;
+               }
+
+               /*
+                * If the root node is being destroyed don't leave a
+                * dangling pointer in tmpfs_mount.
+                */
+               if (node == tmp->tm_root)
+                       tmp->tm_root = NULL;
                break;
        case VFIFO:
                /* FALLTHROUGH */
@@ -257,12 +284,17 @@ tmpfs_free_node(struct tmpfs_mount *tmp, struct tmpfs_node *node)
                panic("tmpfs_free_node: type %p %d", node, (int)node->tn_type);
        }
 
+       /*
+        * Clean up fields for the next allocation.  The objcache only ctors
+        * new allocations.
+        */
+       tmpfs_node_ctor(node, NULL, 0);
        objcache_put(tmp->tm_node_pool, node);
+       /* node is now invalid */
 
        TMPFS_LOCK(tmp);
        tmp->tm_pages_used -= pages;
        TMPFS_UNLOCK(tmp);
-       TMPFS_NODE_UNLOCK(node);
 }
 
 /* --------------------------------------------------------------------- */
@@ -312,23 +344,22 @@ tmpfs_alloc_dirent(struct tmpfs_mount *tmp, struct tmpfs_node *node,
  * directory entry, as it may already have been released from the outside.
  */
 void
-tmpfs_free_dirent(struct tmpfs_mount *tmp, struct tmpfs_dirent *de,
-    boolean_t node_exists)
+tmpfs_free_dirent(struct tmpfs_mount *tmp, struct tmpfs_dirent *de)
 {
-       if (node_exists) {
-               struct tmpfs_node *node;
+       struct tmpfs_node *node;
 
-               node = de->td_node;
+       node = de->td_node;
 
-               TMPFS_NODE_LOCK(node);
-               TMPFS_ASSERT_ELOCKED(node);
-               KKASSERT(node->tn_links > 0);
-               node->tn_links--;
-               TMPFS_NODE_UNLOCK(node);
-       }
+       TMPFS_NODE_LOCK(node);
+       TMPFS_ASSERT_ELOCKED(node);
+       KKASSERT(node->tn_links > 0);
+       node->tn_links--;
+       TMPFS_NODE_UNLOCK(node);
 
        kfree(de->td_name, M_TMPFSNAME);
+       de->td_namelen = 0;
        de->td_name = NULL;
+       de->td_node = NULL;
        objcache_put(tmp->tm_dirent_pool, de);
 }
 
@@ -367,8 +398,10 @@ loop:
                goto out;
        }
 
-       if ((node->tn_vpstate & TMPFS_VNODE_DOOMED) ||
-           (node->tn_type == VDIR && node->tn_dir.tn_parent == NULL)) {
+       /*
+        * This should never happen.
+        */
+       if (node->tn_vpstate & TMPFS_VNODE_DOOMED) {
                TMPFS_NODE_UNLOCK(node);
                error = ENOENT;
                vp = NULL;
@@ -422,7 +455,6 @@ loop:
                break;
        case VDIR:
                vinitvmio(vp, node->tn_size);
-               KKASSERT(node->tn_dir.tn_parent != NULL);
                break;
 
        default:
@@ -526,20 +558,22 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap,
            dnode->tn_gid, vap->va_mode, parent, target, vap->va_rmajor, vap->va_rminor, &node);
        if (error != 0)
                return error;
+       TMPFS_NODE_LOCK(node);
 
        /* Allocate a directory entry that points to the new file. */
-       error = tmpfs_alloc_dirent(tmp, node, ncp->nc_name, ncp->nc_nlen,
-           &de);
+       error = tmpfs_alloc_dirent(tmp, node, ncp->nc_name, ncp->nc_nlen, &de);
        if (error != 0) {
                tmpfs_free_node(tmp, node);
+               /* eats node lock */
                return error;
        }
 
        /* Allocate a vnode for the new file. */
        error = tmpfs_alloc_vp(dvp->v_mount, node, LK_EXCLUSIVE, vpp);
        if (error != 0) {
-               tmpfs_free_dirent(tmp, de, TRUE);
+               tmpfs_free_dirent(tmp, de);
                tmpfs_free_node(tmp, node);
+               /* eats node lock */
                return error;
        }
 
@@ -547,6 +581,7 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap,
         * insert the new node into the directory, an operation that
         * cannot fail. */
        tmpfs_dir_attach(dvp, de);
+       TMPFS_NODE_UNLOCK(node);
 
        return error;
 }
@@ -565,9 +600,7 @@ tmpfs_dir_attach(struct vnode *vp, struct tmpfs_dirent *de)
 
        dnode = VP_TO_TMPFS_DIR(vp);
 
-       crit_enter();
        TAILQ_INSERT_TAIL(&dnode->tn_dir.tn_dirhead, de, td_entries);
-       crit_exit();
 
        TMPFS_NODE_LOCK(dnode);
        TMPFS_ASSERT_ELOCKED(dnode);
@@ -592,14 +625,11 @@ tmpfs_dir_detach(struct vnode *vp, struct tmpfs_dirent *de)
        dnode = VP_TO_TMPFS_DIR(vp);
 
 
-       crit_enter();
        if (dnode->tn_dir.tn_readdir_lastp == de) {
                dnode->tn_dir.tn_readdir_lastn = 0;
                dnode->tn_dir.tn_readdir_lastp = NULL;
        }
-
        TAILQ_REMOVE(&dnode->tn_dir.tn_dirhead, de, td_entries);
-       crit_exit();
 
        TMPFS_NODE_LOCK(dnode);
        TMPFS_ASSERT_ELOCKED(dnode);
index f3d3d03..ce0165d 100644 (file)
@@ -125,7 +125,7 @@ get_swpgtotal(void)
 }
 
 /* --------------------------------------------------------------------- */
-static int
+int
 tmpfs_node_ctor(void *obj, void *privdata, int flags)
 {
        struct tmpfs_node *node = (struct tmpfs_node *)obj;
@@ -137,6 +137,7 @@ tmpfs_node_ctor(void *obj, void *privdata, int flags)
        node->tn_links = 0;
        node->tn_vnode = NULL;
        node->tn_vpstate = TMPFS_VNODE_WANT;
+       bzero(&node->tn_spec, sizeof(node->tn_spec));
 
        return (1);
 }
@@ -304,9 +305,9 @@ tmpfs_unmount(struct mount *mp, int mntflags)
 {
        int error;
        int flags = 0;
+       int found;
        struct tmpfs_mount *tmp;
        struct tmpfs_node *node;
-       struct vnode *vp;
 
        /* Handle forced unmounts. */
        if (mntflags & MNT_FORCE)
@@ -321,40 +322,69 @@ tmpfs_unmount(struct mount *mp, int mntflags)
        if (error != 0)
                return error;
 
-       /* Free all associated data.  The loop iterates over the linked list
-        * we have containing all used nodes.  For each of them that is
-        * a directory, we free all its directory entries.  Note that after
-        * freeing a node, it will automatically go to the available list,
-        * so we will later have to iterate over it to release its items. */
-       node = LIST_FIRST(&tmp->tm_nodes_used);
-       while (node != NULL) {
-               struct tmpfs_node *next;
-
+       /*
+        * First pass get rid of all the directory entries and
+        * vnode associations.  The directory structure will
+        * remain via the extra link count representing tn_dir.tn_parent.
+        *
+        * No vnodes should remain after the vflush above.
+        */
+       LIST_FOREACH(node, &tmp->tm_nodes_used, tn_entries) {
+               ++node->tn_links;
+               TMPFS_NODE_LOCK(node);
                if (node->tn_type == VDIR) {
                        struct tmpfs_dirent *de;
 
-                       de = TAILQ_FIRST(&node->tn_dir.tn_dirhead);
-                       while (de != NULL) {
-                               struct tmpfs_dirent *nde;
-
-                               nde = TAILQ_NEXT(de, td_entries);
-                               tmpfs_free_dirent(tmp, de, FALSE);
-                               de = nde;
+                       while (!TAILQ_EMPTY(&node->tn_dir.tn_dirhead)) {
+                               de = TAILQ_FIRST(&node->tn_dir.tn_dirhead);
+                               tmpfs_free_dirent(tmp, de);
                                node->tn_size -= sizeof(struct tmpfs_dirent);
                        }
                }
-
-               next = LIST_NEXT(node, tn_entries);
+               KKASSERT(node->tn_vnode == NULL);
+#if 0
                vp = node->tn_vnode;
                if (vp != NULL) {
                        tmpfs_free_vp(vp);
                        vrecycle(vp);
-                        node->tn_vnode = NULL;
+                       node->tn_vnode = NULL;
                }
-               tmpfs_free_node(tmp, node);
-               node = next;
+#endif
+               TMPFS_NODE_UNLOCK(node);
+               --node->tn_links;
        }
 
+       /*
+        * Now get rid of all nodes.  We can remove any node with a
+        * link count of 0 or any directory node with a link count of
+        * 1.  The parents will not be destroyed until all their children
+        * have been destroyed.
+        *
+        * Recursion in tmpfs_free_node() can further modify the list so
+        * we cannot use a next pointer here.
+        *
+        * The root node will be destroyed by this loop (it will be last).
+        */
+       while (!LIST_EMPTY(&tmp->tm_nodes_used)) {
+               found = 0;
+               LIST_FOREACH(node, &tmp->tm_nodes_used, tn_entries) {
+                       if (node->tn_links == 0 ||
+                           (node->tn_links == 1 && node->tn_type == VDIR)) {
+                               TMPFS_NODE_LOCK(node);
+                               tmpfs_free_node(tmp, node);
+                               /* eats lock */
+                               found = 1;
+                               break;
+                       }
+               }
+               if (found == 0) {
+                       kprintf("tmpfs: Cannot free entire node tree!");
+                       break;
+               }
+       }
+
+       KKASSERT(tmp->tm_root == NULL);
+
        objcache_destroy(tmp->tm_dirent_pool);
        objcache_destroy(tmp->tm_node_pool);
 
@@ -363,7 +393,7 @@ tmpfs_unmount(struct mount *mp, int mntflags)
        KKASSERT(tmp->tm_nodes_inuse == 0);
 
        /* Throw away the tmpfs_mount structure. */
-       kfree(mp->mnt_data, M_TMPFSMNT);
+       kfree(tmp, M_TMPFSMNT);
        mp->mnt_data = NULL;
 
        mp->mnt_flag &= ~MNT_LOCAL;
@@ -375,11 +405,20 @@ tmpfs_unmount(struct mount *mp, int mntflags)
 static int
 tmpfs_root(struct mount *mp, struct vnode **vpp)
 {
+       struct tmpfs_mount *tmp;
        int error;
-       error = tmpfs_alloc_vp(mp, VFS_TO_TMPFS(mp)->tm_root, LK_EXCLUSIVE, vpp);
-       (*vpp)->v_flag |= VROOT;
-       (*vpp)->v_type = VDIR;
 
+       tmp = VFS_TO_TMPFS(mp);
+       if (tmp->tm_root == NULL) {
+               kprintf("tmpfs_root: called without root node %p\n", mp);
+               print_backtrace();
+               *vpp = NULL;
+               error = EINVAL;
+       } else {
+               error = tmpfs_alloc_vp(mp, tmp->tm_root, LK_EXCLUSIVE, vpp);
+               (*vpp)->v_flag |= VROOT;
+               (*vpp)->v_type = VDIR;
+       }
        return error;
 }
 
index a78fee5..59c7cd8 100644 (file)
@@ -631,7 +631,6 @@ tmpfs_write (struct vop_write_args *ap)
 #else
                got_mplock = -1;
 #endif
-       crit_enter();
        while (uio->uio_resid > 0) {
                /*
                 * Use buffer cache I/O (via tmpfs_strategy)
@@ -694,7 +693,6 @@ tmpfs_write (struct vop_write_args *ap)
                        break;
                }
        }
-       crit_exit();
 
        if (got_mplock > 0)
                rel_mplock();
@@ -827,7 +825,7 @@ tmpfs_nremove(struct vop_nremove_args *v)
        /* Free the directory entry we just deleted.  Note that the node
         * referred by it will not be removed until the vnode is really
         * reclaimed. */
-       tmpfs_free_dirent(tmp, de, TRUE);
+       tmpfs_free_dirent(tmp, de);
 
        if (node->tn_links > 0) {
                TMPFS_NODE_LOCK(node);
@@ -1138,7 +1136,7 @@ tmpfs_nrename(struct vop_nrename_args *v)
                /* Free the directory entry we just deleted.  Note that the
                 * node referred by it will not be removed until the vnode is
                 * really reclaimed. */
-               tmpfs_free_dirent(VFS_TO_TMPFS(tvp->v_mount), de, TRUE);
+               tmpfs_free_dirent(VFS_TO_TMPFS(tvp->v_mount), de);
                /*cache_inval_vp(tvp, CINV_DESTROY);*/
        }
 
@@ -1259,12 +1257,20 @@ tmpfs_nrmdir(struct vop_nrmdir_args *v)
        TMPFS_NODE_LOCK(dnode);
        TMPFS_ASSERT_ELOCKED(dnode);
 
+#if 0
+       /* handled by tmpfs_free_node */
+       KKASSERT(node->tn_links > 0);
        node->tn_links--;
        node->tn_dir.tn_parent = NULL;
+#endif
        node->tn_status |= TMPFS_NODE_ACCESSED | TMPFS_NODE_CHANGED | \
            TMPFS_NODE_MODIFIED;
 
+#if 0
+       /* handled by tmpfs_free_node */
+       KKASSERT(dnode->tn_links > 0);
        dnode->tn_links--;
+#endif
        dnode->tn_status |= TMPFS_NODE_ACCESSED | \
            TMPFS_NODE_CHANGED | TMPFS_NODE_MODIFIED;
 
@@ -1274,7 +1280,7 @@ tmpfs_nrmdir(struct vop_nrmdir_args *v)
        /* Free the directory entry we just deleted.  Note that the node
         * referred by it will not be removed until the vnode is really
         * reclaimed. */
-       tmpfs_free_dirent(tmp, de, TRUE);
+       tmpfs_free_dirent(tmp, de);
 
        /* Release the deleted vnode (will destroy the node, notify
         * interested parties and clean it from the cache). */
@@ -1487,9 +1493,8 @@ tmpfs_reclaim(struct vop_reclaim_args *v)
        if (node->tn_links == 0 &&
            (node->tn_vpstate & TMPFS_VNODE_ALLOCATING) == 0) {
                node->tn_vpstate = TMPFS_VNODE_DOOMED;
-               TMPFS_NODE_UNLOCK(node);
-               kprintf("reclaim\n");
                tmpfs_free_node(tmp, node);
+               /* eats the lock */
        } else {
                TMPFS_NODE_UNLOCK(node);
        }