Uses kref in the FS, fixes up refcount bugs
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 5 Aug 2010 02:52:35 +0000 (19:52 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:50 +0000 (17:35 -0700)
References are still a bit messy, esp regarding the sb and vfsmounts.
Also, KFS "pins all paths," which makes it difficult to check the dentry
and inode release paths.  Lots of TODOs left.

Also, this changes f_inode to f_dentry for reasons intuitively obvious
to the casual observer.

Documentation/vfs.txt
kern/include/kref.h
kern/include/vfs.h
kern/src/elf.c
kern/src/kfs.c
kern/src/manager.c
kern/src/mm.c
kern/src/monitor.c
kern/src/process.c
kern/src/syscall.c
kern/src/vfs.c

index 4d1ee48..bc810ad 100644 (file)
@@ -205,3 +205,40 @@ pain.
 This also applies to trying to write to a block beyond the EOF.  If the request
 hits the page cache and readpage(), it's because it was already checked and
 cleared in another part of the VFS, such as in generic_file_write().
+
+Kref, Dentries, Inodes, and Files (or "I don't see why it's like X, but..."
+--------------------------
+There are multiple dentries pointing to an inode.  The dentries are (or will be)
+cached too, but that is irrelevant.  The dentries pin the inodes in memory.
+However, files pin inodes in memory (or they did) briefly.  After running around
+in circles a bit, I asked, why doesn't the file pin the dentry instead of the
+inode?  The answer: it is supposed to.  Linux does that, and I didn't because
+pinning the inode made more sense at the time.
+
+The heart of the issue is about understanding what files are and how they
+relate to the rest of the VFS.  A 'file' in the OS is a structure to track an
+open FS-disk-file, which is managed by the inode.  Given this, it makes sense
+that a dentry (which is a name on a path) would be pinned by the corresponding
+inode, and the file would pin the inode.  It doesn't, but it is believable.  In
+reality, there are many names (dentries) for a given disk file, and the OS file
+that you open corresponds to *one* of those names, and thus a dentry, and not to
+the inode/specific file.  You need to go through the dentry to pin the inode.
+
+In short, it's not: file -> inode -> dentry -> parent_dentry -> ...
+It's file -> dentry -> parent_dentry ->
+             |-> inode      |-> parent_inode
+Another dentry and file (both OS and disk) can point to the same inode.  If you
+don't do it this way, you can't pin up past the multi-dentry-point in the inode,
+and your system doesn't really make sense.
+
+So here is how it works: files pin dentries.  Dentries can pin other dentries,
+on up the directory hierarchy.  Independently of the files, dentries pin their
+inode.  There are many dentries per inode (or can be).  Since each dentry
+doesn't know if it is the last dentry to decref the inode, we use a kref on
+i_kref.  The inodes are storing references to the dentries, but they are the
+kref "internal" / weak references.  Even if we did decref them, we don't trigger
+anything with it.
+
+The moral of the story is that if you don't fully understand something, you are
+not in as good of a position to recommend changes or criticize as if you did
+your homework.  Not that you can't, just that you should 'do your homework.'
index 27fe63b..1817a6d 100644 (file)
@@ -25,6 +25,12 @@ struct kref {
        void (*release)(struct kref *kref);
 };
 
+/* Helper for some debugging situations */
+static long kref_refcnt(struct kref *kref)
+{
+       return atomic_read(&kref->refcount);
+}
+
 static void kref_init(struct kref *kref, void (*release)(struct kref *kref),
                       unsigned int init)
 {
@@ -51,7 +57,7 @@ static struct kref *kref_get_not_zero(struct kref *kref, unsigned int inc)
 /* Returns True if we hit 0 and executed 'release', False otherwise */
 static bool kref_put(struct kref *kref)
 {
-       assert(atomic_read(&kref->refcount) > 0);               /* catch some bugs */
+       assert(kref_refcnt(kref) > 0);          /* catch some bugs */
        if (atomic_sub_and_test(&kref->refcount, 1)) {
                kref->release(kref);
                return TRUE;
@@ -59,4 +65,11 @@ static bool kref_put(struct kref *kref)
        return FALSE;
 }
 
+/* Dev / debugging function to catch the attempted freeing of objects we don't
+ * know how to free yet. */
+static void fake_release(struct kref *kref)
+{
+       panic("Cleaning up this object is not supported!\n");
+}
+
 #endif /* ROS_KERN_KREF_H */
index 12cb53c..3398c44 100644 (file)
@@ -16,7 +16,7 @@
 #include <ros/common.h>
 #include <sys/queue.h>
 #include <arch/bitmask.h>
-#include <atomic.h>
+#include <kref.h>
 #include <timing.h>
 #include <radix.h>
 
@@ -176,7 +176,7 @@ struct super_block {
        unsigned long                           s_magic;
        struct vfsmount                         *s_mount;               /* vfsmount point */
        spinlock_t                                      s_lock;                 /* used for all sync */
-       atomic_t                                        s_refcnt;
+       struct kref                                     s_kref;
        bool                                            s_syncing;              /* currently syncing metadata */
        struct inode_tailq                      s_inodes;               /* all inodes */
        struct inode_tailq                      s_dirty_i;              /* dirty inodes */
@@ -215,7 +215,7 @@ struct inode {
        TAILQ_ENTRY(inode)                      i_list;                 /* describes state (dirty) */
        struct dentry_tailq                     i_dentry;               /* all dentries pointing here*/
        unsigned long                           i_ino;
-       atomic_t                                        i_refcnt;
+       struct kref                                     i_kref;
        int                                                     i_mode;                 /* access mode */
        unsigned short                          i_type;                 /* file type */
        unsigned int                            i_nlink;                /* hard links */
@@ -275,7 +275,7 @@ struct inode_operations {
  * requests quickly.  If none of these, dealloc it back to the slab cache.
  * Unused and negatives go in the LRU list. */
 struct dentry {
-       atomic_t                                        d_refcnt;               /* don't discard when 0 */
+       struct kref                                     d_kref;                 /* don't discard when 0 */
        unsigned long                           d_flags;                /* dentry cache flags */
        spinlock_t                                      d_lock;
        struct inode                            *d_inode;
@@ -315,10 +315,10 @@ struct dentry_operations {
 /* File: represents a file opened by a process. */
 struct file {
        TAILQ_ENTRY(file)                       f_list;                 /* list of all files */
-       struct inode                            *f_inode;               /* was dentry.  i prefer this */
+       struct dentry                           *f_dentry;              /* definitely not inode.  =( */
        struct vfsmount                         *f_vfsmnt;
        struct file_operations          *f_op;
-       atomic_t                                        f_refcnt;
+       struct kref                                     f_kref;
        unsigned int                            f_flags;                /* O_APPEND, etc */
        int                                                     f_mode;                 /* O_RDONLY, etc */
        off_t                                           f_pos;                  /* offset / file pointer */
@@ -376,7 +376,7 @@ struct vfsmount {
        struct super_block                      *mnt_sb;
        struct vfsmount_tailq           mnt_child_mounts;
        TAILQ_ENTRY(vfsmount)           mnt_child_link;
-       atomic_t                                        mnt_refcnt;
+       struct kref                                     mnt_kref;
        int                                                     mnt_flags;
        char                                            *mnt_devname;
        struct namespace                        *mnt_namespace;
@@ -420,7 +420,7 @@ struct fs_struct {
 
 /* Each process can have its own (eventually), but default to the same NS */
 struct namespace {
-       atomic_t                                        refcnt;
+       struct kref                                     kref;
        spinlock_t                                      lock;
        struct vfsmount                         *root;
        struct vfsmount_tailq           vfsmounts;      /* all vfsmounts in this ns */
@@ -459,10 +459,11 @@ void init_sb(struct super_block *sb, struct vfsmount *vmnt,
 struct dentry *get_dentry(struct super_block *sb, struct dentry *parent,
                           char *name);
 void dcache_put(struct dentry *dentry);
-void free_dentry(struct dentry *dentry);
+void dentry_release(struct kref *kref);
 
 /* Inode Functions */
 int check_perms(struct inode *inode, int access_mode);
+void inode_release(struct kref *kref);
 
 /* File functions */
 ssize_t generic_file_read(struct file *file, char *buf, size_t count,
@@ -470,7 +471,7 @@ ssize_t generic_file_read(struct file *file, char *buf, size_t count,
 ssize_t generic_file_write(struct file *file, const char *buf, size_t count,
                            off_t *offset);
 struct file *do_file_open(char *path, int flags, int mode);
-int do_file_close(struct file *file);
+void file_release(struct kref *kref);
 
 /* Page cache functions */
 struct page *pm_find_page(struct page_map *pm, unsigned long index);
index c749b06..31d65ee 100644 (file)
@@ -84,7 +84,6 @@ load_one_elf(struct proc* p, struct file* f, int pgoffset, elf_info_t* ei)
                        uintptr_t memsz = memend-memstart;
                        if(memend > ei->highest_addr)
                                ei->highest_addr = memend;
-
                        /* This needs to be a PRIVATE mapping, and the stuff after the file
                         * needs to be zeroed. */
                        if (filesz) {
@@ -136,14 +135,15 @@ int load_elf(struct proc* p, struct file* f)
        if(load_one_elf(p,f,0,&ei))
                return -1;
 
-       if(ei.dynamic)
-       {
-               /* this won't work til we convert some more syscalls to use the FS */
+       if (ei.dynamic) {
                struct file *interp = path_to_file(ei.interp);
-               /* this will probably conflict with the mmap from the TLS up above */
-               if (interp == NULL || load_one_elf(p, interp, 2, &interp_ei))
+               if (!interp)
+                       return -1;
+               /* careful, this could conflict with the mmap from the TLS up above */
+               int error = load_one_elf(p, interp, 2, &interp_ei);
+               kref_put(&interp->f_kref);
+               if (error)
                        return -1;
-               atomic_dec(&interp->f_refcnt);  /* TODO: REF */
        }
 
        // fill in auxiliary info for dynamic linker/runtime
index 7e0eab0..e2c374e 100644 (file)
@@ -57,8 +57,8 @@ static void kfs_init(void)
                                         __alignof__(struct kfs_i_info), 0, 0, 0);
 }
 
-/* Creates the SB (normally would read in from disc and create).  Ups the refcnt
- * for whoever consumes this.  Returns 0 on failure.
+/* Creates the SB (normally would read in from disc and create).  Passes it's
+ * ref out to whoever consumes this.  Returns 0 on failure.
  * TODO: consider pulling out more of the FS-independent stuff, if possible.
  * There are only two things, but the pain in the ass is that you'd need to read
  * the disc to get that first inode, and it's a FS-specific thing. */
@@ -119,7 +119,8 @@ struct fs_type kfs_fs_type = {"KFS", 0, kfs_get_sb, kfs_kill_sb, {0, 0},
 int kfs_readpage(struct file *file, struct page *page)
 {
        size_t pg_idx_byte = page->pg_index * PGSIZE;
-       struct kfs_i_info *k_i_info = (struct kfs_i_info*)file->f_inode->i_fs_info;
+       struct kfs_i_info *k_i_info = (struct kfs_i_info*)
+                                     file->f_dentry->d_inode->i_fs_info;
        uintptr_t begin = (size_t)k_i_info->filestart + pg_idx_byte;
        /* If we're beyond the initial start point, we just need a zero page.  This
         * is for a hole or for extending a file (even though it won't be saved).
@@ -154,7 +155,7 @@ struct inode *kfs_alloc_inode(struct super_block *sb)
        TAILQ_INSERT_HEAD(&sb->s_inodes, inode, i_sb_list);
        TAILQ_INIT(&inode->i_dentry);
        inode->i_ino = 0;                                       /* set by caller later */
-       atomic_set(&inode->i_refcnt, 1);
+       kref_init(&inode->i_kref, inode_release, 1);
        inode->i_blksize = 1;                           /* keep in sync with get_sb() */
        spinlock_init(&inode->i_lock);
        inode->i_op = &kfs_i_op;
@@ -282,20 +283,20 @@ void kfs_umount_begin(struct super_block *sb)
 /* inode_operations */
 
 /* Helper op, used when creating regular files (kfs_create()) and when making
- * directories (kfs_mkdir()).  References are a bit ugly.  We're passing out a
- * ref that is already stored/accounted for.  Might change that...  Also, this
- * needs to handle having nd == 0.  Note we make a distinction between the mode
- * and the file type (for now).  The caller of this should set the filetype. */
-struct inode *kfs_create_generic(struct inode *dir, struct dentry *dentry,
-                                 int mode, struct nameidata *nd)
+ * directories (kfs_mkdir()).  
+ * This needs to handle having nd == 0.  Note we make a distinction between the
+ * mode and the file type (for now).  The caller of this should set the
+ * filetype. */
+static struct inode *kfs_create_generic(struct dentry *dentry,
+                                        int mode, struct nameidata *nd)
 {
        /* note it is the i_ino that uniquely identifies a file in the system.
         * there's a diff between creating an inode (even for an in-use ino) and
         * then filling it in, and vs creating a brand new one */
        struct inode *inode = kfs_alloc_inode(dentry->d_sb);
-       dentry->d_inode = inode;                /* inode ref stored here */
-       TAILQ_INSERT_TAIL(&inode->i_dentry, dentry, d_alias); /* stored dentry ref*/
-       atomic_inc(&dentry->d_refcnt);  /* TODO: REF/KREF */
+       kref_get(&dentry->d_kref, 1);   /* to pin the dentry in RAM, KFS-style... */
+       dentry->d_inode = inode;                /* inode ref from alloc() stored here */
+       TAILQ_INSERT_TAIL(&inode->i_dentry, dentry, d_alias); /* weak dentry ref*/
        /* Need to finish the dentry */
        dentry->d_op = &kfs_d_op;
        dentry->d_fs_info = 0;
@@ -320,7 +321,7 @@ struct inode *kfs_create_generic(struct inode *dir, struct dentry *dentry,
 int kfs_create(struct inode *dir, struct dentry *dentry, int mode,
                struct nameidata *nd)
 {
-       struct inode *inode = kfs_create_generic(dir, dentry, mode, nd);        
+       struct inode *inode = kfs_create_generic(dentry, mode, nd);     
        if (!inode)
                return -1;
        inode->i_type = FS_I_FILE;
@@ -358,12 +359,13 @@ struct dentry *kfs_lookup(struct inode *dir, struct dentry *dentry,
 
        assert(dir_dent && dir_dent == TAILQ_LAST(&dir->i_dentry, dentry_tailq));
        assert(dir->i_type & FS_I_DIR);
-
+       assert(kref_refcnt(&dentry->d_kref) == 1);
        TAILQ_FOREACH(d_i, &dir_dent->d_subdirs, d_subdirs_link) {
                if (!strcmp(d_i->d_name.name, dentry->d_name.name)) {
                        /* since this dentry is already in memory (that's how KFS works), we
                         * can free the one that came in and return the real one */
-                       kmem_cache_free(dentry_kcache, dentry);
+                       kref_put(&dentry->d_kref);
+                       kref_get(&d_i->d_kref, 1);
                        return d_i;
                }
        }
@@ -371,14 +373,15 @@ struct dentry *kfs_lookup(struct inode *dir, struct dentry *dentry,
                if (!strcmp(d_i->d_name.name, dentry->d_name.name)) {
                        /* since this dentry is already in memory (that's how KFS works), we
                         * can free the one that came in and return the real one */
-                       kmem_cache_free(dentry_kcache, dentry);
+                       kref_put(&dentry->d_kref);
+                       kref_get(&d_i->d_kref, 1);
                        return d_i;
                }
        }
        /* no match, consider caching the negative result, freeing the
         * dentry, etc */
        printd("Not Found %s!!\n", dentry->d_name.name);
-       free_dentry(dentry);
+       kref_put(&dentry->d_kref);
        return 0;
 }
 
@@ -409,15 +412,15 @@ int kfs_symlink(struct inode *dir, struct dentry *dentry, const char *symname)
  * need it. */
 int kfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 {
-       struct inode *inode = kfs_create_generic(dir, dentry, mode, 0); 
+       struct inode *inode = kfs_create_generic(dentry, mode, 0);      
        if (!inode)
                return -1;
+       /* this is okay, since no directory (dir) can have more than one dentry */
        struct dentry *parent = TAILQ_FIRST(&dir->i_dentry);
        assert(parent && parent == TAILQ_LAST(&dir->i_dentry, dentry_tailq));
        inode->i_type = FS_I_DIR;
-       /* parent dentry tracks dentry as a subdir */
+       /* parent dentry tracks dentry as a subdir, weak reference */
        TAILQ_INSERT_TAIL(&parent->d_subdirs, dentry, d_subdirs_link);
-       atomic_inc(&dentry->d_refcnt);
        /* get ready to have our own kids */
        TAILQ_INIT(&((struct kfs_i_info*)inode->i_fs_info)->children);
        ((struct kfs_i_info*)inode->i_fs_info)->filestart = 0;
@@ -533,13 +536,13 @@ off_t kfs_llseek(struct file *file, off_t offset, int whence)
                        temp_off = file->f_pos + offset;
                        break;
                case SEEK_END:
-                       temp_off = file->f_inode->i_size + offset;
+                       temp_off = file->f_dentry->d_inode->i_size + offset;
                        break;
                default:
                        warn("Unknown 'whence' in llseek()!\n");
        }
        /* make sure the f_pos isn't outside the limits of the existing file */
-       temp_off = MAX(MIN(temp_off, file->f_inode->i_size), 0);
+       temp_off = MAX(MIN(temp_off, file->f_dentry->d_inode->i_size), 0);
        file->f_pos = temp_off;
        return temp_off;
 }
@@ -554,8 +557,8 @@ int kfs_readdir(struct file *dir, struct dirent *dirent)
        int count = 0;
        bool found = FALSE;
        struct dentry *subent;
-       struct dentry *dir_d = TAILQ_FIRST(&dir->f_inode->i_dentry);
-       struct kfs_i_info *k_i_info = (struct kfs_i_info*)dir->f_inode->i_fs_info;
+       struct dentry *dir_d = dir->f_dentry;
+       struct kfs_i_info *k_i_info = (struct kfs_i_info*)dir_d->d_inode->i_fs_info;
 
        /* how we check inside the for loops below.  moderately ghetto. */
        void check_entry(void)
@@ -571,7 +574,7 @@ int kfs_readdir(struct file *dir, struct dirent *dirent)
        }
        /* some of this error handling can be done by the VFS.  The syscall should
         * handle EBADF, EFAULT, and EINVAL (TODO, memory related). */
-       if (!(dir->f_inode->i_type & FS_I_DIR)) {
+       if (!(dir_d->d_inode->i_type & FS_I_DIR)) {
                set_errno(current_tf, ENOTDIR);
                return -1;
        }
@@ -598,24 +601,32 @@ int kfs_readdir(struct file *dir, struct dirent *dirent)
  * the file was opened or the file type. */
 int kfs_mmap(struct file *file, struct vm_region *vmr)
 {
-       if (file->f_inode->i_type & FS_I_FILE)
+       if (file->f_dentry->d_inode->i_type & FS_I_FILE)
                return 0;
        return -1;
 }
 
 /* Opens the file specified by the inode, creating and filling in the file */
-/* TODO: fill out the other // entries, sort vmnt refcnting */
+/* TODO: fill out the other // entries */
 int kfs_open(struct inode *inode, struct file *file)
 {
-       /* This is mostly FS-agnostic, consider a helper */
+       /* This is mostly FS-agnostic, consider a get_file() helper (TODO) */
        //file = kmem_cache_alloc(file_kcache, 0); /* done in the VFS */
+
+       /* one for the ref passed out, and *none* for the sb TAILQ */
+       kref_init(&file->f_kref, file_release, 1);
        /* Add to the list of all files of this SB */
        TAILQ_INSERT_TAIL(&inode->i_sb->s_files, file, f_list);
-       file->f_inode = inode;
-       atomic_inc(&inode->i_refcnt);
+
+       /* TODO: when we pull this out to the VFS, the function will take the dentry
+        * instead, and then call the fs_open(dentry->d_inode) */
+       struct dentry *my_d = TAILQ_FIRST(&inode->i_dentry);
+       kref_get(&my_d->d_kref, 1);
+       file->f_dentry = my_d;
+
+       kref_get(&inode->i_sb->s_mount->mnt_kref, 1);
        file->f_vfsmnt = inode->i_sb->s_mount;          /* saving a ref to the vmnt...*/
-       file->f_op = &kfs_f_op;
-       atomic_set(&file->f_refcnt, 1);                         /* ref passed out */
+       file->f_op = &kfs_f_op;         /* TODO: set me == to i_fop */
        file->f_flags = inode->i_flags;                         /* just taking the inode vals */
        file->f_mode = inode->i_mode;
        file->f_pos = 0;
@@ -635,15 +646,9 @@ int kfs_flush(struct file *file)
        return -1;
 }
 
-/* Called when the file refcnt == 0 */
+/* Called when the file is about to be closed (file obj freed) */
 int kfs_release(struct inode *inode, struct file *file)
 {
-       TAILQ_REMOVE(&inode->i_sb->s_files, file, f_list);
-       /* TODO: (REF) need to dealloc when this hits 0, atomic/concurrent/etc */
-       atomic_dec(&inode->i_refcnt);
-       /* TODO: clean up the inode if it was the last and we don't want it around
-        */
-       kmem_cache_free(file_kcache, file);
        return 0;
 }
 
index f4f9391..dca9157 100644 (file)
@@ -53,13 +53,16 @@ void manager(void)
        MANAGER_FUNC(DEVELOPER_NAME)();
 }
 
+char *p_argv[] = {0, 0, 0};
+char *p_envp[] = {"LD_LIBRARY_PATH=/lib", 0};
 /* Helper macro for quickly running a process.  Pass it a string, *file, and a
  * *proc. */
 #define quick_proc_run(x, p, f)                                                  \
        (f) = path_to_file((x));                                                     \
        assert((f));                                                                 \
-       (p) = proc_create(f, 0, 0);                                                  \
-       atomic_dec(&(f)->f_refcnt);                                                  \
+       p_argv[0] = file_name((f));                                               \
+       (p) = proc_create((f), p_argv, p_envp);                                      \
+       kref_put(&(f)->f_kref);                                                      \
        spin_lock(&(p)->proc_lock);                                                  \
        __proc_set_state((p), PROC_RUNNABLE_S);                                      \
        spin_unlock(&(p)->proc_lock);                                                \
@@ -69,8 +72,9 @@ void manager(void)
 #define quick_proc_create(x, p, f)                                               \
        (f) = path_to_file((x));                                                     \
        assert((f));                                                                 \
-       (p) = proc_create(f, 0, 0);                                                  \
-       atomic_dec(&(f)->f_refcnt);                                                  \
+       p_argv[0] = file_name((f));                                               \
+       (p) = proc_create((f), p_argv, p_envp);                                      \
+       kref_put(&(f)->f_kref);                                                      \
        spin_lock(&(p)->proc_lock);                                                  \
        __proc_set_state((p), PROC_RUNNABLE_S);                                      \
        spin_unlock(&(p)->proc_lock);
@@ -78,8 +82,9 @@ void manager(void)
 #define quick_proc_color_run(x, p, c, f)                                         \
        (f) = path_to_file((x));                                                     \
        assert((f));                                                                 \
-       (p) = proc_create(f, 0, 0);                                                  \
-       atomic_dec(&(f)->f_refcnt);                                                  \
+       p_argv[0] = file_name((f));                                               \
+       (p) = proc_create((f), p_argv, p_envp);                                      \
+       kref_put(&(f)->f_kref);                                                      \
        spin_lock(&(p)->proc_lock);                                                  \
        __proc_set_state((p), PROC_RUNNABLE_S);                                      \
        spin_unlock(&(p)->proc_lock);                                                \
@@ -92,8 +97,9 @@ void manager(void)
 #define quick_proc_color_create(x, p, c, f)                                      \
        (f) = path_to_file((x));                                                     \
        assert((f));                                                                 \
-       (p) = proc_create(f, 0, 0);                                                  \
-       atomic_dec(&(f)->f_refcnt);                                                  \
+       p_argv[0] = file_name((f));                                               \
+       (p) = proc_create((f), p_argv, p_envp);                                      \
+       kref_put(&(f)->f_kref);                                                      \
        spin_lock(&(p)->proc_lock);                                                  \
        __proc_set_state((p), PROC_RUNNABLE_S);                                      \
        spin_unlock(&(p)->proc_lock);                                                \
index 2d6a0ba..90a53c4 100644 (file)
@@ -104,8 +104,8 @@ struct vm_region *split_vmr(struct vm_region *old_vmr, uintptr_t va)
        new_vmr->vm_prot = old_vmr->vm_prot;
        new_vmr->vm_flags = old_vmr->vm_flags;
        if (old_vmr->vm_file) {
+               kref_get(&old_vmr->vm_file->f_kref, 1);
                new_vmr->vm_file = old_vmr->vm_file;
-               atomic_inc(&new_vmr->vm_file->f_refcnt);
                new_vmr->vm_foff = old_vmr->vm_foff +
                                      old_vmr->vm_end - old_vmr->vm_base;
        } else {
@@ -181,7 +181,7 @@ int shrink_vmr(struct vm_region *vmr, uintptr_t va)
 void destroy_vmr(struct vm_region *vmr)
 {
        if (vmr->vm_file)
-               atomic_dec(&vmr->vm_file->f_refcnt);
+               kref_put(&vmr->vm_file->f_kref);
        TAILQ_REMOVE(&vmr->vm_proc->vm_regions, vmr, vm_link);
        kmem_cache_free(vmr_kcache, vmr);
 }
@@ -253,9 +253,9 @@ void duplicate_vmrs(struct proc *p, struct proc *new_p)
                vmr->vm_end = vm_i->vm_end;
                vmr->vm_prot = vm_i->vm_prot;   
                vmr->vm_flags = vm_i->vm_flags; 
+               if (vm_i->vm_file)
+                       kref_get(&vm_i->vm_file->f_kref, 1);
                vmr->vm_file = vm_i->vm_file;
-               if (vmr->vm_file)
-                       atomic_inc(&vmr->vm_file->f_refcnt);
                vmr->vm_foff = vm_i->vm_foff;
                TAILQ_INSERT_TAIL(&new_p->vm_regions, vmr, vm_link);
        }
@@ -303,7 +303,7 @@ void *mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
        addr = MAX(addr, MMAP_LOWEST_VA);
        void *result = do_mmap(p, addr, len, prot, flags, file, offset);
        if (file)
-               file_decref(file);
+               kref_put(&file->f_kref);
        return result;
 }
 
@@ -341,6 +341,8 @@ void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
        }
        vmr->vm_prot = prot;
        vmr->vm_flags = flags;
+       if (file)
+               kref_get(&file->f_kref, 1);
        vmr->vm_file = file;
        vmr->vm_foff = offset;
        /* Prep the FS to make sure it can mmap the file.  Slightly weird semantics:
index d89738b..c762157 100644 (file)
@@ -268,10 +268,7 @@ int mon_bin_ls(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                printk("%s\n", dir.d_name);
                dir.d_off++;
        } while (retval == 1);
-       /* TODO: (REF) need to dealloc when this hits 0, atomic/concurrent/etc */
-       atomic_dec(&bin_dir->f_refcnt);
-       assert(!bin_dir->f_refcnt);
-       bin_dir->f_op->release(bin_dir->f_inode, bin_dir);
+       kref_put(&bin_dir->f_kref);
        return 0;
 }
 
@@ -291,7 +288,7 @@ int mon_bin_run(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                return 1;
        }
        char *p_argv[] = {0, 0, 0};
-       char *p_envp[] = {"LD_LIBRARY_PATH=/lib", 0};   /* for /bin/sh, i think */
+       char *p_envp[] = {"LD_LIBRARY_PATH=/lib", 0};
        p_argv[0] = file_name(program);
        struct proc *p = proc_create(program, p_argv, p_envp);
 
@@ -300,8 +297,7 @@ int mon_bin_run(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        schedule_proc(p);
        spin_unlock(&p->proc_lock);
        kref_put(&p->kref); /* let go of the reference created in proc_create() */
-       /* TODO: (REF) need to dealloc when this hits 0, atomic/concurrent/etc */
-       atomic_dec(&program->f_refcnt);
+       kref_put(&program->f_kref);
        /* Should never return from schedule (env_pop in there) also note you may
         * not get the process you created, in the event there are others floating
         * around that are runnable */
@@ -494,15 +490,14 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                begin = start_timing();
 #ifdef __CONFIG_APPSERVER__
                printk("Warning: this will be inaccurate due to the appserver.\n");
-               end_refcnt = atomic_read(&p->kref.refcount) -
-                            p->procinfo->num_vcores - 1;
+               end_refcnt = kref_refcnt(&p->kref) - p->procinfo->num_vcores - 1;
 #endif /* __CONFIG_APPSERVER__ */
                proc_destroy(p);
                kref_put(&p->kref);
 #ifdef __CONFIG_APPSERVER__
                /* Won't be that accurate, since it's not actually going through the
                 * __proc_free() path. */
-               spin_on(atomic_read(&p->kref.refcount) != end_refcnt);  
+               spin_on(kref_refcnt(&p->kref) != end_refcnt);   
 #else
                /* this is a little ghetto. it's not fully free yet, but we are also
                 * slowing it down by messing with it, esp with the busy waiting on a
@@ -529,12 +524,11 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        spin_on(p->procinfo->pcoremap[pcoreid].valid);
                        diff = stop_timing(begin);
                } else { /* preempt all cores, warned but no delay */
-                       end_refcnt = atomic_read(&p->kref.refcount)
-                                    - p->procinfo->num_vcores;
+                       end_refcnt = kref_refcnt(&p->kref) - p->procinfo->num_vcores;
                        begin = start_timing();
                        proc_preempt_all(p, 1000000);
                        /* a little ghetto, implies no one is using p */
-                       spin_on(atomic_read(&p->kref.refcount) != end_refcnt);
+                       spin_on(kref_refcnt(&p->kref) != end_refcnt);
                        diff = stop_timing(begin);
                }
                kref_put(&p->kref);
@@ -603,15 +597,14 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        /* TODO: (RMS), if num_vcores == 0, RUNNABLE_M, schedule */
                } else { /* preempt all cores, no warning or waiting */
                        spin_lock(&p->proc_lock);
-                       end_refcnt = atomic_read(&p->kref.refcount)
-                                    - p->procinfo->num_vcores;
+                       end_refcnt = kref_refcnt(&p->kref) - p->procinfo->num_vcores;
                        begin = start_timing();
                        self_ipi_pending = __proc_preempt_all(p);
                        /* TODO: (RMS), RUNNABLE_M, schedule */
                        spin_unlock(&p->proc_lock);
                        __proc_kmsg_pending(p, self_ipi_pending);
                        /* a little ghetto, implies no one else is using p */
-                       spin_on(atomic_read(&p->kref.refcount) != end_refcnt);
+                       spin_on(kref_refcnt(&p->kref) != end_refcnt);
                        diff = stop_timing(begin);
                }
                kref_put(&p->kref);
@@ -781,7 +774,10 @@ int mon_fs(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        if (!strcmp(argv[1], "open")) {
                printk("Open Files:\n----------------------------\n");
                TAILQ_FOREACH(i, &sb->s_files, f_list)
-                       printk("File: %08p, %s, Refs: %d\n", i, file_name(i), i->f_refcnt);
+                       printk("File: %08p, %s, Refs: %d, Drefs: %d, Irefs: %d\n", i,
+                              file_name(i), kref_refcnt(&i->f_kref),
+                                  kref_refcnt(&i->f_dentry->d_kref),
+                                  kref_refcnt(&i->f_dentry->d_inode->i_kref));
        } else if (!strcmp(argv[1], "pid")) {
                if (argc != 3) {
                        printk("Give me a pid number.\n");
index 1f353cd..c146799 100644 (file)
@@ -311,14 +311,14 @@ error_t proc_alloc(struct proc **pp, struct proc *parent)
                        SYSEVENTRINGSIZE);
 
        /* Init FS structures TODO: cleanup (might pull this out) */
-       atomic_inc(&default_ns.refcnt);
+       kref_get(&default_ns.kref, 1);
        p->ns = &default_ns;
        spinlock_init(&p->fs_env.lock);
        p->fs_env.umask = parent ? parent->fs_env.umask : 0002;
        p->fs_env.root = p->ns->root->mnt_root;
-       atomic_inc(&p->fs_env.root->d_refcnt);
+       kref_get(&p->fs_env.root->d_kref, 1);
        p->fs_env.pwd = parent ? parent->fs_env.pwd : p->fs_env.root;
-       atomic_inc(&p->fs_env.pwd->d_refcnt);
+       kref_get(&p->fs_env.pwd->d_kref, 1);
        memset(&p->open_files, 0, sizeof(p->open_files));       /* slightly ghetto */
        spinlock_init(&p->open_files.lock);
        p->open_files.max_files = NR_OPEN_FILES_DEFAULT;
@@ -363,7 +363,7 @@ static void __proc_free(struct kref *kref)
 
        printd("[PID %d] freeing proc: %d\n", current ? current->pid : 0, p->pid);
        // All parts of the kernel should have decref'd before __proc_free is called
-       assert(atomic_read(&p->kref.refcount) == 0);
+       assert(kref_refcnt(&p->kref) == 0);
 
        close_all_files(&p->open_files);
        destroy_vmrs(p);
index a43baae..77ae012 100644 (file)
@@ -217,28 +217,25 @@ static int sys_proc_create(struct proc *p, char *path, size_t path_l,
         * args/env, since auxp gets set up there. */
        //new_p = proc_create(program, 0, 0);
        if (proc_alloc(&new_p, current))
-               return -1;
+               goto mid_error;
        /* Set the argument stuff needed by glibc */
        if (memcpy_from_user_errno(p, new_p->procinfo->argp, pi->argp,
-                                  sizeof(pi->argp))) {
-               atomic_dec(&program->f_refcnt); /* TODO: REF */
-               proc_destroy(new_p);
-               return -1;
-       }
+                                  sizeof(pi->argp)))
+               goto late_error;
        if (memcpy_from_user_errno(p, new_p->procinfo->argbuf, pi->argbuf,
-                                  sizeof(pi->argbuf))) {
-               atomic_dec(&program->f_refcnt); /* TODO: REF */
-               proc_destroy(new_p);
-               return -1;
-       }
-       if (load_elf(new_p, program)) {
-               proc_destroy(new_p);
-               return -1;
-       }
+                                  sizeof(pi->argbuf)))
+               goto late_error;
+       if (load_elf(new_p, program))
+               goto late_error;
+       kref_put(&program->f_kref);
        pid = new_p->pid;
        kref_put(&new_p->kref); /* give up the reference created in proc_create() */
-       atomic_dec(&program->f_refcnt);         /* TODO: REF / KREF */
        return pid;
+late_error:
+       proc_destroy(new_p);
+mid_error:
+       kref_put(&program->f_kref);
+       return -1;
 }
 
 /* Makes process PID runnable.  Consider moving the functionality to process.c */
@@ -411,15 +408,11 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
                return -1;                      /* presumably, errno is already set */
        /* Set the argument stuff needed by glibc */
        if (memcpy_from_user_errno(p, p->procinfo->argp, pi->argp,
-                                  sizeof(pi->argp))) {
-               atomic_dec(&program->f_refcnt); /* TODO: REF */
-               return -1;
-       }
+                                  sizeof(pi->argp)))
+               goto mid_error;
        if (memcpy_from_user_errno(p, p->procinfo->argbuf, pi->argbuf,
-                                  sizeof(pi->argbuf))) {
-               atomic_dec(&program->f_refcnt); /* TODO: REF */
-               return -1;
-       }
+                                  sizeof(pi->argbuf)))
+               goto mid_error;
        /* This is the point of no return for the process. */
        /* TODO: issues with this: Need to also assert there are no outstanding
         * users of the sysrings.  the ldt page will get freed shortly, so that's
@@ -428,13 +421,17 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
        memset(p->procdata, 0, sizeof(procdata_t));
        env_user_mem_free(p, 0, UMAPTOP);
        if (load_elf(p, program)) {
+               kref_put(&program->f_kref);
                proc_destroy(p);
                smp_idle();             /* syscall can't return on failure now */
        }
        printd("[PID %d] exec %s\n", p->pid, file_name(program));
-       atomic_dec(&program->f_refcnt);         /* TODO: (REF) / KREF */
+       kref_put(&program->f_kref);
        *current_tf = p->env_tf;
        return 0;
+mid_error:
+       kref_put(&program->f_kref);
+       return -1;
 }
 
 static ssize_t sys_trywait(env_t* e, pid_t pid, int* status)
@@ -872,7 +869,7 @@ intreg_t sys_open(struct proc *p, const char *path, int oflag, int mode)
        if (!file)
                return -1;
        fd = insert_file(&p->open_files, file); /* stores the ref to file */
-       atomic_dec(&file->f_refcnt);    /* TODO: REF / KREF */
+       kref_put(&file->f_kref);
        if (fd < 0) {
                warn("File insertion failed");
                return -1;
@@ -889,7 +886,11 @@ intreg_t sys_close(struct proc *p, int fd)
                return -1;
        }
        /* TEMP TEST */
-       assert(!file->f_refcnt);
+       if (kref_refcnt(&file->f_kref)) {
+               printk("sys_close: Detected positive refcnt %d for file %s\n",
+                      kref_refcnt(&file->f_kref), file_name(file));
+               panic("Idiot.");
+       }
        return 0;
 }
 
index 613c9db..9b10dcf 100644 (file)
@@ -39,6 +39,8 @@ struct vfsmount *mount_fs(struct fs_type *fs, char *dev_name,
        struct super_block *sb;
        struct vfsmount *vmnt = kmalloc(sizeof(struct vfsmount), 0);
 
+       /* this first ref is stored in the NS tailq below */
+       kref_init(&vmnt->mnt_kref, fake_release, 1);
        /* Build the vfsmount, if there is no mnt_pt, mnt is the root vfsmount (for now).
         * fields related to the actual FS, like the sb and the mnt_root are set in
         * the fs-specific get_sb() call. */
@@ -48,7 +50,7 @@ struct vfsmount *mount_fs(struct fs_type *fs, char *dev_name,
        } else { /* common case, but won't be tested til we try to mount another FS */
                mnt_pt->d_mount_point = TRUE;
                mnt_pt->d_mounted_fs = vmnt;
-               atomic_inc(&vmnt->mnt_refcnt); /* held by mnt_pt */
+               kref_get(&vmnt->mnt_kref, 1); /* held by mnt_pt */
                vmnt->mnt_parent = mnt_pt->d_sb->s_mount;
                vmnt->mnt_mountpoint = mnt_pt;
        }
@@ -56,8 +58,7 @@ struct vfsmount *mount_fs(struct fs_type *fs, char *dev_name,
        vmnt->mnt_flags = flags;
        vmnt->mnt_devname = dev_name;
        vmnt->mnt_namespace = ns;
-       atomic_inc(&ns->refcnt); /* held by vmnt */
-       atomic_set(&vmnt->mnt_refcnt, 1); /* for the ref in the NS tailq below */
+       kref_get(&ns->kref, 1); /* held by vmnt */
 
        /* Read in / create the SB */
        sb = fs->get_sb(fs, flags, dev_name, vmnt);
@@ -90,8 +91,8 @@ void vfs_init(void)
                                         __alignof__(struct inode), 0, 0, 0);
        file_kcache = kmem_cache_create("file", sizeof(struct file),
                                        __alignof__(struct file), 0, 0, 0);
-
-       atomic_set(&default_ns.refcnt, 1); // default NS never dies, +1 to exist
+       /* default NS never dies, +1 to exist */
+       kref_init(&default_ns.kref, fake_release, 1);
        spinlock_init(&default_ns.lock);
        default_ns.root = NULL;
        TAILQ_INIT(&default_ns.vfsmounts);
@@ -124,7 +125,7 @@ void qstr_builder(struct dentry *dentry, char *l_name)
 /* Useful little helper - return the string ptr for a given file */
 char *file_name(struct file *file)
 {
-       return (char*)TAILQ_FIRST(&file->f_inode->i_dentry)->d_name.name;
+       return file->f_dentry->d_name.name;
 }
 
 /* Some issues with this, coupled closely to fs_lookup.  This assumes that
@@ -160,10 +161,16 @@ static int climb_up(struct nameidata *nd)
 static int next_link(struct dentry *dentry, struct nameidata *nd)
 {
        assert(nd->dentry && nd->mnt);
-       atomic_dec(&nd->dentry->d_refcnt);
-       atomic_dec(&nd->mnt->mnt_refcnt);
+       /* update the dentry */
+       kref_get(&dentry->d_kref, 1);
+       kref_put(&nd->dentry->d_kref);
        nd->dentry = dentry;
-       nd->mnt = dentry->d_sb->s_mount;
+       /* update the mount, if we need to */
+       if (dentry->d_sb->s_mount != nd->mnt) {
+               kref_get(&dentry->d_sb->s_mount->mnt_kref, 1);
+               kref_put(&nd->mnt->mnt_kref);
+               nd->mnt = dentry->d_sb->s_mount;
+       }
        return 0;
 }
 
@@ -234,6 +241,7 @@ static int link_path_walk(char *path, struct nameidata *nd)
                        return -ENOENT;
                /* make link_dentry the current step/answer */
                next_link(link_dentry, nd);
+               kref_put(&link_dentry->d_kref); /* do_lookup gave us a refcnt dentry */
                /* we could be on a mountpoint or a symlink - need to follow them */
                follow_mount(nd);
                follow_symlink(nd);
@@ -262,6 +270,7 @@ static int link_path_walk(char *path, struct nameidata *nd)
        if (!link_dentry)
                return -ENOENT;
        next_link(link_dentry, nd);
+       kref_put(&link_dentry->d_kref); /* do_lookup gave us a refcnt dentry */
        follow_mount(nd);
        follow_symlink(nd);
        /* If we wanted a directory, but didn't get one, error out */
@@ -282,6 +291,7 @@ static int link_path_walk(char *path, struct nameidata *nd)
  * it's still user input.  */
 int path_lookup(char *path, int flags, struct nameidata *nd)
 {
+       printd("Path lookup for %s\n", path);
        /* we allow absolute lookups with no process context */
        if (path[0] == '/') {                   /* absolute lookup */
                if (!current)
@@ -296,8 +306,8 @@ int path_lookup(char *path, int flags, struct nameidata *nd)
        nd->mnt = nd->dentry->d_sb->s_mount;
        /* Whenever references get put in the nd, incref them.  Whenever they are
         * removed, decref them. */
-       atomic_inc(&nd->mnt->mnt_refcnt);
-       atomic_inc(&nd->dentry->d_refcnt);
+       kref_get(&nd->mnt->mnt_kref, 1);
+       kref_get(&nd->dentry->d_kref, 1);
        nd->flags = flags;
        nd->depth = 0;                                  /* used in symlink following */
        return link_path_walk(path, nd);        
@@ -307,9 +317,8 @@ int path_lookup(char *path, int flags, struct nameidata *nd)
  * regardless of whether it succeeded or not.  It will free any references */
 void path_release(struct nameidata *nd)
 {
-       /* TODO: (REF), do something when we hit 0, etc... */
-       atomic_dec(&nd->dentry->d_refcnt);
-       atomic_dec(&nd->mnt->mnt_refcnt);
+       kref_put(&nd->dentry->d_kref);
+       kref_put(&nd->mnt->mnt_kref);
 }
 
 /* Seems convenient: Given a path, return the appropriate file.  The reference
@@ -332,6 +341,7 @@ struct file *path_to_file(char *path)
                return 0;
        }
        path_release(&nd);
+       assert(kref_refcnt(&f->f_kref) == 1);
        return f;
 }
 
@@ -345,7 +355,7 @@ struct super_block *get_sb(void)
        struct super_block *sb = kmalloc(sizeof(struct super_block), 0);
        sb->s_dirty = FALSE;
        spinlock_init(&sb->s_lock);
-       atomic_set(&sb->s_refcnt, 1); // for the ref passed out
+       kref_init(&sb->s_kref, fake_release, 1); /* for the ref passed out */
        TAILQ_INIT(&sb->s_inodes);
        TAILQ_INIT(&sb->s_dirty_i);
        TAILQ_INIT(&sb->s_io_wb);
@@ -376,22 +386,21 @@ void init_sb(struct super_block *sb, struct vfsmount *vmnt,
        struct inode *inode = sb->s_op->alloc_inode(sb);
        if (!inode)
                panic("This FS sucks!");
-       d_root->d_inode = inode;
-       TAILQ_INSERT_TAIL(&inode->i_dentry, d_root, d_alias);
-       atomic_inc(&d_root->d_refcnt);                  /* held by the inode */
+       d_root->d_inode = inode;                                /* storing the inode's kref here */
+       TAILQ_INSERT_TAIL(&inode->i_dentry, d_root, d_alias);   /* weak ref */
        inode->i_ino = root_ino;
        /* TODO: add the inode to the appropriate list (off i_list) */
        /* TODO: do we need to read in the inode?  can we do this on demand? */
        /* if this FS is already mounted, we'll need to do something different. */
        sb->s_op->read_inode(inode);
        /* Link the dentry and SB to the VFS mount */
-       vmnt->mnt_root = d_root;                                /* refcnt'd above */
+       vmnt->mnt_root = d_root;                                /* ref comes from get_dentry */
        vmnt->mnt_sb = sb;
        /* If there is no mount point, there is no parent.  This is true only for
         * the rootfs. */
        if (vmnt->mnt_mountpoint) {
+               kref_get(&vmnt->mnt_mountpoint->d_kref, 1);     /* held by d_root */
                d_root->d_parent = vmnt->mnt_mountpoint;        /* dentry of the root */
-               atomic_inc(&vmnt->mnt_mountpoint->d_refcnt);/* held by d_root */
        }
        /* insert the dentry into the dentry cache.  when's the earliest we can?
         * when's the earliest we should?  what about concurrent accesses to the
@@ -403,8 +412,8 @@ void init_sb(struct super_block *sb, struct vfsmount *vmnt,
 
 /* Helper to alloc and initialize a generic dentry.  The following needs to be
  * set still: d_op, d_fs_info (opt), d_inode, connect the inode to the dentry
- * (and up the d_refcnt), maybe dcache_put().  The FS related things need to be
- * done in fs_create and fs_mkdir.
+ * (and up the d_kref again), maybe dcache_put().  The FS related things need to
+ * be done in fs_create and fs_mkdir.
  *
  * If the name is longer than the inline name, it will kmalloc a buffer, so
  * don't worry about the storage for *name after calling this. */
@@ -417,16 +426,17 @@ struct dentry *get_dentry(struct super_block *sb, struct dentry *parent,
        char *l_name = 0;
 
        //memset(dentry, 0, sizeof(struct dentry));
-       atomic_set(&dentry->d_refcnt, 1);       /* this ref is returned */
+       kref_init(&dentry->d_kref, dentry_release, 1);  /* this ref is returned */
        spinlock_init(&dentry->d_lock);
        TAILQ_INIT(&dentry->d_subdirs);
        dentry->d_time = 0;
+       kref_get(&sb->s_kref, 1);
        dentry->d_sb = sb;                                      /* storing a ref here... */
        dentry->d_mount_point = FALSE;
        dentry->d_mounted_fs = 0;
-       dentry->d_parent = parent;
        if (parent)                                                     /* no parent for rootfs mount */
-               atomic_inc(&parent->d_refcnt);
+               kref_get(&parent->d_kref, 1);
+       dentry->d_parent = parent;
        dentry->d_flags = 0;                            /* related to its dcache state */
        dentry->d_fs_info = 0;
        SLIST_INIT(&dentry->d_bucket);
@@ -441,28 +451,52 @@ struct dentry *get_dentry(struct super_block *sb, struct dentry *parent,
                l_name[name_len] = '\0';
                qstr_builder(dentry, l_name);
        }
+       /* Catch bugs by aggressively zeroing this (o/w we use old stuff) */
+       dentry->d_op = 0;
+       dentry->d_fs_info = 0;
+       dentry->d_inode = 0;
        return dentry;
 }
 
 /* Adds a dentry to the dcache. */
 void dcache_put(struct dentry *dentry)
 {
+#if 0 /* pending a more thorough review of the dcache */
        /* TODO: should set a d_flag too */
        spin_lock(&dcache_lock);
        SLIST_INSERT_HEAD(&dcache, dentry, d_hash);
        spin_unlock(&dcache_lock);
+#endif
 }
 
-/* Free the dentry (after ref == 0 and we no longer want it).  Pairs with
- * get_dentry(), mostly. */
-void free_dentry(struct dentry *dentry)
+/* Cleans up the dentry (after ref == 0).  We still may want it, and this is
+ * where we should add it to the dentry cache.  (TODO).  For now, we do nothing,
+ * since we don't have a dcache.
+ * 
+ * This has to handle two types of dentries: full ones (ones that had been used)
+ * and ones that had been just for lookups - hence the checks for d_op and
+ * d_inode.
+ *
+ * Note that dentries pin and kref their inodes.  When all the dentries are
+ * gone, we want the inode to be released via kref.  The inode has internal /
+ * weak references to the dentry, which are not refcounted. */
+void dentry_release(struct kref *kref)
 {
+       struct dentry *dentry = container_of(kref, struct dentry, d_kref);
+       printd("Freeing dentry %08p: %s\n", dentry, dentry->d_name.name);
        /* Might not have a d_op, if it was involved in a failed operation */
        if (dentry->d_op && dentry->d_op->d_release)
                dentry->d_op->d_release(dentry);
        /* TODO: check/test the boundaries on this. */
        if (dentry->d_name.len > DNAME_INLINE_LEN)
                kfree((void*)dentry->d_name.name);
+       kref_put(&dentry->d_sb->s_kref);
+       if (dentry->d_mounted_fs)
+               kref_put(&dentry->d_mounted_fs->mnt_kref);
+       if (dentry->d_inode) {
+               TAILQ_REMOVE(&dentry->d_inode->i_dentry, dentry, d_alias);
+               kref_put(&dentry->d_inode->i_kref);     /* but dentries kref inodes */
+       }
        kmem_cache_free(dentry_kcache, dentry);
 }
 
@@ -476,6 +510,19 @@ int check_perms(struct inode *inode, int access_mode)
        return 0;       /* anything goes! */
 }
 
+/* Called after all external refs are gone to clean up the inode.  Once this is
+ * called, all dentries pointing here are already done (one of them triggered
+ * this via kref_put(). */
+void inode_release(struct kref *kref)
+{
+       struct inode *inode = container_of(kref, struct inode, i_kref);
+       inode->i_sb->s_op->destroy_inode(inode);
+       kref_put(&inode->i_sb->s_kref);
+       assert(inode->i_mapping == &inode->i_pm);
+       /* TODO: (BDEV) */
+       // kref_put(inode->i_bdev->kref); /* assuming it's a bdev */
+}
+
 /* File functions */
 
 /* Read count bytes from the file into buf, starting at *offset, which is increased
@@ -495,11 +542,11 @@ ssize_t generic_file_read(struct file *file, char *buf, size_t count,
        /* Consider pushing some error checking higher in the VFS */
        if (!count)
                return 0;
-       if (*offset == file->f_inode->i_size)
+       if (*offset == file->f_dentry->d_inode->i_size)
                return 0; /* EOF */
        /* Make sure we don't go past the end of the file */
-       if (*offset + count > file->f_inode->i_size) {
-               count = file->f_inode->i_size - *offset;
+       if (*offset + count > file->f_dentry->d_inode->i_size) {
+               count = file->f_dentry->d_inode->i_size - *offset;
        }
        page_off = *offset & (PGSIZE - 1);
        first_idx = *offset >> PGSHIFT;
@@ -550,8 +597,8 @@ ssize_t generic_file_write(struct file *file, const char *buf, size_t count,
                return 0;
        /* Extend the file.  Should put more checks in here, and maybe do this per
         * page in the for loop below. */
-       if (*offset + count > file->f_inode->i_size)
-               file->f_inode->i_size = *offset + count;
+       if (*offset + count > file->f_dentry->d_inode->i_size)
+               file->f_dentry->d_inode->i_size = *offset + count;
        page_off = *offset & (PGSIZE - 1);
        first_idx = *offset >> PGSHIFT;
        last_idx = (*offset + count) >> PGSHIFT;
@@ -614,10 +661,9 @@ struct file *do_file_open(char *path, int flags, int mode)
                /* TODO: mode should be & ~umask.  Note that mode only applies to future
                 * opens. */
                if (parent_i->i_op->create(parent_i, file_d, mode, nd)) {
-                       atomic_dec(&file_d->d_refcnt);
-                       free_dentry(file_d); /* TODO: REF trigger off a decref */
-                       set_errno(current_tf, EFAULT);
+                       kref_put(&file_d->d_kref);
                        path_release(nd);
+                       set_errno(current_tf, EFAULT);
                        return 0;
                }
                /* when we have notions of users, do something here: */
@@ -625,44 +671,60 @@ struct file *do_file_open(char *path, int flags, int mode)
                file_d->d_inode->i_gid = 0;
                file_d->d_inode->i_flags = flags & ~(O_CREAT|O_TRUNC|O_EXCL|O_NOCTTY);
                dcache_put(file_d);
-               atomic_dec(&file_d->d_refcnt);  /* TODO: REF / KREF */
        } else {        /* the file exists */
                if ((flags & O_CREAT) && (flags & O_EXCL)) {
-                       /* wanted to create, not open */
+                       /* wanted to create, not open, bail out */
+                       kref_put(&file_d->d_kref);
                        path_release(nd);
                        set_errno(current_tf, EACCES);
                        return 0;
                }
        }
-       /* now open the file (freshly created or if it already existed) */
+       /* now open the file (freshly created or if it already existed).  At this
+        * point, file_d is a refcnt'd dentry, regardless of which branch we took.*/
        file = kmem_cache_alloc(file_kcache, 0);
        if (!file) {
-               set_errno(current_tf, ENOMEM);
+               kref_put(&file_d->d_kref);
                path_release(nd);
+               set_errno(current_tf, ENOMEM);
                return 0;
        }
        if (flags & O_TRUNC)
                warn("File truncation not supported yet.");
        if (file_d->d_inode->i_fop->open(file_d->d_inode, file)) {
-               set_errno(current_tf, ENOENT);
+               kref_put(&file_d->d_kref);
                path_release(nd);
                kmem_cache_free(file_kcache, file);
+               set_errno(current_tf, ENOENT);
                return 0;
        }
        /* TODO: check the inode's mode (S_XXX) against the flags O_RDWR */
        /* f_mode stores how the FILE is open, regardless of the mode */
        file->f_mode = flags & (O_RDONLY|O_WRONLY|O_RDWR);
+       kref_put(&file_d->d_kref);
        path_release(nd);
        return file;
 }
 
-/* Closes a file, fsync, whatever else is necessary */
-int do_file_close(struct file *file)
+/* Closes a file, fsync, whatever else is necessary.  Called when the kref hits
+ * 0.  Note that the file is not refcounted on the s_files list, nor is the
+ * f_mapping refcounted (it is pinned by the i_mapping). */
+void file_release(struct kref *kref)
 {
-       assert(!file->f_refcnt);
-       /* TODO: fsync */
-       file->f_op->release(file->f_inode, file);
-       return 0;
+       struct file *file = container_of(kref, struct file, f_kref);
+
+       struct super_block *sb = file->f_dentry->d_sb;
+       spin_lock(&sb->s_lock);
+       TAILQ_REMOVE(&sb->s_files, file, f_list);
+       spin_unlock(&sb->s_lock);
+
+       /* TODO: fsync (BLK).  also, we may want to parallelize the blocking that
+        * could happen in here (spawn kernel threads)... */
+       file->f_op->release(file->f_dentry->d_inode, file);
+       /* Clean up the other refs we hold */
+       kref_put(&file->f_dentry->d_kref);
+       kref_put(&file->f_vfsmnt->mnt_kref);
+       kmem_cache_free(file_kcache, file);
 }
 
 /* Page cache functions */
@@ -798,7 +860,7 @@ struct file *get_file_from_fd(struct files_struct *open_files, int file_desc)
                        assert(file_desc < open_files->max_files);
                        retval = open_files->fd[file_desc];
                        assert(retval);
-                       atomic_inc(&retval->f_refcnt);
+                       kref_get(&retval->f_kref, 1);
                }
        }
        spin_unlock(&open_files->lock);
@@ -820,14 +882,9 @@ struct file *put_file_from_fd(struct files_struct *open_files, int file_desc)
                        file = open_files->fd[file_desc];
                        open_files->fd[file_desc] = 0;
                        CLR_BITMASK_BIT(open_files->open_fds->fds_bits, file_desc);
-                       /* TODO: (REF) need to make sure we free if we hit 0 (might do this
-                        * in the caller */
-                       if (file) {
-                               atomic_dec(&file->f_refcnt);
-                               if (!file->f_refcnt)
-                                       assert(!do_file_close(file));
-                       }
                        /* the if case is due to files (stdin) without a *file yet */
+                       if (file)
+                               kref_put(&file->f_kref);
                }
        }
        spin_unlock(&open_files->lock);
@@ -846,8 +903,8 @@ int insert_file(struct files_struct *open_files, struct file *file)
                slot = i;
                SET_BITMASK_BIT(open_files->open_fds->fds_bits, slot);
                assert(slot < open_files->max_files && open_files->fd[slot] == 0);
+               kref_get(&file->f_kref, 1);
                open_files->fd[slot] = file;
-               atomic_inc(&file->f_refcnt);
                if (slot >= open_files->next_fd)
                        open_files->next_fd = slot + 1;
                break;
@@ -858,7 +915,7 @@ int insert_file(struct files_struct *open_files, struct file *file)
        return slot;
 }
 
-/* Closes all open files.  Sort of a "put" plus do_file_close(), for all. */
+/* Closes all open files.  Mostly just a "put" for all files. */
 void close_all_files(struct files_struct *open_files)
 {
        struct file *file;
@@ -870,14 +927,9 @@ void close_all_files(struct files_struct *open_files)
                        assert(i < open_files->max_files);
                        file = open_files->fd[i];
                        open_files->fd[i] = 0;
-                       /* TODO: we may want to parallelize the blocking... */
-                       if (file) {
-                               /* TODO: REF/KREF */
-                               atomic_dec(&file->f_refcnt);
-                               if (!file->f_refcnt)
-                                       do_file_close(file);
-                       }
                        /* the if case is due to files (stdin) without a *file yet */
+                       if (file)
+                               kref_put(&file->f_kref);
                        CLR_BITMASK_BIT(open_files->open_fds->fds_bits, i);
                }
        }