From: Eric Biggers <ebiggers3@gmail.com>
Date: Sat, 24 Jan 2015 16:20:12 +0000 (-0600)
Subject: Clean up inode alias tracking
X-Git-Tag: v1.8.0~58
X-Git-Url: https://wimlib.net/git/?p=wimlib;a=commitdiff_plain;h=b5fae0e5ddac332b25856e3df7556aa3ee7f69fb

Clean up inode alias tracking

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.
---

diff --git a/include/wimlib/inode.h b/include/wimlib/inode.h
index 4b956775..11c2361b 100644
--- a/include/wimlib/inode.h
+++ b/include/wimlib/inode.h
@@ -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.
diff --git a/src/dentry.c b/src/dentry.c
index 862c6640..38b4efa6 100644
--- a/src/dentry.c
+++ b/src/dentry.c
@@ -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);
 	}
 }
diff --git a/src/inode.c b/src/inode.c
index 9e4177c9..6384b11c 100644
--- a/src/inode.c
+++ b/src/inode.c
@@ -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
diff --git a/src/inode_fixup.c b/src/inode_fixup.c
index 616c5ff3..76245623 100644
--- a/src/inode_fixup.c
+++ b/src/inode_fixup.c
@@ -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;
 	}
 
diff --git a/src/inode_table.c b/src/inode_table.c
index 6e1c143b..49dd8217 100644
--- a/src/inode_table.c
+++ b/src/inode_table.c
@@ -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;
diff --git a/src/mount_image.c b/src/mount_image.c
index 0bd4a1f1..0672c9c2 100644
--- a/src/mount_image.c
+++ b/src/mount_image.c
@@ -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;
 }