fusefs: update atime on reads when using cached attributes

When using cached attributes, whether or not the data cache is enabled,
fusefs must update a file's atime whenever it reads from it, so long as
it wasn't mounted with -o noatime.  Update it in-kernel, and flush it to
the server on close or during the next setattr operation.

The downside is that close() will now frequently trigger a FUSE_SETATTR
upcall.  But if you care about performance, you should be using
-o noatime anyway.

MFC after:	2 weeks
Reviewed by:	pfg
Differential Revision: https://reviews.freebsd.org/D33145

sys/fs/fuse/fuse_internal.c

@@ -925,6 +925,7 @@ fuse_internal_do_getattr(struct vnode *vp, struct vattr *vap,
 	struct fuse_getattr_in *fgai;
 	struct fuse_attr_out *fao;
 	off_t old_filesize = fvdat->cached_attrs.va_size;
+	struct timespec old_atime = fvdat->cached_attrs.va_atime;
 	struct timespec old_ctime = fvdat->cached_attrs.va_ctime;
 	struct timespec old_mtime = fvdat->cached_attrs.va_mtime;
 	enum vtype vtyp;
@@ -949,6 +950,10 @@ fuse_internal_do_getattr(struct vnode *vp, struct vattr *vap,
 	vtyp = IFTOVT(fao->attr.mode);
 	if (fvdat->flag & FN_SIZECHANGE)
 		fao->attr.size = old_filesize;
+	if (fvdat->flag & FN_ATIMECHANGE) {
+		fao->attr.atime = old_atime.tv_sec;
+		fao->attr.atimensec = old_atime.tv_nsec;
+	}
 	if (fvdat->flag & FN_CTIMECHANGE) {
 		fao->attr.ctime = old_ctime.tv_sec;
 		fao->attr.ctimensec = old_ctime.tv_nsec;
@@ -1208,6 +1213,10 @@ int fuse_internal_setattr(struct vnode *vp, struct vattr *vap,
 		fsai->valid |= FATTR_ATIME;
 		if (vap->va_vaflags & VA_UTIMES_NULL)
 			fsai->valid |= FATTR_ATIME_NOW;
+	} else if (fvdat->flag & FN_ATIMECHANGE) {
+		fsai->atime = fvdat->cached_attrs.va_atime.tv_sec;
+		fsai->atimensec = fvdat->cached_attrs.va_atime.tv_nsec;
+		fsai->valid |= FATTR_ATIME;
 	}
 	if (vap->va_mtime.tv_sec != VNOVAL) {
 		fsai->mtime = vap->va_mtime.tv_sec;
@@ -1256,7 +1265,7 @@ int fuse_internal_setattr(struct vnode *vp, struct vattr *vap,
 	}
 	if (err == 0) {
 		struct fuse_attr_out *fao = (struct fuse_attr_out*)fdi.answ;
-		fuse_vnode_undirty_cached_timestamps(vp);
+		fuse_vnode_undirty_cached_timestamps(vp, true);
 		fuse_internal_cache_attrs(vp, &fao->attr, fao->attr_valid,
 			fao->attr_valid_nsec, NULL, false);
 	}

sys/fs/fuse/fuse_io.c

@@ -236,6 +236,7 @@ fuse_io_dispatch(struct vnode *vp, struct uio *uio, int ioflag,
 
 	switch (uio->uio_rw) {
 	case UIO_READ:
+		fuse_vnode_update(vp, FN_ATIMECHANGE);
 		if (directio) {
 			SDT_PROBE2(fusefs, , io, trace, 1,
 				"direct read of vnode");
@@ -616,7 +617,7 @@ fuse_write_directbackend(struct vnode *vp, struct uio *uio,
 	fdisp_destroy(&fdi);
 
 	if (wrote_anything)
-		fuse_vnode_undirty_cached_timestamps(vp);
+		fuse_vnode_undirty_cached_timestamps(vp, false);
 
 	return (err);
 }

sys/fs/fuse/fuse_node.c

@@ -475,11 +475,13 @@ fuse_vnode_size(struct vnode *vp, off_t *filesize, struct ucred *cred,
 }
 
 void
-fuse_vnode_undirty_cached_timestamps(struct vnode *vp)
+fuse_vnode_undirty_cached_timestamps(struct vnode *vp, bool atime)
 {
 	struct fuse_vnode_data *fvdat = VTOFUD(vp);
 
 	fvdat->flag &= ~(FN_MTIMECHANGE | FN_CTIMECHANGE);
+	if (atime)
+		fvdat->flag &= ~FN_ATIMECHANGE;
 }
 
 /* Update a fuse file's cached timestamps */
@@ -487,7 +489,8 @@ void
 fuse_vnode_update(struct vnode *vp, int flags)
 {
 	struct fuse_vnode_data *fvdat = VTOFUD(vp);
-	struct fuse_data *data = fuse_get_mpdata(vnode_mount(vp));
+	struct mount *mp = vnode_mount(vp);
+	struct fuse_data *data = fuse_get_mpdata(mp);
 	struct timespec ts;
 
 	vfs_timestamp(&ts);
@@ -495,6 +498,11 @@ fuse_vnode_update(struct vnode *vp, int flags)
 	if (data->time_gran > 1)
 		ts.tv_nsec = rounddown(ts.tv_nsec, data->time_gran);
 
+	if (mp->mnt_flag & MNT_NOATIME)
+		flags &= ~FN_ATIMECHANGE;
+
+	if (flags & FN_ATIMECHANGE)
+		fvdat->cached_attrs.va_atime = ts;
 	if (flags & FN_MTIMECHANGE)
 		fvdat->cached_attrs.va_mtime = ts;
 	if (flags & FN_CTIMECHANGE)

sys/fs/fuse/fuse_node.h

@@ -92,6 +92,7 @@
  */
 #define	FN_MTIMECHANGE		0x00000800
 #define	FN_CTIMECHANGE		0x00001000
+#define	FN_ATIMECHANGE		0x00002000
 
 struct fuse_vnode_data {
 	/** self **/
@@ -203,7 +204,7 @@ int fuse_vnode_savesize(struct vnode *vp, struct ucred *cred, pid_t pid);
 
 int fuse_vnode_setsize(struct vnode *vp, off_t newsize, bool from_server);
 
-void fuse_vnode_undirty_cached_timestamps(struct vnode *vp);
+void fuse_vnode_undirty_cached_timestamps(struct vnode *vp, bool atime);
 
 void fuse_vnode_update(struct vnode *vp, int flags);
 

sys/fs/fuse/fuse_vnops.c

@@ -613,6 +613,7 @@ fuse_vnop_close(struct vop_close_args *ap)
 	int fflag = ap->a_fflag;
 	struct thread *td = ap->a_td;
 	pid_t pid = td->td_proc->p_pid;
+	struct fuse_vnode_data *fvdat = VTOFUD(vp);
 	int err = 0;
 
 	if (fuse_isdeadfs(vp))
@@ -623,8 +624,15 @@ fuse_vnop_close(struct vop_close_args *ap)
 		return 0;
 
 	err = fuse_flush(vp, cred, pid, fflag);
+	if (err == 0 && (fvdat->flag & FN_ATIMECHANGE)) {
+		struct vattr vap;
+
+		VATTR_NULL(&vap);
+		vap.va_atime = fvdat->cached_attrs.va_atime;
+		err = fuse_internal_setattr(vp, &vap, td, NULL);
+	}
 	/* TODO: close the file handle, if we're sure it's no longer used */
-	if ((VTOFUD(vp)->flag & FN_SIZECHANGE) != 0) {
+	if ((fvdat->flag & FN_SIZECHANGE) != 0) {
 		fuse_vnode_savesize(vp, cred, td->td_proc->p_pid);
 	}
 	return err;

tests/sys/fs/fusefs/cache.cc

@@ -75,6 +75,7 @@ virtual void SetUp() {
 		default:
 			FAIL() << "Unknown cache mode";
 	}
+	m_noatime = true;	// To prevent SETATTR for atime on close
 
 	FuseTest::SetUp();
 	if (IsSkipped())

tests/sys/fs/fusefs/io.cc

@@ -114,6 +114,7 @@ void SetUp()
 		default:
 			FAIL() << "Unknown cache mode";
 	}
+	m_noatime = true;	// To prevent SETATTR for atime on close
 
 	FuseTest::SetUp();
 	if (IsSkipped())

tests/sys/fs/fusefs/mockfs.cc

@@ -392,7 +392,7 @@ void MockFS::debug_response(const mockfs_buf_out &out) {
 MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions,
 	bool push_symlinks_in, bool ro, enum poll_method pm, uint32_t flags,
 	uint32_t kernel_minor_version, uint32_t max_write, bool async,
-	bool noclusterr, unsigned time_gran, bool nointr)
+	bool noclusterr, unsigned time_gran, bool nointr, bool noatime)
 {
 	struct sigaction sa;
 	struct iovec *iov = NULL;
@@ -467,6 +467,10 @@ MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions,
 		build_iovec(&iov, &iovlen, "async", __DECONST(void*, &trueval),
 			sizeof(bool));
 	}
+	if (noatime) {
+		build_iovec(&iov, &iovlen, "noatime",
+			__DECONST(void*, &trueval), sizeof(bool));
+	}
 	if (noclusterr) {
 		build_iovec(&iov, &iovlen, "noclusterr",
 			__DECONST(void*, &trueval), sizeof(bool));

tests/sys/fs/fusefs/mockfs.hh

@@ -356,7 +356,8 @@ class MockFS {
 		bool default_permissions, bool push_symlinks_in, bool ro,
 		enum poll_method pm, uint32_t flags,
 		uint32_t kernel_minor_version, uint32_t max_write, bool async,
-		bool no_clusterr, unsigned time_gran, bool nointr);
+		bool no_clusterr, unsigned time_gran, bool nointr,
+		bool noatime);
 
 	virtual ~MockFS();
 

tests/sys/fs/fusefs/read.cc

@@ -105,6 +105,13 @@ class ReadAhead: public Read,
 	}
 };
 
+class ReadNoatime: public Read {
+	virtual void SetUp() {
+		m_noatime = true;
+		Read::SetUp();
+	}
+};
+
 class ReadSigbus: public Read
 {
 public:
@@ -132,6 +139,14 @@ handle_sigbus(int signo __unused, siginfo_t *info, void *uap __unused) {
 jmp_buf ReadSigbus::s_jmpbuf;
 void *ReadSigbus::s_si_addr;
 
+class TimeGran: public Read, public WithParamInterface<unsigned> {
+public:
+virtual void SetUp() {
+	m_time_gran = 1 << GetParam();
+	Read::SetUp();
+}
+};
+
 /* AIO reads need to set the header's pid field correctly */
 /* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236379 */
 TEST_F(AioRead, aio_read)
@@ -323,6 +338,172 @@ TEST_F(AsyncRead, async_read)
 	leak(fd);
 }
 
+/* The kernel should update the cached atime attribute during a read */
+TEST_F(Read, atime)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	struct stat sb1, sb2;
+	uint64_t ino = 42;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+
+	fd = open(FULLPATH, O_RDONLY);
+	ASSERT_LE(0, fd) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb1));
+
+	/* Ensure atime will be different than it was during lookup */
+	nap();
+
+	ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb2));
+
+	/* The kernel should automatically update atime during read */
+	EXPECT_TRUE(timespeccmp(&sb1.st_atim, &sb2.st_atim, <));
+	EXPECT_TRUE(timespeccmp(&sb1.st_ctim, &sb2.st_ctim, ==));
+	EXPECT_TRUE(timespeccmp(&sb1.st_mtim, &sb2.st_mtim, ==));
+
+	leak(fd);
+}
+
+/* The kernel should update the cached atime attribute during a cached read */
+TEST_F(Read, atime_cached)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	struct stat sb1, sb2;
+	uint64_t ino = 42;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+
+	fd = open(FULLPATH, O_RDONLY);
+	ASSERT_LE(0, fd) << strerror(errno);
+
+	ASSERT_EQ(bufsize, pread(fd, buf, bufsize, 0)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb1));
+
+	/* Ensure atime will be different than it was during the first read */
+	nap();
+
+	ASSERT_EQ(bufsize, pread(fd, buf, bufsize, 0)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb2));
+
+	/* The kernel should automatically update atime during read */
+	EXPECT_TRUE(timespeccmp(&sb1.st_atim, &sb2.st_atim, <));
+	EXPECT_TRUE(timespeccmp(&sb1.st_ctim, &sb2.st_ctim, ==));
+	EXPECT_TRUE(timespeccmp(&sb1.st_mtim, &sb2.st_mtim, ==));
+
+	leak(fd);
+}
+
+/* dirty atime values should be flushed during close */
+TEST_F(Read, atime_during_close)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	struct stat sb;
+	uint64_t ino = 42;
+	const mode_t newmode = 0755;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([&](auto in) {
+			uint32_t valid = FATTR_ATIME;
+			return (in.header.opcode == FUSE_SETATTR &&
+				in.header.nodeid == ino &&
+				in.body.setattr.valid == valid &&
+				(time_t)in.body.setattr.atime ==
+					sb.st_atim.tv_sec &&
+				in.body.setattr.atimensec ==
+					sb.st_atim.tv_nsec);
+		}, Eq(true)),
+		_)
+	).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, attr);
+		out.body.attr.attr.ino = ino;
+		out.body.attr.attr.mode = S_IFREG | newmode;
+	})));
+	expect_flush(ino, 1, ReturnErrno(0));
+	expect_release(ino, FuseTest::FH);
+
+	fd = open(FULLPATH, O_RDONLY);
+	ASSERT_LE(0, fd) << strerror(errno);
+
+	/* Ensure atime will be different than during lookup */
+	nap();
+
+	ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb));
+
+	close(fd);
+}
+
+/* A cached atime should be flushed during FUSE_SETATTR */
+TEST_F(Read, atime_during_setattr)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	struct stat sb;
+	uint64_t ino = 42;
+	const mode_t newmode = 0755;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([&](auto in) {
+			uint32_t valid = FATTR_MODE | FATTR_ATIME;
+			return (in.header.opcode == FUSE_SETATTR &&
+				in.header.nodeid == ino &&
+				in.body.setattr.valid == valid &&
+				(time_t)in.body.setattr.atime ==
+					sb.st_atim.tv_sec &&
+				in.body.setattr.atimensec ==
+					sb.st_atim.tv_nsec);
+		}, Eq(true)),
+		_)
+	).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, attr);
+		out.body.attr.attr.ino = ino;
+		out.body.attr.attr.mode = S_IFREG | newmode;
+	})));
+
+	fd = open(FULLPATH, O_RDONLY);
+	ASSERT_LE(0, fd) << strerror(errno);
+
+	/* Ensure atime will be different than during lookup */
+	nap();
+
+	ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb));
+	ASSERT_EQ(0, fchmod(fd, newmode)) << strerror(errno);
+
+	leak(fd);
+}
+
+/* The kernel should flush dirty atime values during close */
 /* 0-length reads shouldn't cause any confusion */
 TEST_F(Read, direct_io_read_nothing)
 {
@@ -613,6 +794,80 @@ TEST_F(Read, mmap)
 	leak(fd);
 }
 
