fusefs: recycle vnodes after their last unlink

Previously fusefs would never recycle vnodes.  After VOP_INACTIVE, they'd
linger around until unmount or the vnlru reclaimed them.  This commit
essentially actives and inlines the old reclaim_revoked sysctl, and fixes
some issues dealing with the attribute cache and multiply linked files.

Sponsored by:	The FreeBSD Foundation

UPDATING

@@ -31,17 +31,17 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 13.x IS SLOW:
 	disable the most expensive debugging functionality run
 	"ln -s 'abort:false,junk:false' /etc/malloc.conf".)
 
-20190620:
+20190627:
 	The vfs.fusefs.sync_unmount and vfs.fusefs.init_backgrounded sysctls
 	and the "-o sync_unmount" and "-o init_backgrounded" mount options have
 	been removed from mount_fusefs(8).  You can safely remove them from
 	your scripts, because they had no effect.
 
 	The vfs.fusefs.fix_broken_io, vfs.fusefs.sync_resize,
-	vfs.fusefs.refresh_size, vfs.fusefs.mmap_enable, and
+	vfs.fusefs.refresh_size, vfs.fusefs.mmap_enable,
-	vfs.fusefs.data_cache_invalidate sysctls have been removed.  If you
+	vfs.fusefs.reclaim_revoked, and vfs.fusefs.data_cache_invalidate
-	felt the need to set any of them to a non-default value, please tell
+	sysctls have been removed.  If you felt the need to set any of them to
-	asomers@FreeBSD.org why.
+	a non-default value, please tell asomers@FreeBSD.org why.
 
 20190612:
 	Clang, llvm, lld, lldb, compiler-rt, libc++, libunwind and openmp have

share/man/man5/fusefs.5

@@ -98,7 +98,6 @@ Current number of allocated FUSE tickets, which is roughly equal to the number
 number of FUSE operations currently being processed by daemons.
 .\" Undocumented sysctls
 .\" ====================
-.\" vfs.fusefs.reclaim_revoked: I don't understand it well-enough
 .\" vfs.fusefs.enforce_dev_perms: I don't understand it well enough.
 .\" vfs.fusefs.iov_credit: I don't understand it well enough
 .\" vfs.fusefs.iov_permanent_bufsize: I don't understand it well enough

sys/fs/fuse/fuse_internal.c

@@ -657,6 +657,7 @@ fuse_internal_remove(struct vnode *dvp,
     enum fuse_opcode op)
 {
 	struct fuse_dispatcher fdi;
+	nlink_t nlink;
 	int err = 0;
 
 	fdisp_init(&fdi, cnp->cn_namelen + 1);
@@ -667,6 +668,35 @@ fuse_internal_remove(struct vnode *dvp,
 
 	err = fdisp_wait_answ(&fdi);
 	fdisp_destroy(&fdi);
+
+	if (err)
+		return (err);
+
+	/* 
+	 * Access the cached nlink even if the attr cached has expired.  If
+	 * it's inaccurate, the worst that will happen is:
+	 * 1) We'll recycle the vnode even though the file has another link we
+	 *    don't know about, costing a bit of cpu time, or
+	 * 2) We won't recycle the vnode even though all of its links are gone.
+	 *    It will linger around until vnlru reclaims it, costing a bit of
+	 *    temporary memory.
+	 */
+	nlink = VTOFUD(vp)->cached_attrs.va_nlink--;
+
+	/* 
+	 * Purge the parent's attribute cache because the daemon
+	 * should've updated its mtime and ctime.
+	 */
+	fuse_vnode_clear_attr_cache(dvp);
+
+	/* NB: nlink could be zero if it was never cached */
+	if (nlink <= 1 || vnode_vtype(vp) == VDIR) {
+		fuse_internal_vnode_disappear(vp);
+	} else {
+		cache_purge(vp);
+		fuse_vnode_update(vp, FN_CTIMECHANGE);
+	}
+
 	return err;
 }
 
@@ -894,8 +924,6 @@ fuse_internal_vnode_disappear(struct vnode *vp)
 
 	ASSERT_VOP_ELOCKED(vp, "fuse_internal_vnode_disappear");
 	fvdat->flag |= FN_REVOKED;
-	bintime_clear(&fvdat->attr_cache_timeout);
-	bintime_clear(&fvdat->entry_cache_timeout);
 	cache_purge(vp);
 }
 

sys/fs/fuse/fuse_vnops.c

@@ -218,15 +218,6 @@ struct vop_vector fuse_vnops = {
 	.vop_vptofh = fuse_vnop_vptofh,
 };
 
