Clean up inode alias tracking
authorEric Biggers <ebiggers3@gmail.com>
Sat, 24 Jan 2015 16:20:12 +0000 (10:20 -0600)
committerEric Biggers <ebiggers3@gmail.com>
Sat, 24 Jan 2015 16:50:58 +0000 (10:50 -0600)
New helper functions:

- d_associate()
- d_disassociate()

i_nlink, d_inode, and i_dentry are no longer changed outside of these
functions.  We maintain the invariants:

i_nlink == list_size(&i_dentry) == # of d_inode references

put_inode() is removed; free_inode() is now private to inode.c.

This fixes at least one bug: when freeing a dentry, it was not being
deleted from the inode's dentry list.

include/wimlib/inode.h
src/dentry.c
src/inode.c
src/inode_fixup.c
src/inode_table.c
src/mount_image.c

index 4b95677..11c2361 100644 (file)
@@ -304,21 +304,10 @@ new_inode(void) _malloc_attribute;
 extern struct wim_inode *
 new_timeless_inode(void) _malloc_attribute;
 
-extern void
-put_inode(struct wim_inode *inode);
-
-extern void
-free_inode(struct wim_inode *inode);
-
 /* Iterate through each alias of the specified inode.  */
 #define inode_for_each_dentry(dentry, inode) \
        list_for_each_entry((dentry), &(inode)->i_dentry, d_alias)
 
-/* Add a new alias for the specified inode.  Does not increment i_nlink; that
- * must be done separately if needed.  */
-#define inode_add_dentry(dentry, inode) \
-       list_add_tail(&(dentry)->d_alias, &(inode)->i_dentry)
-
 /* Return an alias of the specified inode.  */
 #define inode_first_dentry(inode) \
        container_of(inode->i_dentry.next, struct wim_dentry, d_alias)
@@ -328,6 +317,17 @@ free_inode(struct wim_inode *inode);
 #define inode_first_full_path(inode) \
        dentry_full_path(inode_first_dentry(inode))
 
+extern void
+d_associate(struct wim_dentry *dentry, struct wim_inode *inode);
+
+extern void
+d_disassociate(struct wim_dentry *dentry);
+
+#ifdef WITH_FUSE
+extern void
+inode_dec_num_opened_fds(struct wim_inode *inode);
+#endif
+
 /* Is the inode a directory?
  * This doesn't count directories with reparse data.
  * wimlib only allows inodes of this type to have children.
index 862c664..38b4efa 100644 (file)
@@ -925,6 +925,7 @@ _new_dentry_with_inode(const tchar *name, struct wim_dentry **dentry_ret,
                       bool timeless)
 {
        struct wim_dentry *dentry;
+       struct wim_inode *inode;
        int ret;
 
        ret = new_dentry(name, &dentry);
@@ -932,15 +933,16 @@ _new_dentry_with_inode(const tchar *name, struct wim_dentry **dentry_ret,
                return ret;
 
        if (timeless)
-               dentry->d_inode = new_timeless_inode();
+               inode = new_timeless_inode();
        else
-               dentry->d_inode = new_inode();
-       if (dentry->d_inode == NULL) {
+               inode = new_inode();
+       if (!inode) {
                free_dentry(dentry);
                return WIMLIB_ERR_NOMEM;
        }
 
-       inode_add_dentry(dentry, dentry->d_inode);
+       d_associate(dentry, inode);
+
        *dentry_ret = dentry;
        return 0;
 }
@@ -997,19 +999,17 @@ dentry_tree_clear_inode_visited(struct wim_dentry *root)
 /*
  * Free a WIM dentry.
  *
- * In addition to freeing the dentry itself, this decrements the link count of
- * the corresponding inode (if any).  If the inode's link count reaches 0, the
- * inode is freed as well.
+ * In addition to freeing the dentry itself, this disassociates the dentry from
+ * its inode.  If the inode is no longer in use, it will be freed as well.
  */
 void
 free_dentry(struct wim_dentry *dentry)
 {
        if (dentry) {
+               d_disassociate(dentry);
                FREE(dentry->file_name);
                FREE(dentry->short_name);
                FREE(dentry->_full_path);
-               if (dentry->d_inode)
-                       put_inode(dentry->d_inode);
                FREE(dentry);
        }
 }
