From 002e54b0aa8f651012fced4223b4cad484f2d08d Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Thu, 9 May 2019 01:16:34 +0000 Subject: [PATCH] fusefs: clear a dir's attr cache when its contents change Any change to a directory's contents should cause its mtime and ctime to be updated by the FUSE daemon. Clear its attribute cache so we'll get the new attributs the next time that they're needed. This affects the following VOPs: VOP_CREATE, VOP_LINK, VOP_MKDIR, VOP_MKNOD, VOP_REMOVE, VOP_RMDIR, and VOP_SYMLINK Reported by: pjdfstest Sponsored by: The FreeBSD Foundation --- sys/fs/fuse/fuse_internal.c | 7 +++ sys/fs/fuse/fuse_node.h | 6 +++ sys/fs/fuse/fuse_vnops.c | 29 ++++++++++-- tests/sys/fs/fusefs/create.cc | 44 +++++++++++++++++- tests/sys/fs/fusefs/link.cc | 85 +++++++++++++++++++++++++++------- tests/sys/fs/fusefs/mockfs.cc | 3 ++ tests/sys/fs/fusefs/rmdir.cc | 61 +++++++++++++++++------- tests/sys/fs/fusefs/symlink.cc | 74 ++++++++++++++++++++++------- tests/sys/fs/fusefs/unlink.cc | 31 +++++++++++++ 9 files changed, 286 insertions(+), 54 deletions(-) diff --git a/sys/fs/fuse/fuse_internal.c b/sys/fs/fuse/fuse_internal.c index 8676d94455d7..b2c3345d5bf9 100644 --- a/sys/fs/fuse/fuse_internal.c +++ b/sys/fs/fuse/fuse_internal.c @@ -535,6 +535,13 @@ fuse_internal_newentry_core(struct vnode *dvp, feo->nodeid, 1); return err; } + + /* + * Purge the parent's attribute cache because the daemon should've + * updated its mtime and ctime + */ + fuse_vnode_clear_attr_cache(dvp); + fuse_internal_cache_attrs(*vpp, &feo->attr, feo->attr_valid, feo->attr_valid_nsec, NULL); diff --git a/sys/fs/fuse/fuse_node.h b/sys/fs/fuse/fuse_node.h index d1ac998bd786..315635d55db3 100644 --- a/sys/fs/fuse/fuse_node.h +++ b/sys/fs/fuse/fuse_node.h @@ -110,6 +110,12 @@ VTOVA(struct vnode *vp) return NULL; } +static inline void +fuse_vnode_clear_attr_cache(struct vnode *vp) +{ + bintime_clear(&VTOFUD(vp)->attr_cache_timeout); +} + #define VTOILLU(vp) ((uint64_t)(VTOFUD(vp) ? VTOI(vp) : 0)) #define FUSE_NULL_ID 0 diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c index eae413e86bea..953e01aee8a8 100644 --- a/sys/fs/fuse/fuse_vnops.c +++ b/sys/fs/fuse/fuse_vnops.c @@ -625,6 +625,11 @@ fuse_vnop_create(struct vop_create_args *ap) fuse_filehandle_init(*vpp, FUFH_RDWR, NULL, td, cred, foo); fuse_vnode_open(*vpp, foo->open_flags, td); + /* + * Purge the parent's attribute cache because the daemon should've + * updated its mtime and ctime + */ + fuse_vnode_clear_attr_cache(dvp); cache_purge_negative(dvp); out: @@ -817,9 +822,15 @@ fuse_vnop_link(struct vop_link_args *ap) feo = fdi.answ; err = fuse_internal_checkentry(feo, vnode_vtype(vp)); - if (!err) + if (!err) { + /* + * Purge the parent's attribute cache because the daemon + * should've updated its mtime and ctime + */ + fuse_vnode_clear_attr_cache(tdvp); fuse_internal_cache_attrs(vp, &feo->attr, feo->attr_valid, feo->attr_valid_nsec, NULL); + } out: fdisp_destroy(&fdi); return err; @@ -1398,8 +1409,14 @@ fuse_vnop_remove(struct vop_remove_args *ap) err = fuse_internal_remove(dvp, vp, cnp, FUSE_UNLINK); - if (err == 0) + 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; } @@ -1511,8 +1528,14 @@ fuse_vnop_rmdir(struct vop_rmdir_args *ap) } err = fuse_internal_remove(dvp, vp, ap->a_cnp, FUSE_RMDIR); - if (err == 0) + 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; } diff --git a/tests/sys/fs/fusefs/create.cc b/tests/sys/fs/fusefs/create.cc index d1225e2517a9..57e00b3ec8ce 100644 --- a/tests/sys/fs/fusefs/create.cc +++ b/tests/sys/fs/fusefs/create.cc @@ -59,7 +59,7 @@ void expect_create(const char *relpath, mode_t mode, ProcessMockerT r) }; /* - * If FUSE_CREATE sets the attr_valid, then subsequent GETATTRs should use the + * If FUSE_CREATE sets attr_valid, then subsequent GETATTRs should use the * attribute cache */ TEST_F(Create, attr_cache) @@ -93,6 +93,48 @@ TEST_F(Create, attr_cache) /* Deliberately leak fd. close(2) will be tested in release.cc */ } +/* A successful CREATE operation should purge the parent dir's attr cache */ +TEST_F(Create, clear_attr_cache) +{ + const char FULLPATH[] = "mountpoint/src"; + const char RELPATH[] = "src"; + mode_t mode = S_IFREG | 0755; + uint64_t ino = 42; + int fd; + struct stat sb; + + EXPECT_LOOKUP(1, RELPATH).WillOnce(Invoke(ReturnErrno(ENOENT))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_GETATTR && + in->header.nodeid == 1); + }, Eq(true)), + _) + ).Times(2) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto out) { + SET_OUT_HEADER_LEN(out, attr); + out->body.attr.attr.ino = 1; + out->body.attr.attr.mode = S_IFDIR | 0755; + out->body.attr.attr_valid = UINT64_MAX; + }))); + + expect_create(RELPATH, mode, + ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, create); + out->body.create.entry.attr.mode = mode; + out->body.create.entry.nodeid = ino; + out->body.create.entry.entry_valid = UINT64_MAX; + out->body.create.entry.attr_valid = UINT64_MAX; + })); + + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); + fd = open(FULLPATH, O_CREAT | O_EXCL, mode); + EXPECT_LE(0, fd) << strerror(errno); + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); + + /* Deliberately leak fd. close(2) will be tested in release.cc */ +} + /* * The fuse daemon fails the request with EEXIST. This usually indicates a * race condition: some other FUSE client created the file in between when the diff --git a/tests/sys/fs/fusefs/link.cc b/tests/sys/fs/fusefs/link.cc index 790f37963dab..471952dfa35c 100644 --- a/tests/sys/fs/fusefs/link.cc +++ b/tests/sys/fs/fusefs/link.cc @@ -39,12 +39,78 @@ using namespace testing; class Link: public FuseTest { public: +void expect_link(uint64_t ino, const char *relpath, mode_t mode, uint32_t nlink) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + const char *name = (const char*)in->body.bytes + + sizeof(struct fuse_link_in); + return (in->header.opcode == FUSE_LINK && + in->body.link.oldnodeid == ino && + (0 == strcmp(name, relpath))); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, entry); + out->body.entry.nodeid = ino; + out->body.entry.attr.mode = mode; + out->body.entry.attr.nlink = nlink; + out->body.entry.attr_valid = UINT64_MAX; + out->body.entry.entry_valid = UINT64_MAX; + }))); +} + void expect_lookup(const char *relpath, uint64_t ino) { FuseTest::expect_lookup(relpath, ino, S_IFREG | 0644, 0, 1); } }; +/* + * A successful link should clear the parent directory's attribute cache, + * because the fuse daemon should update its mtime and ctime + */ +TEST_F(Link, clear_attr_cache) +{ + const char FULLPATH[] = "mountpoint/src"; + const char RELPATH[] = "src"; + const char FULLDST[] = "mountpoint/dst"; + const char RELDST[] = "dst"; + const uint64_t ino = 42; + mode_t mode = S_IFREG | 0644; + struct stat sb; + + EXPECT_LOOKUP(1, RELPATH).WillOnce(Invoke(ReturnErrno(ENOENT))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_GETATTR && + in->header.nodeid == 1); + }, Eq(true)), + _) + ).Times(2) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto out) { + SET_OUT_HEADER_LEN(out, attr); + out->body.attr.attr.ino = 1; + out->body.attr.attr.mode = S_IFDIR | 0755; + out->body.attr.attr_valid = UINT64_MAX; + }))); + + EXPECT_LOOKUP(1, RELDST) + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, entry); + out->body.entry.attr.mode = mode; + out->body.entry.nodeid = ino; + out->body.entry.attr.nlink = 1; + out->body.entry.attr_valid = UINT64_MAX; + out->body.entry.entry_valid = UINT64_MAX; + }))); + expect_link(ino, RELPATH, mode, 2); + + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); + EXPECT_EQ(0, link(FULLDST, FULLPATH)) << strerror(errno); + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); +} + TEST_F(Link, emlink) { const char FULLPATH[] = "mountpoint/lnk"; @@ -91,24 +157,7 @@ TEST_F(Link, ok) out->body.entry.attr_valid = UINT64_MAX; out->body.entry.entry_valid = UINT64_MAX; }))); - - EXPECT_CALL(*m_mock, process( - ResultOf([=](auto in) { - const char *name = (const char*)in->body.bytes - + sizeof(struct fuse_link_in); - return (in->header.opcode == FUSE_LINK && - in->body.link.oldnodeid == ino && - (0 == strcmp(name, RELPATH))); - }, Eq(true)), - _) - ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { - SET_OUT_HEADER_LEN(out, entry); - out->body.entry.attr.mode = mode; - out->body.entry.nodeid = ino; - out->body.entry.attr.nlink = 2; - out->body.entry.attr_valid = UINT64_MAX; - out->body.entry.entry_valid = UINT64_MAX; - }))); + expect_link(ino, RELPATH, mode, 2); ASSERT_EQ(0, link(FULLDST, FULLPATH)) << strerror(errno); // Check that the original file's nlink count has increased. diff --git a/tests/sys/fs/fusefs/mockfs.cc b/tests/sys/fs/fusefs/mockfs.cc index c5c6e30366f2..c80d2c8a5732 100644 --- a/tests/sys/fs/fusefs/mockfs.cc +++ b/tests/sys/fs/fusefs/mockfs.cc @@ -187,6 +187,9 @@ void debug_fuseop(const mockfs_buf_in *in) case FUSE_INTERRUPT: printf(" unique=%lu", in->body.interrupt.unique); break; + case FUSE_LINK: + printf(" oldnodeid=%lu", in->body.link.oldnodeid); + break; case FUSE_LOOKUP: printf(" %s", in->body.lookup); break; diff --git a/tests/sys/fs/fusefs/rmdir.cc b/tests/sys/fs/fusefs/rmdir.cc index e4c42c233d3e..434ea658f3c3 100644 --- a/tests/sys/fs/fusefs/rmdir.cc +++ b/tests/sys/fs/fusefs/rmdir.cc @@ -65,8 +65,51 @@ void expect_lookup(const char *relpath, uint64_t ino) out->body.entry.attr.nlink = 2; }))); } + +void expect_rmdir(uint64_t parent, const char *relpath, int error) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_RMDIR && + 0 == strcmp(relpath, in->body.rmdir) && + in->header.nodeid == parent); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnErrno(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) +{ + const char FULLPATH[] = "mountpoint/some_dir"; + const char RELPATH[] = "some_dir"; + struct stat sb; + uint64_t ino = 42; + + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_GETATTR && + in->header.nodeid == 1); + }, Eq(true)), + _) + ).Times(2) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto out) { + SET_OUT_HEADER_LEN(out, attr); + out->body.attr.attr.ino = ino; // Must match nodeid + out->body.attr.attr.mode = S_IFDIR | 0755; + out->body.attr.attr_valid = UINT64_MAX; + }))); + expect_lookup(RELPATH, ino); + expect_rmdir(1, RELPATH, 0); + + ASSERT_EQ(0, rmdir(FULLPATH)) << strerror(errno); + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); +} + TEST_F(Rmdir, enotempty) { const char FULLPATH[] = "mountpoint/some_dir"; @@ -75,14 +118,7 @@ TEST_F(Rmdir, enotempty) expect_getattr(1, S_IFDIR | 0755); expect_lookup(RELPATH, ino); - EXPECT_CALL(*m_mock, process( - ResultOf([=](auto in) { - return (in->header.opcode == FUSE_RMDIR && - 0 == strcmp(RELPATH, in->body.rmdir) && - in->header.nodeid == 1); - }, Eq(true)), - _) - ).WillOnce(Invoke(ReturnErrno(ENOTEMPTY))); + expect_rmdir(1, RELPATH, ENOTEMPTY); ASSERT_NE(0, rmdir(FULLPATH)); ASSERT_EQ(ENOTEMPTY, errno); @@ -96,14 +132,7 @@ TEST_F(Rmdir, ok) expect_getattr(1, S_IFDIR | 0755); expect_lookup(RELPATH, ino); - EXPECT_CALL(*m_mock, process( - ResultOf([=](auto in) { - return (in->header.opcode == FUSE_RMDIR && - 0 == strcmp(RELPATH, in->body.rmdir) && - in->header.nodeid == 1); - }, Eq(true)), - _) - ).WillOnce(Invoke(ReturnErrno(0))); + expect_rmdir(1, RELPATH, 0); ASSERT_EQ(0, rmdir(FULLPATH)) << strerror(errno); } diff --git a/tests/sys/fs/fusefs/symlink.cc b/tests/sys/fs/fusefs/symlink.cc index d1bfa8d56e20..99dc3086a2cc 100644 --- a/tests/sys/fs/fusefs/symlink.cc +++ b/tests/sys/fs/fusefs/symlink.cc @@ -37,7 +37,63 @@ extern "C" { using namespace testing; -class Symlink: public FuseTest {}; +class Symlink: public FuseTest { +public: + +void expect_symlink(uint64_t ino, const char *target, const char *relpath) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + const char *name = (const char*)in->body.bytes; + const char *linkname = name + strlen(name) + 1; + return (in->header.opcode == FUSE_SYMLINK && + (0 == strcmp(linkname, target)) && + (0 == strcmp(name, relpath))); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, entry); + out->body.entry.attr.mode = S_IFLNK | 0777; + out->body.entry.nodeid = ino; + out->body.entry.entry_valid = UINT64_MAX; + out->body.entry.attr_valid = UINT64_MAX; + }))); +} + +}; + +/* + * A successful symlink should clear the parent directory's attribute cache, + * because the fuse daemon should update its mtime and ctime + */ +TEST_F(Symlink, clear_attr_cache) +{ + const char FULLPATH[] = "mountpoint/src"; + const char RELPATH[] = "src"; + const char dst[] = "dst"; + const uint64_t ino = 42; + struct stat sb; + + EXPECT_LOOKUP(1, RELPATH).WillOnce(Invoke(ReturnErrno(ENOENT))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_GETATTR && + in->header.nodeid == 1); + }, Eq(true)), + _) + ).Times(2) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto out) { + SET_OUT_HEADER_LEN(out, attr); + out->body.attr.attr.ino = 1; + out->body.attr.attr.mode = S_IFDIR | 0755; + out->body.attr.attr_valid = UINT64_MAX; + }))); + expect_symlink(ino, dst, RELPATH); + + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); + EXPECT_EQ(0, symlink(dst, FULLPATH)) << strerror(errno); + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); +} TEST_F(Symlink, enospc) { @@ -70,21 +126,7 @@ TEST_F(Symlink, ok) const uint64_t ino = 42; EXPECT_LOOKUP(1, RELPATH).WillOnce(Invoke(ReturnErrno(ENOENT))); - - EXPECT_CALL(*m_mock, process( - ResultOf([=](auto in) { - const char *name = (const char*)in->body.bytes; - const char *linkname = name + strlen(name) + 1; - return (in->header.opcode == FUSE_SYMLINK && - (0 == strcmp(linkname, dst)) && - (0 == strcmp(name, RELPATH))); - }, Eq(true)), - _) - ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { - SET_OUT_HEADER_LEN(out, entry); - out->body.entry.attr.mode = S_IFLNK | 0777; - out->body.entry.nodeid = ino; - }))); + expect_symlink(ino, dst, RELPATH); EXPECT_EQ(0, symlink(dst, FULLPATH)) << strerror(errno); } diff --git a/tests/sys/fs/fusefs/unlink.cc b/tests/sys/fs/fusefs/unlink.cc index a807b9a7e0dc..e98d944e63f2 100644 --- a/tests/sys/fs/fusefs/unlink.cc +++ b/tests/sys/fs/fusefs/unlink.cc @@ -61,6 +61,37 @@ void expect_lookup(const char *relpath, uint64_t ino, int times) }; +/* + * 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) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + struct stat sb; + uint64_t ino = 42; + + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_GETATTR && + in->header.nodeid == 1); + }, Eq(true)), + _) + ).Times(2) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto out) { + SET_OUT_HEADER_LEN(out, attr); + out->body.attr.attr.ino = ino; // Must match nodeid + out->body.attr.attr.mode = S_IFDIR | 0755; + out->body.attr.attr_valid = UINT64_MAX; + }))); + expect_lookup(RELPATH, ino, 1); + expect_unlink(1, RELPATH, 0); + + ASSERT_EQ(0, unlink(FULLPATH)) << strerror(errno); + EXPECT_EQ(0, stat("mountpoint", &sb)) << strerror(errno); +} + TEST_F(Unlink, eperm) { const char FULLPATH[] = "mountpoint/some_file.txt";