-/*
- * XXX: This feature is highly experimental and can bring to instabilities,
- * needs revisiting before to be enabled by default.
- */
-static int fuse_reclaim_revoked = 0;
-
-SYSCTL_INT(_vfs_fusefs, OID_AUTO, reclaim_revoked, CTLFLAG_RW,
-    &fuse_reclaim_revoked, 0, "");
-
 uma_zone_t fuse_pbuf_zone;
 
 #define fuse_vm_page_lock(m)		vm_page_lock((m));
@@ -880,9 +871,9 @@ fuse_vnop_inactive(struct vop_inactive_args *ap)
 		fuse_filehandle_close(vp, fufh, td, NULL);
 	}
 
-	if ((fvdat->flag & FN_REVOKED) != 0 && fuse_reclaim_revoked) {
+	if ((fvdat->flag & FN_REVOKED) != 0)
 		vrecycle(vp);
-	}
+
 	return 0;
 }
 
@@ -1568,18 +1559,9 @@ fuse_vnop_remove(struct vop_remove_args *ap)
 	if (vnode_isdir(vp)) {
 		return EPERM;
 	}
-	cache_purge(vp);
 
 	err = fuse_internal_remove(dvp, vp, cnp, FUSE_UNLINK);
 
-	if (err == 0) {
-		fuse_internal_vnode_disappear(vp);
-		/* 
-		 * Purge the parent's attribute cache because the daemon
-		 * should've updated its mtime and ctime
-		 */
-		fuse_vnode_clear_attr_cache(dvp);
-	}
 	return err;
 }
 
@@ -1691,14 +1673,6 @@ fuse_vnop_rmdir(struct vop_rmdir_args *ap)
 	}
 	err = fuse_internal_remove(dvp, vp, ap->a_cnp, FUSE_RMDIR);
 
-	if (err == 0) {
-		fuse_internal_vnode_disappear(vp);
-		/* 
-		 * Purge the parent's attribute cache because the daemon
-		 * should've updated its mtime and ctime
-		 */
-		fuse_vnode_clear_attr_cache(dvp);
-	}
 	return err;
 }
 

tests/sys/fs/fusefs/rmdir.cc

@@ -30,6 +30,7 @@
 
 extern "C" {
 #include <fcntl.h>
+#include <semaphore.h>
 }
 
 #include "mockfs.hh"
@@ -55,11 +56,13 @@ void expect_getattr(uint64_t ino, mode_t mode)
 	})));
 }
 
-void expect_lookup(const char *relpath, uint64_t ino)
+void expect_lookup(const char *relpath, uint64_t ino, int times=1)
 {
 	EXPECT_LOOKUP(FUSE_ROOT_ID, relpath)
-	.WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
+	.Times(times)
+	.WillRepeatedly(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
 		SET_OUT_HEADER_LEN(out, entry);
+		out.body.entry.attr_valid = UINT64_MAX;
 		out.body.entry.attr.mode = S_IFDIR | 0755;
 		out.body.entry.nodeid = ino;
 		out.body.entry.attr.nlink = 2;
@@ -83,13 +86,16 @@ void expect_rmdir(uint64_t parent, const char *relpath, int error)
  * A successful rmdir should clear the parent directory's attribute cache,
  * because the fuse daemon should update its mtime and ctime
  */
-TEST_F(Rmdir, clear_attr_cache)
+TEST_F(Rmdir, parent_attr_cache)
 {
 	const char FULLPATH[] = "mountpoint/some_dir";
 	const char RELPATH[] = "some_dir";
 	struct stat sb;
+	sem_t sem;
 	uint64_t ino = 42;
 
+	ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno);
+
 	EXPECT_CALL(*m_mock, process(
 		ResultOf([=](auto in) {
 			return (in.header.opcode == FUSE_GETATTR &&
@@ -105,9 +111,12 @@ TEST_F(Rmdir, clear_attr_cache)
 	})));
 	expect_lookup(RELPATH, ino);
 	expect_rmdir(FUSE_ROOT_ID, RELPATH, 0);
+	expect_forget(ino, 1, &sem);
 
 	ASSERT_EQ(0, rmdir(FULLPATH)) << strerror(errno);
 	EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno);
+	sem_wait(&sem);
+	sem_destroy(&sem);
 }
 
 TEST_F(Rmdir, enotempty)
@@ -124,15 +133,40 @@ TEST_F(Rmdir, enotempty)
 	ASSERT_EQ(ENOTEMPTY, errno);
 }
 