index 9e4177c..6384b11 100644 (file)
@@ -61,7 +61,7 @@ new_timeless_inode(void)
        struct wim_inode *inode = CALLOC(1, sizeof(struct wim_inode));
        if (inode) {
                inode->i_security_id = -1;
-               inode->i_nlink = 1;
+               /*inode->i_nlink = 0;*/
                inode->i_next_stream_id = 1;
                inode->i_not_rpfixed = 1;
                inode->i_canonical_streams = 1;
@@ -71,21 +71,6 @@ new_timeless_inode(void)
        return inode;
 }
 
-/* Decrement an inode's link count.  */
-void
-put_inode(struct wim_inode *inode)
-{
-       wimlib_assert(inode->i_nlink != 0);
-       if (--inode->i_nlink == 0) {
-               /* If FUSE mounts are enabled, we must keep a unlinked inode
-                * around until all file descriptors to it have been closed.  */
-       #ifdef WITH_FUSE
-               if (inode->i_num_opened_fds == 0)
-       #endif
-                       free_inode(inode);
-       }
-}
-
 /* Free memory allocated within an alternate data stream entry.  */
 static void
 destroy_ads_entry(struct wim_ads_entry *ads_entry)
@@ -93,14 +78,9 @@ destroy_ads_entry(struct wim_ads_entry *ads_entry)
        FREE(ads_entry->stream_name);
 }
 
-/* Free an inode.  Only use this if there can't be other links to the inode or
- * if it doesn't matter if there are.  */
-void
+static void
 free_inode(struct wim_inode *inode)
 {
-       if (unlikely(!inode))
-               return;
-
        if (unlikely(inode->i_ads_entries)) {
                for (unsigned i = 0; i < inode->i_num_ads; i++)
                        destroy_ads_entry(&inode->i_ads_entries[i]);
@@ -115,6 +95,65 @@ free_inode(struct wim_inode *inode)
        FREE(inode);
 }
 
+static inline void
+free_inode_if_unneeded(struct wim_inode *inode)
+{
+       if (inode->i_nlink)
+               return;
+#ifdef WITH_FUSE
+       if (inode->i_num_opened_fds)
+               return;
+#endif
+       free_inode(inode);
+}
+
+/* Associate a dentry with the specified inode.  */
+void
+d_associate(struct wim_dentry *dentry, struct wim_inode *inode)
+{
+       wimlib_assert(!dentry->d_inode);
+
+       list_add_tail(&dentry->d_alias, &inode->i_dentry);
+       dentry->d_inode = inode;
+       inode->i_nlink++;
+}
+
+/* Disassociate a dentry from its inode, if any.  Following this, free the inode
+ * if it is no longer in use.  */
+void
+d_disassociate(struct wim_dentry *dentry)
+{
+       struct wim_inode *inode = dentry->d_inode;
+
+       if (unlikely(!inode))
+               return;
+
+       wimlib_assert(inode->i_nlink > 0);
+
+       list_del(&dentry->d_alias);
+       dentry->d_inode = NULL;
+       inode->i_nlink--;
+
+       free_inode_if_unneeded(inode);
+}
+
+#ifdef WITH_FUSE
+void
+inode_dec_num_opened_fds(struct wim_inode *inode)
+{
+       wimlib_assert(inode->i_num_opened_fds > 0);
+
+       if (--inode->i_num_opened_fds == 0) {
+               /* The last file descriptor to this inode was closed.  */
+               FREE(inode->i_fds);
+               inode->i_fds = NULL;
+               inode->i_num_allocated_fds = 0;
+
+               free_inode_if_unneeded(inode);
+       }
+}
+#endif
+
 /*
  * Returns the alternate data stream entry belonging to @inode that has the
  * stream name @stream_name, or NULL if the inode has no alternate data stream
index 616c5ff..7624562 100644 (file)
@@ -102,10 +102,8 @@ inode_table_insert(struct wim_dentry *dentry, void *_params)
                        continue;
                }
                /* Transfer this dentry to the existing inode.  */
-               free_inode(d_inode);
-               dentry->d_inode = inode;
-               inode->i_nlink++;
-               inode_add_dentry(dentry, inode);
+               d_disassociate(dentry);
+               d_associate(dentry, inode);
                return 0;
        }
 