+/*
+ * The kernel should not update the cached atime attribute during a read, if
+ * MNT_NOATIME is used.
+ */
+TEST_F(ReadNoatime, atime)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	struct stat sb1, sb2;
+	uint64_t ino = 42;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+
+	fd = open(FULLPATH, O_RDONLY);
+	ASSERT_LE(0, fd) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb1));
+
+	nap();
+
+	ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb2));
+
+	/* The kernel should not update atime during read */
+	EXPECT_TRUE(timespeccmp(&sb1.st_atim, &sb2.st_atim, ==));
+	EXPECT_TRUE(timespeccmp(&sb1.st_ctim, &sb2.st_ctim, ==));
+	EXPECT_TRUE(timespeccmp(&sb1.st_mtim, &sb2.st_mtim, ==));
+
+	leak(fd);
+}
+
+/*
+ * The kernel should not update the cached atime attribute during a cached
+ * read, if MNT_NOATIME is used.
+ */
+TEST_F(ReadNoatime, atime_cached)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	struct stat sb1, sb2;
+	uint64_t ino = 42;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+
+	fd = open(FULLPATH, O_RDONLY);
+	ASSERT_LE(0, fd) << strerror(errno);
+
+	ASSERT_EQ(bufsize, pread(fd, buf, bufsize, 0)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb1));
+
+	nap();
+
+	ASSERT_EQ(bufsize, pread(fd, buf, bufsize, 0)) << strerror(errno);
+	ASSERT_EQ(0, fstat(fd, &sb2));
+
+	/* The kernel should automatically update atime during read */
+	EXPECT_TRUE(timespeccmp(&sb1.st_atim, &sb2.st_atim, ==));
+	EXPECT_TRUE(timespeccmp(&sb1.st_ctim, &sb2.st_ctim, ==));
+	EXPECT_TRUE(timespeccmp(&sb1.st_mtim, &sb2.st_mtim, ==));
+
+	leak(fd);
+}
+
 /* Read of an mmap()ed file fails */
 TEST_F(ReadSigbus, mmap_eio)
 {
@@ -1068,3 +1323,44 @@ INSTANTIATE_TEST_CASE_P(RA, ReadAhead,
 	       tuple<bool, int>(true, 0),
 	       tuple<bool, int>(true, 1),
 	       tuple<bool, int>(true, 2)));