+/* Removing a directory should expire its entry cache */
+TEST_F(Rmdir, entry_cache)
+{
+	const char FULLPATH[] = "mountpoint/some_dir";
+	const char RELPATH[] = "some_dir";
+	sem_t sem;
+	uint64_t ino = 42;
+
+	expect_getattr(1, S_IFDIR | 0755);
+	expect_lookup(RELPATH, ino, 2);
+	expect_rmdir(FUSE_ROOT_ID, RELPATH, 0);
+	expect_forget(ino, 1, &sem);
+
+	ASSERT_EQ(0, rmdir(FULLPATH)) << strerror(errno);
+	ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno);
+	sem_wait(&sem);
+	sem_destroy(&sem);
+}
+
 TEST_F(Rmdir, ok)
 {
 	const char FULLPATH[] = "mountpoint/some_dir";
 	const char RELPATH[] = "some_dir";
+	sem_t sem;
 	uint64_t ino = 42;
 
+	ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno);
+
 	expect_getattr(FUSE_ROOT_ID, S_IFDIR | 0755);
 	expect_lookup(RELPATH, ino);
 	expect_rmdir(FUSE_ROOT_ID, RELPATH, 0);
+	expect_forget(ino, 1, &sem);
 
 	ASSERT_EQ(0, rmdir(FULLPATH)) << strerror(errno);
+	sem_wait(&sem);
+	sem_destroy(&sem);
 }

tests/sys/fs/fusefs/unlink.cc

@@ -29,6 +29,7 @@
 
 extern "C" {
 #include <fcntl.h>
+#include <semaphore.h>
 }
 
 #include "mockfs.hh"
@@ -54,18 +55,61 @@ void expect_getattr(uint64_t ino, mode_t mode)
 	})));
 }
 
-void expect_lookup(const char *relpath, uint64_t ino, int times)
+void expect_lookup(const char *relpath, uint64_t ino, int times, int nlink=1)
 {
-	FuseTest::expect_lookup(relpath, ino, S_IFREG | 0644, 0, times);
+	EXPECT_LOOKUP(FUSE_ROOT_ID, relpath)
+	.Times(times)
+	.WillRepeatedly(Invoke(
+		ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, entry);
+		out.body.entry.attr.mode = S_IFREG | 0644;
+		out.body.entry.nodeid = ino;
+		out.body.entry.attr.nlink = nlink;
+		out.body.entry.attr_valid = UINT64_MAX;
+		out.body.entry.attr.size = 0;
+	})));
 }
 
 };
 
+/*
+ * Unlinking a multiply linked file should update its ctime and nlink.  This
+ * could be handled simply by invalidating the attributes, necessitating a new
+ * GETATTR, but we implement it in-kernel for efficiency's sake.
+ */
+TEST_F(Unlink, attr_cache)
+{
+	const char FULLPATH0[] = "mountpoint/some_file.txt";
+	const char RELPATH0[] = "some_file.txt";
+	const char FULLPATH1[] = "mountpoint/other_file.txt";
+	const char RELPATH1[] = "other_file.txt";
+	uint64_t ino = 42;
+	struct stat sb_old, sb_new;
+	int fd1;
+
+	expect_getattr(1, S_IFDIR | 0755);
+	expect_lookup(RELPATH0, ino, 1, 2);
+	expect_lookup(RELPATH1, ino, 1, 2);
+	expect_open(ino, 0, 1);
+	expect_unlink(1, RELPATH0, 0);
+
+	fd1 = open(FULLPATH1, O_RDONLY);
+	ASSERT_LE(0, fd1) << strerror(errno);
+
+	ASSERT_EQ(0, fstat(fd1, &sb_old)) << strerror(errno);
+	ASSERT_EQ(0, unlink(FULLPATH0)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd1, &sb_new)) << strerror(errno);
+	EXPECT_NE(sb_old.st_ctime, sb_new.st_ctime);
+	EXPECT_EQ(1u, sb_new.st_nlink);
+
+	leak(fd1);
+}
+
 /*
  * A successful unlink should clear the parent directory's attribute cache,
  * because the fuse daemon should update its mtime and ctime
  */
-TEST_F(Unlink, clear_attr_cache)
+TEST_F(Unlink, parent_attr_cache)
 {
 	const char FULLPATH[] = "mountpoint/some_file.txt";
 	const char RELPATH[] = "some_file.txt";
@@ -85,7 +129,8 @@ TEST_F(Unlink, clear_attr_cache)
 		out.body.attr.attr.mode = S_IFDIR | 0755;
 		out.body.attr.attr_valid = UINT64_MAX;
 	})));
