Date: Thu, 27 Jun 2019 20:18:13 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r349468 - in projects/fuse2: . share/man/man5 sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201906272018.x5RKIDtF035779@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Thu Jun 27 20:18:12 2019 New Revision: 349468 URL: https://svnweb.freebsd.org/changeset/base/349468 Log: 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 Modified: projects/fuse2/UPDATING projects/fuse2/share/man/man5/fusefs.5 projects/fuse2/sys/fs/fuse/fuse_internal.c projects/fuse2/sys/fs/fuse/fuse_vnops.c projects/fuse2/tests/sys/fs/fusefs/rmdir.cc projects/fuse2/tests/sys/fs/fusefs/unlink.cc Modified: projects/fuse2/UPDATING ============================================================================== --- projects/fuse2/UPDATING Thu Jun 27 19:36:30 2019 (r349467) +++ projects/fuse2/UPDATING Thu Jun 27 20:18:12 2019 (r349468) @@ -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.data_cache_invalidate sysctls have been removed. If you - felt the need to set any of them to a non-default value, please tell - asomers@FreeBSD.org why. + vfs.fusefs.refresh_size, vfs.fusefs.mmap_enable, + vfs.fusefs.reclaim_revoked, and vfs.fusefs.data_cache_invalidate + sysctls have been removed. If you felt the need to set any of them to + a non-default value, please tell asomers@FreeBSD.org why. 20190612: Clang, llvm, lld, lldb, compiler-rt, libc++, libunwind and openmp have Modified: projects/fuse2/share/man/man5/fusefs.5 ============================================================================== --- projects/fuse2/share/man/man5/fusefs.5 Thu Jun 27 19:36:30 2019 (r349467) +++ projects/fuse2/share/man/man5/fusefs.5 Thu Jun 27 20:18:12 2019 (r349468) @@ -98,7 +98,6 @@ Current number of allocated FUSE tickets, which is rou 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 Modified: projects/fuse2/sys/fs/fuse/fuse_internal.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_internal.c Thu Jun 27 19:36:30 2019 (r349467) +++ projects/fuse2/sys/fs/fuse/fuse_internal.c Thu Jun 27 20:18:12 2019 (r349468) @@ -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); } Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_vnops.c Thu Jun 27 19:36:30 2019 (r349467) +++ projects/fuse2/sys/fs/fuse/fuse_vnops.c Thu Jun 27 20:18:12 2019 (r349468) @@ -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; } Modified: projects/fuse2/tests/sys/fs/fusefs/rmdir.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/rmdir.cc Thu Jun 27 19:36:30 2019 (r349467) +++ projects/fuse2/tests/sys/fs/fusefs/rmdir.cc Thu Jun 27 20:18:12 2019 (r349468) @@ -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 * 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); } Modified: projects/fuse2/tests/sys/fs/fusefs/unlink.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/unlink.cc Thu Jun 27 19:36:30 2019 (r349467) +++ projects/fuse2/tests/sys/fs/fusefs/unlink.cc Thu Jun 27 20:18:12 2019 (r349468) @@ -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 RELPATH[] = "some_file.txt"; + 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; 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); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201906272018.x5RKIDtF035779>