index 6e1c143..49dd821 100644 (file)
@@ -50,35 +50,6 @@ destroy_inode_table(struct wim_inode_table *table)
        FREE(table->array);
 }
 
-static struct wim_inode *
-inode_table_get_inode(struct wim_inode_table *table, u64 ino, u64 devno)
-{
-       size_t pos;
-       struct wim_inode *inode;
-       struct hlist_node *cur;
-
-       /* Search for an existing inode having the same inode number and device
-        * number.  */
-       pos = hash_u64(hash_u64(ino) + hash_u64(devno)) % table->capacity;
-       hlist_for_each_entry(inode, cur, &table->array[pos], i_hlist) {
-               if (inode->i_ino == ino && inode->i_devno == devno) {
-                       /* Found; use the existing inode.  */
-                       inode->i_nlink++;
-                       return inode;
-               }
-       }
-
-       /* Create a new inode and insert it into the table.  */
-       inode = new_timeless_inode();
-       if (inode) {
-               inode->i_ino = ino;
-               inode->i_devno = devno;
-               hlist_add_head(&inode->i_hlist, &table->array[pos]);
-               table->num_entries++;
-       }
-       return inode;
-}
-
 /*
  * Allocate a new dentry, with hard link detection.
  *
@@ -130,23 +101,39 @@ inode_table_new_dentry(struct wim_inode_table *table, const tchar *name,
                        return ret;
                list_add_tail(&dentry->d_inode->i_list, &table->extra_inodes);
        } else {
+               size_t pos;
+               struct hlist_node *cur;
+
                /* File that can be hardlinked--- search the table for an
                 * existing inode matching the inode number and device;
                 * otherwise create a new inode. */
                ret = new_dentry(name, &dentry);
                if (ret)
                        return ret;
-               inode = inode_table_get_inode(table, ino, devno);
+
+               /* Search for an existing inode having the same inode number and
+                * device number.  */
+               pos = hash_u64(hash_u64(ino) + hash_u64(devno)) % table->capacity;
+               hlist_for_each_entry(inode, cur, &table->array[pos], i_hlist) {
+                       if (inode->i_ino == ino && inode->i_devno == devno) {
+                               /* Found; use the existing inode.  */
+                               inode_ref_streams(inode);
+                               goto have_inode;
+                       }
+               }
+
+               /* Create a new inode and insert it into the table.  */
+               inode = new_timeless_inode();
                if (!inode) {
                        free_dentry(dentry);
                        return WIMLIB_ERR_NOMEM;
                }
-               /* If using an existing inode, we need to gain a reference to
-                * each of its streams. */
-               if (inode->i_nlink > 1)
-                       inode_ref_streams(inode);
-               dentry->d_inode = inode;
-               inode_add_dentry(dentry, inode);
+               inode->i_ino = ino;
+               inode->i_devno = devno;
+               hlist_add_head(&inode->i_hlist, &table->array[pos]);
+               table->num_entries++;
+       have_inode:
+               d_associate(dentry, inode);
        }
        *dentry_ret = dentry;
        return 0;
index 0bd4a1f..0672c9c 100644 (file)
@@ -324,15 +324,7 @@ close_wimfs_fd(struct wimfs_fd *fd)
        if (fd->f_idx < inode->i_next_fd)
                inode->i_next_fd = fd->f_idx;
        FREE(fd);
-       if (--inode->i_num_opened_fds == 0) {
-               /* The last file descriptor to this inode was closed.  */
-               FREE(inode->i_fds);
-               inode->i_fds = NULL;
-               inode->i_num_allocated_fds = 0;
-               if (inode->i_nlink == 0)
-                       /* No links to this inode remain.  Get rid of it.  */
-                       free_inode(inode);
-       }
+       inode_dec_num_opened_fds(inode);
        return ret;
 }
 
@@ -1439,12 +1431,10 @@ wimfs_link(const char *existing_path, const char *new_path)
        if (new_dentry(new_name, &new_alias))
                return -ENOMEM;
 
-       new_alias->d_inode = inode;
-       inode_add_dentry(new_alias, inode);
+       inode_ref_streams(inode);
+       d_associate(new_alias, inode);
        dentry_add_child(dir, new_alias);
        touch_inode(dir->d_inode);
-       inode->i_nlink++;
-       inode_ref_streams(inode);
        return 0;
 }