-	expect_lookup(RELPATH, ino, 1);
+	/* Use nlink=2 so we don't get a FUSE_FORGET */
+	expect_lookup(RELPATH, ino, 1, 2);
 	expect_unlink(1, RELPATH, 0);
 
 	ASSERT_EQ(0, unlink(FULLPATH)) << strerror(errno);
@@ -106,34 +151,101 @@ TEST_F(Unlink, eperm)
 	ASSERT_EQ(EPERM, errno);
 }
 
+/*
+ * Unlinking a file should expire its entry cache, even if it's multiply linked
+ */
+TEST_F(Unlink, entry_cache)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	uint64_t ino = 42;
+
+	expect_getattr(1, S_IFDIR | 0755);
+	expect_lookup(RELPATH, ino, 2, 2);
+	expect_unlink(1, RELPATH, 0);
+
+	ASSERT_EQ(0, unlink(FULLPATH)) << strerror(errno);
+	ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno);
+}
+
+/*
+ * Unlink a multiply-linked file.  There should be no FUSE_FORGET because the
+ * file is still linked.
+ */
+TEST_F(Unlink, multiply_linked)
+{
+	const char FULLPATH0[] = "mountpoint/some_file.txt";
+	const char RELPATH0[] = "some_file.txt";
+	const char FULLPATH1[] = "mountpoint/other_file.txt";
+	const char RELPATH1[] = "other_file.txt";
+	uint64_t ino = 42;
+
+	expect_getattr(1, S_IFDIR | 0755);
+	expect_lookup(RELPATH0, ino, 1, 2);
+	expect_unlink(1, RELPATH0, 0);
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([=](auto in) {
+			return (in.header.opcode == FUSE_FORGET &&
+				in.header.nodeid == ino);
+		}, Eq(true)),
+		_)
+	).Times(0);
+	expect_lookup(RELPATH1, ino, 1, 1);
+
+	ASSERT_EQ(0, unlink(FULLPATH0)) << strerror(errno);
+
+	/* 
+	 * The final syscall simply ensures that no FUSE_FORGET was ever sent,
+	 * by scheduling an arbitrary different operation after a FUSE_FORGET
+	 * would've been sent.
+	 */
+	ASSERT_EQ(0, access(FULLPATH1, F_OK)) << strerror(errno);
+}
+
 TEST_F(Unlink, ok)
 {
 	const char FULLPATH[] = "mountpoint/some_file.txt";
 	const char RELPATH[] = "some_file.txt";
 	uint64_t ino = 42;
+	sem_t sem;
+
+	ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno);
 
 	expect_getattr(1, S_IFDIR | 0755);
 	expect_lookup(RELPATH, ino, 1);
 	expect_unlink(1, RELPATH, 0);
+	expect_forget(ino, 1, &sem);
 
 	ASSERT_EQ(0, unlink(FULLPATH)) << strerror(errno);
+	sem_wait(&sem);
+	sem_destroy(&sem);
 }
 
 /* Unlink an open file */
 TEST_F(Unlink, open_but_deleted)
 {
-	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char FULLPATH0[] = "mountpoint/some_file.txt";
-	const char RELPATH[] = "some_file.txt";
+	const char RELPATH0[] = "some_file.txt";
+	const char FULLPATH1[] = "mountpoint/other_file.txt";
+	const char RELPATH1[] = "other_file.txt";
 	uint64_t ino = 42;
 	int fd;
 
 	expect_getattr(1, S_IFDIR | 0755);
-	expect_lookup(RELPATH, ino, 2);
+	expect_lookup(RELPATH0, ino, 2);
 	expect_open(ino, 0, 1);
-	expect_unlink(1, RELPATH, 0);
+	expect_unlink(1, RELPATH0, 0);
+	expect_lookup(RELPATH1, ino, 1, 1);
 
-	fd = open(FULLPATH, O_RDWR);
+	fd = open(FULLPATH0, O_RDWR);
 	ASSERT_LE(0, fd) << strerror(errno);
-	ASSERT_EQ(0, unlink(FULLPATH)) << strerror(errno);
+	ASSERT_EQ(0, unlink(FULLPATH0)) << strerror(errno);
+
+	/* 
+	 * The final syscall simply ensures that no FUSE_FORGET was ever sent,
+	 * by scheduling an arbitrary different operation after a FUSE_FORGET
+	 * would've been sent.
+	 */
+	ASSERT_EQ(0, access(FULLPATH1, F_OK)) << strerror(errno);
 	leak(fd);
 }