Page MenuHomeFreeBSD
Files F108199499

D24012.diff
No OneTemporary
Actions

D24012.diff
View Options

Index: head/sys/fs/fuse/fuse_internal.c
===================================================================
--- head/sys/fs/fuse/fuse_internal.c
+++ head/sys/fs/fuse/fuse_internal.c
@@ -857,6 +857,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,24 @@
if (fvdat->flag & FN_MTIMECHANGE) {
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! "
+ "Buggy FUSE server detected. To prevent data corruption, "
+ "disable the data cache by mounting with -o direct_io, or "
+ "as directed otherwise by your FUSE server's "
+ "documentation\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);
Index: head/sys/fs/fuse/fuse_node.h
===================================================================
--- head/sys/fs/fuse/fuse_node.h
+++ head/sys/fs/fuse/fuse_node.h
@@ -134,13 +134,19 @@
#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: head/sys/fs/fuse/fuse_node.c
===================================================================
--- head/sys/fs/fuse/fuse_node.c
+++ head/sys/fs/fuse/fuse_node.c
@@ -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: head/sys/fs/fuse/fuse_vnops.c
===================================================================
--- head/sys/fs/fuse/fuse_vnops.c
+++ head/sys/fs/fuse/fuse_vnops.c
@@ -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,27 @@
*/
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",
- __func__,
+ printf("%s: cache incoherent on %s! "
+ "Buggy FUSE server detected. To prevent "
+ "data corruption, disable the data cache "
+ "by mounting with -o direct_io, or as "
+ "directed otherwise by your FUSE server's "
+ "documentation\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: head/tests/sys/fs/fusefs/Makefile
===================================================================
--- head/tests/sys/fs/fusefs/Makefile
+++ head/tests/sys/fs/fusefs/Makefile
@@ -12,6 +12,7 @@
GTESTS+= access
GTESTS+= allow_other
GTESTS+= bmap
+GTESTS+= cache
GTESTS+= create
GTESTS+= default_permissions
GTESTS+= default_permissions_privileged
Index: head/tests/sys/fs/fusefs/cache.cc
===================================================================
--- head/tests/sys/fs/fusefs/cache.cc
+++ head/tests/sys/fs/fusefs/cache.cc
@@ -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)
+ )
+);
Index: head/tests/sys/fs/fusefs/getattr.cc
===================================================================
--- head/tests/sys/fs/fusefs/getattr.cc
+++ head/tests/sys/fs/fusefs/getattr.cc
@@ -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: head/tests/sys/fs/fusefs/io.cc
===================================================================
--- head/tests/sys/fs/fusefs/io.cc
+++ head/tests/sys/fs/fusefs/io.cc
@@ -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: head/tests/sys/fs/fusefs/utils.hh
===================================================================
--- head/tests/sys/fs/fusefs/utils.hh
+++ head/tests/sys/fs/fusefs/utils.hh
@@ -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;
Index: head/tests/sys/fs/fusefs/utils.cc
===================================================================
--- head/tests/sys/fs/fusefs/utils.cc
+++ head/tests/sys/fs/fusefs/utils.cc
@@ -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;

File Metadata

Mime Type
text/plain
Expires
Thu, Jan 23, 1:36 PM (21 h, 40 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
16055145
Default Alt Text
D24012.diff (14 KB)

Event Timeline