+
+/* fuse_init_out.time_gran controls the granularity of timestamps */
+TEST_P(TimeGran, atime_during_setattr)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	ssize_t bufsize = strlen(CONTENTS);
+	uint8_t buf[bufsize];
+	uint64_t ino = 42;
+	const mode_t newmode = 0755;
+	int fd;
+
+	expect_lookup(RELPATH, ino, bufsize);
+	expect_open(ino, 0, 1);
+	expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([=](auto in) {
+			uint32_t valid = FATTR_MODE | FATTR_ATIME;
+			return (in.header.opcode == FUSE_SETATTR &&
+				in.header.nodeid == ino &&
+				in.body.setattr.valid == valid &&
+				in.body.setattr.atimensec % m_time_gran == 0);
+		}, Eq(true)),
+		_)
+	).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, attr);
+		out.body.attr.attr.ino = ino;
+		out.body.attr.attr.mode = S_IFREG | newmode;
+	})));
+
+	fd = open(FULLPATH, O_RDWR);
+	ASSERT_LE(0, fd) << strerror(errno);
+
+	ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+	ASSERT_EQ(0, fchmod(fd, newmode)) << strerror(errno);
+
+	leak(fd);
+}
+
+INSTANTIATE_TEST_CASE_P(TG, TimeGran, Range(0u, 10u));

tests/sys/fs/fusefs/utils.cc

@@ -161,7 +161,7 @@ void FuseTest::SetUp() {
 			m_default_permissions, m_push_symlinks_in, m_ro,
 			m_pm, m_init_flags, m_kernel_minor_version,
 			m_maxwrite, m_async, m_noclusterr, m_time_gran,
-			m_nointr);
+			m_nointr, m_noatime);
 		/* 
 		 * FUSE_ACCESS is called almost universally.  Expecting it in
 		 * each test case would be super-annoying.  Instead, set a

tests/sys/fs/fusefs/utils.hh

@@ -64,6 +64,7 @@ class FuseTest : public ::testing::Test {
 	bool m_default_permissions;
 	uint32_t m_kernel_minor_version;
 	enum poll_method m_pm;
+	bool m_noatime;
 	bool m_push_symlinks_in;
 	bool m_ro;
 	bool m_async;
@@ -85,6 +86,7 @@ class FuseTest : public ::testing::Test {
 		m_default_permissions(false),
 		m_kernel_minor_version(FUSE_KERNEL_MINOR_VERSION),
 		m_pm(BLOCKING),
+		m_noatime(false),
 		m_push_symlinks_in(false),
 		m_ro(false),
 		m_async(false),