FreeBSD Bugzilla – Attachment 212269 Details for
Bug 244178
[FUSEFS]: underlying file modified leads to corrupted fuse data
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
invalidate cache when a file changes unexpectedly
pr244178.diff (text/plain), 14.08 KB, created by
Alan Somers
on 2020-03-09 00:25:35 UTC
(
hide
)
Description:
invalidate cache when a file changes unexpectedly
Filename:
MIME Type:
Creator:
Alan Somers
Created:
2020-03-09 00:25:35 UTC
Size:
14.08 KB
patch
obsolete
>Index: sys/fs/fuse/fuse_internal.c >=================================================================== >--- sys/fs/fuse/fuse_internal.c (revision 358258) >+++ sys/fs/fuse/fuse_internal.c (working copy) >@@ -856,6 +856,9 @@ > fdisp_destroy(&fdi); > } > >+SDT_PROBE_DEFINE2(fusefs, , internal, getattr_cache_incoherent, >+ "struct vnode*", "struct fuse_attr_out*"); >+ > /* Fetch the vnode's attributes from the daemon*/ > int > fuse_internal_do_getattr(struct vnode *vp, struct vattr *vap, >@@ -898,6 +901,20 @@ > fao->attr.mtime = old_mtime.tv_sec; > fao->attr.mtimensec = old_mtime.tv_nsec; > } >+ if (vnode_isreg(vp) && >+ fvdat->cached_attrs.va_size != VNOVAL && >+ fao->attr.size != fvdat->cached_attrs.va_size) { >+ /* >+ * The server changed the file's size even though we had it >+ * cached! That's a server bug. >+ */ >+ SDT_PROBE2(fusefs, , internal, getattr_cache_incoherent, vp, >+ fao); >+ printf("%s: cache incoherent on %s!\n", __func__, >+ vnode_mount(vp)->mnt_stat.f_mntonname); >+ int iosize = fuse_iosize(vp); >+ v_inval_buf_range(vp, 0, INT64_MAX, iosize); >+ } > fuse_internal_cache_attrs(vp, &fao->attr, fao->attr_valid, > fao->attr_valid_nsec, vap); > if (vtyp != vnode_vtype(vp)) { >Index: sys/fs/fuse/fuse_node.c >=================================================================== >--- sys/fs/fuse/fuse_node.c (revision 358258) >+++ sys/fs/fuse/fuse_node.c (working copy) >@@ -450,7 +450,8 @@ > int error = 0; > > if (!(fvdat->flag & FN_SIZECHANGE) && >- (VTOVA(vp) == NULL || fvdat->cached_attrs.va_size == VNOVAL)) >+ (!fuse_vnode_attr_cache_valid(vp) || >+ fvdat->cached_attrs.va_size == VNOVAL)) > error = fuse_internal_do_getattr(vp, NULL, cred, td); > > if (!error) >Index: sys/fs/fuse/fuse_node.h >=================================================================== >--- sys/fs/fuse/fuse_node.h (revision 358258) >+++ sys/fs/fuse/fuse_node.h (working copy) >@@ -134,13 +134,18 @@ > #define VTOFUD(vp) \ > ((struct fuse_vnode_data *)((vp)->v_data)) > #define VTOI(vp) (VTOFUD(vp)->nid) >-static inline struct vattr* >-VTOVA(struct vnode *vp) >+static inline bool fuse_vnode_attr_cache_valid(struct vnode *vp) > { > struct bintime now; > > getbinuptime(&now); >- if (bintime_cmp(&(VTOFUD(vp)->attr_cache_timeout), &now, >)) >+ return (bintime_cmp(&(VTOFUD(vp)->attr_cache_timeout), &now, >)); >+} >+ >+static inline struct vattr* >+VTOVA(struct vnode *vp) >+{ >+ if (fuse_vnode_attr_cache_valid(vp)) > return &(VTOFUD(vp)->cached_attrs); > else > return NULL; >Index: sys/fs/fuse/fuse_vnops.c >=================================================================== >--- sys/fs/fuse/fuse_vnops.c (revision 358258) >+++ sys/fs/fuse/fuse_vnops.c (working copy) >@@ -961,6 +961,8 @@ > > SDT_PROBE_DEFINE3(fusefs, , vnops, cache_lookup, > "int", "struct timespec*", "struct timespec*"); >+SDT_PROBE_DEFINE2(fusefs, , vnops, lookup_cache_incoherent, >+ "struct vnode*", "struct fuse_entry_out*"); > /* > struct vnop_lookup_args { > struct vnodeop_desc *a_desc; >@@ -1137,6 +1139,7 @@ > *vpp = dvp; > } else { > struct fuse_vnode_data *fvdat; >+ struct vattr *vap; > > err = fuse_vnode_get(vnode_mount(dvp), feo, nid, dvp, > &vp, cnp, vtyp); >@@ -1157,22 +1160,23 @@ > */ > fvdat = VTOFUD(vp); > if (vnode_isreg(vp) && >- filesize != fvdat->cached_attrs.va_size && >- fvdat->flag & FN_SIZECHANGE) { >+ ((filesize != fvdat->cached_attrs.va_size && >+ fvdat->flag & FN_SIZECHANGE) || >+ ((vap = VTOVA(vp)) && >+ filesize != vap->va_size))) >+ { >+ SDT_PROBE2(fusefs, , vnops, lookup_cache_incoherent, vp, feo); >+ fvdat->flag &= ~FN_SIZECHANGE; > /* >- * The FN_SIZECHANGE flag reflects a dirty >- * append. If userspace lets us know our cache >- * is invalid, that write was lost. (Dirty >- * writes that do not cause append are also >- * lost, but we don't detect them here.) >- * >- * XXX: Maybe disable WB caching on this mount. >+ * The server changed the file's size even >+ * though we had it cached, or had dirty writes >+ * in the WB cache! > */ >- printf("%s: WB cache incoherent on %s!\n", >+ printf("%s: cache incoherent on %s!\n", > __func__, > vnode_mount(vp)->mnt_stat.f_mntonname); >- >- fvdat->flag &= ~FN_SIZECHANGE; >+ int iosize = fuse_iosize(vp); >+ v_inval_buf_range(vp, 0, INT64_MAX, iosize); > } > > MPASS(feo != NULL); >Index: tests/sys/fs/fusefs/Makefile >=================================================================== >--- tests/sys/fs/fusefs/Makefile (revision 358797) >+++ tests/sys/fs/fusefs/Makefile (working copy) >@@ -12,6 +12,7 @@ > GTESTS+= access > GTESTS+= allow_other > GTESTS+= bmap >+GTESTS+= cache > GTESTS+= create > GTESTS+= default_permissions > GTESTS+= default_permissions_privileged >Index: tests/sys/fs/fusefs/cache.cc >=================================================================== >--- tests/sys/fs/fusefs/cache.cc (nonexistent) >+++ tests/sys/fs/fusefs/cache.cc (working copy) >@@ -0,0 +1,219 @@ >+/*- >+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD >+ * >+ * Copyright (c) 2020 Alan Somers >+ * >+ * Redistribution and use in source and binary forms, with or without >+ * modification, are permitted provided that the following conditions >+ * are met: >+ * 1. Redistributions of source code must retain the above copyright >+ * notice, this list of conditions and the following disclaimer. >+ * 2. Redistributions in binary form must reproduce the above copyright >+ * notice, this list of conditions and the following disclaimer in the >+ * documentation and/or other materials provided with the distribution. >+ * >+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND >+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE >+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE >+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE >+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL >+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS >+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) >+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT >+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY >+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF >+ * SUCH DAMAGE. >+ * >+ * $FreeBSD$ >+ */ >+ >+extern "C" { >+#include <sys/param.h> >+#include <fcntl.h> >+} >+ >+#include "mockfs.hh" >+#include "utils.hh" >+ >+/* >+ * Tests for thorny cache problems not specific to any one opcode >+ */ >+ >+using namespace testing; >+ >+/* >+ * Parameters >+ * - reopen file - If true, close and reopen the file between reads >+ * - cache lookups - If true, allow lookups to be cached >+ * - cache attrs - If true, allow file attributes to be cached >+ * - cache_mode - uncached, writeback, or writethrough >+ * - initial size - File size before truncation >+ * - truncated size - File size after truncation >+ */ >+typedef tuple<tuple<bool, bool, bool>, cache_mode, ssize_t, ssize_t> CacheParam; >+ >+class Cache: public FuseTest, public WithParamInterface<CacheParam> { >+public: >+bool m_direct_io; >+ >+Cache(): m_direct_io(false) {}; >+ >+virtual void SetUp() { >+ int cache_mode = get<1>(GetParam()); >+ switch (cache_mode) { >+ case Uncached: >+ m_direct_io = true; >+ break; >+ case WritebackAsync: >+ m_async = true; >+ /* FALLTHROUGH */ >+ case Writeback: >+ m_init_flags |= FUSE_WRITEBACK_CACHE; >+ /* FALLTHROUGH */ >+ case Writethrough: >+ break; >+ default: >+ FAIL() << "Unknown cache mode"; >+ } >+ >+ FuseTest::SetUp(); >+ if (IsSkipped()) >+ return; >+} >+ >+void expect_getattr(uint64_t ino, int times, uint64_t size, uint64_t attr_valid) >+{ >+ EXPECT_CALL(*m_mock, process( >+ ResultOf([=](auto in) { >+ return (in.header.opcode == FUSE_GETATTR && >+ in.header.nodeid == ino); >+ }, Eq(true)), >+ _) >+ ).Times(times) >+ .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto& out) { >+ SET_OUT_HEADER_LEN(out, attr); >+ out.body.attr.attr_valid = attr_valid; >+ out.body.attr.attr.ino = ino; >+ out.body.attr.attr.mode = S_IFREG | 0644; >+ out.body.attr.attr.size = size; >+ }))); >+} >+ >+void expect_lookup(const char *relpath, uint64_t ino, >+ uint64_t size, uint64_t entry_valid, uint64_t attr_valid) >+{ >+ EXPECT_LOOKUP(FUSE_ROOT_ID, relpath) >+ .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 = 1; >+ out.body.entry.attr_valid = attr_valid; >+ out.body.entry.attr.size = size; >+ out.body.entry.entry_valid = entry_valid; >+ }))); >+} >+ >+void expect_open(uint64_t ino, int times) >+{ >+ FuseTest::expect_open(ino, m_direct_io ? FOPEN_DIRECT_IO: 0, times); >+} >+ >+void expect_release(uint64_t ino, ProcessMockerT r) >+{ >+ EXPECT_CALL(*m_mock, process( >+ ResultOf([=](auto in) { >+ return (in.header.opcode == FUSE_RELEASE && >+ in.header.nodeid == ino); >+ }, Eq(true)), >+ _) >+ ).WillRepeatedly(Invoke(r)); >+} >+ >+}; >+ >+// If the server truncates the file behind the kernel's back, the kernel should >+// invalidate cached pages beyond the new EOF >+TEST_P(Cache, truncate_by_surprise_invalidates_cache) >+{ >+ const char FULLPATH[] = "mountpoint/some_file.txt"; >+ const char RELPATH[] = "some_file.txt"; >+ const char *CONTENTS = "abcdefghijklmnopqrstuvwxyz"; >+ uint64_t ino = 42; >+ uint64_t attr_valid, entry_valid; >+ int fd; >+ ssize_t bufsize = strlen(CONTENTS); >+ uint8_t buf[bufsize]; >+ bool reopen = get<0>(get<0>(GetParam())); >+ bool cache_lookups = get<1>(get<0>(GetParam())); >+ bool cache_attrs = get<2>(get<0>(GetParam())); >+ ssize_t osize = get<2>(GetParam()); >+ ssize_t nsize = get<3>(GetParam()); >+ >+ ASSERT_LE(osize, bufsize); >+ ASSERT_LE(nsize, bufsize); >+ if (cache_attrs) >+ attr_valid = UINT64_MAX; >+ else >+ attr_valid = 0; >+ if (cache_lookups) >+ entry_valid = UINT64_MAX; >+ else >+ entry_valid = 0; >+ >+ expect_lookup(RELPATH, ino, osize, entry_valid, attr_valid); >+ expect_open(ino, 1); >+ if (!cache_attrs) >+ expect_getattr(ino, 2, osize, attr_valid); >+ expect_read(ino, 0, osize, osize, CONTENTS); >+ >+ fd = open(FULLPATH, O_RDONLY); >+ ASSERT_LE(0, fd) << strerror(errno); >+ >+ ASSERT_EQ(osize, read(fd, buf, bufsize)) << strerror(errno); >+ ASSERT_EQ(0, memcmp(buf, CONTENTS, osize)); >+ >+ // Now truncate the file behind the kernel's back. The next read >+ // should discard cache and fetch from disk again. >+ if (reopen) { >+ // Close and reopen the file >+ expect_flush(ino, 1, ReturnErrno(ENOSYS)); >+ expect_release(ino, ReturnErrno(0)); >+ ASSERT_EQ(0, close(fd)); >+ expect_lookup(RELPATH, ino, nsize, entry_valid, attr_valid); >+ expect_open(ino, 1); >+ fd = open(FULLPATH, O_RDONLY); >+ ASSERT_LE(0, fd) << strerror(errno); >+ } >+ >+ if (!cache_attrs) >+ expect_getattr(ino, 1, nsize, attr_valid); >+ expect_read(ino, 0, nsize, nsize, CONTENTS); >+ ASSERT_EQ(0, lseek(fd, 0, SEEK_SET)); >+ ASSERT_EQ(nsize, read(fd, buf, bufsize)) << strerror(errno); >+ ASSERT_EQ(0, memcmp(buf, CONTENTS, nsize)); >+ >+ leak(fd); >+} >+ >+INSTANTIATE_TEST_CASE_P(Cache, Cache, >+ Combine( >+ /* Test every combination that: >+ * - does not cache at least one of entries and attrs >+ * - either doesn't cache attrs, or reopens the file >+ * In the other combinations, the kernel will never learn that >+ * the file's size has changed. >+ */ >+ Values( >+ std::make_tuple(false, false, false), >+ std::make_tuple(false, true, false), >+ std::make_tuple(true, false, false), >+ std::make_tuple(true, false, true), >+ std::make_tuple(true, true, false) >+ ), >+ Values(Writethrough, Writeback), >+ /* Test both reductions and extensions to file size */ >+ Values(20), >+ Values(10, 25) >+ ) >+); > >Property changes on: tests/sys/fs/fusefs/cache.cc >___________________________________________________________________ >Added: svn:eol-style >## -0,0 +1 ## >+native >\ No newline at end of property >Added: svn:keywords >## -0,0 +1 ## >+FreeBSD=%H >\ No newline at end of property >Added: svn:mime-type >## -0,0 +1 ## >+text/plain >\ No newline at end of property >Index: tests/sys/fs/fusefs/getattr.cc >=================================================================== >--- tests/sys/fs/fusefs/getattr.cc (revision 358797) >+++ tests/sys/fs/fusefs/getattr.cc (working copy) >@@ -159,6 +159,7 @@ > out.body.attr.attr.mode = S_IFREG | 0644; > out.body.attr.attr.ino = ino; // Must match nodeid > out.body.attr.attr.blksize = 0; >+ out.body.attr.attr.size = 1; > }))); > > ASSERT_EQ(0, stat(FULLPATH, &sb)) << strerror(errno); >Index: tests/sys/fs/fusefs/io.cc >=================================================================== >--- tests/sys/fs/fusefs/io.cc (revision 358797) >+++ tests/sys/fs/fusefs/io.cc (working copy) >@@ -50,28 +50,6 @@ > > using namespace testing; > >-enum cache_mode { >- Uncached, >- Writethrough, >- Writeback, >- WritebackAsync >-}; >- >-const char *cache_mode_to_s(enum cache_mode cm) { >- switch (cm) { >- case Uncached: >- return "Uncached"; >- case Writethrough: >- return "Writethrough"; >- case Writeback: >- return "Writeback"; >- case WritebackAsync: >- return "WritebackAsync"; >- default: >- return "Unknown"; >- } >-} >- > const char FULLPATH[] = "mountpoint/some_file.txt"; > const char RELPATH[] = "some_file.txt"; > const uint64_t ino = 42; >Index: tests/sys/fs/fusefs/utils.cc >=================================================================== >--- tests/sys/fs/fusefs/utils.cc (revision 358797) >+++ tests/sys/fs/fusefs/utils.cc (working copy) >@@ -90,6 +90,21 @@ > GTEST_SKIP() << "current user is not allowed to mount"; > } > >+const char *cache_mode_to_s(enum cache_mode cm) { >+ switch (cm) { >+ case Uncached: >+ return "Uncached"; >+ case Writethrough: >+ return "Writethrough"; >+ case Writeback: >+ return "Writeback"; >+ case WritebackAsync: >+ return "WritebackAsync"; >+ default: >+ return "Unknown"; >+ } >+} >+ > bool is_unsafe_aio_enabled(void) { > const char *node = "vfs.aio.enable_unsafe"; > int val = 0; >Index: tests/sys/fs/fusefs/utils.hh >=================================================================== >--- tests/sys/fs/fusefs/utils.hh (revision 358797) >+++ tests/sys/fs/fusefs/utils.hh (working copy) >@@ -44,6 +44,14 @@ > usleep(NAP_NS / 1000); > } > >+enum cache_mode { >+ Uncached, >+ Writethrough, >+ Writeback, >+ WritebackAsync >+}; >+ >+const char *cache_mode_to_s(enum cache_mode cm); > bool is_unsafe_aio_enabled(void); > > extern const uint32_t libfuse_max_write;
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 244178
: 212269