From 44f10c6e4023c958a267009deaa96a5e8a27fd00 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Tue, 9 Apr 2019 21:22:02 +0000 Subject: [PATCH] fusefs: cache negative lookups The FUSE protocol includes a way for a server to tell the client that a negative lookup response is cacheable for a certain amount of time. PR: 236226 Sponsored by: The FreeBSD Foundation --- sys/fs/fuse/fuse_internal.c | 14 +------ sys/fs/fuse/fuse_internal.h | 46 +++++++++++++++++++++ sys/fs/fuse/fuse_node.c | 12 +----- sys/fs/fuse/fuse_vnops.c | 75 +++++++---------------------------- tests/sys/fs/fusefs/lookup.cc | 3 +- 5 files changed, 66 insertions(+), 84 deletions(-) diff --git a/sys/fs/fuse/fuse_internal.c b/sys/fs/fuse/fuse_internal.c index 34595a329151..98413d8a50ec 100644 --- a/sys/fs/fuse/fuse_internal.c +++ b/sys/fs/fuse/fuse_internal.c @@ -189,22 +189,12 @@ fuse_internal_cache_attrs(struct vnode *vp, struct fuse_attr *attr, struct mount *mp; struct fuse_vnode_data *fvdat; struct vattr *vp_cache_at; - struct timespec now, duration, timeout; mp = vnode_mount(vp); fvdat = VTOFUD(vp); - getnanouptime(&now); - /* "+ 2" is the bound of attr_valid_nsec + now.tv_nsec */ - /* Why oh why isn't there a TIME_MAX defined? */ - if (attr_valid >= INT_MAX || attr_valid + now.tv_sec + 2 >= INT_MAX) { - fvdat->attr_cache_timeout.sec = INT_MAX; - } else { - duration.tv_sec = attr_valid; - duration.tv_nsec = attr_valid_nsec; - timespecadd(&duration, &now, &timeout); - timespec2bintime(&timeout, &fvdat->attr_cache_timeout); - } + fuse_validity_2_bintime(attr_valid, attr_valid_nsec, + &fvdat->attr_cache_timeout); vp_cache_at = VTOVA(vp); diff --git a/sys/fs/fuse/fuse_internal.h b/sys/fs/fuse/fuse_internal.h index 49064994831b..e878901ef967 100644 --- a/sys/fs/fuse/fuse_internal.h +++ b/sys/fs/fuse/fuse_internal.h @@ -150,6 +150,52 @@ fuse_iosize(struct vnode *vp) return (vp->v_mount->mnt_stat.f_iosize); } +/* + * Make a cacheable timeout in bintime format value based on a fuse_attr_out + * response + */ +static inline void +fuse_validity_2_bintime(uint64_t attr_valid, uint32_t attr_valid_nsec, + struct bintime *timeout) +{ + struct timespec now, duration, timeout_ts; + + getnanouptime(&now); + /* "+ 2" is the bound of attr_valid_nsec + now.tv_nsec */ + /* Why oh why isn't there a TIME_MAX defined? */ + if (attr_valid >= INT_MAX || attr_valid + now.tv_sec + 2 >= INT_MAX) { + timeout->sec = INT_MAX; + } else { + duration.tv_sec = attr_valid; + duration.tv_nsec = attr_valid_nsec; + timespecadd(&duration, &now, &timeout_ts); + timespec2bintime(&timeout_ts, timeout); + } +} + +/* + * Make a cacheable timeout value in timespec format based on the fuse_entry_out + * response + */ +static inline void +fuse_validity_2_timespec(const struct fuse_entry_out *feo, + struct timespec *timeout) +{ + struct timespec duration, now; + + getnanouptime(&now); + /* "+ 2" is the bound of entry_valid_nsec + now.tv_nsec */ + if (feo->entry_valid >= INT_MAX || + feo->entry_valid + now.tv_sec + 2 >= INT_MAX) { + timeout->tv_sec = INT_MAX; + } else { + duration.tv_sec = feo->entry_valid; + duration.tv_nsec = feo->entry_valid_nsec; + timespecadd(&duration, &now, timeout); + } +} + + /* access */ #define FVP_ACCESS_NOOP 0x01 diff --git a/sys/fs/fuse/fuse_node.c b/sys/fs/fuse/fuse_node.c index 520cb41eea1a..93b8b91ebd0f 100644 --- a/sys/fs/fuse/fuse_node.c +++ b/sys/fs/fuse/fuse_node.c @@ -291,20 +291,12 @@ fuse_vnode_get(struct mount *mp, if (dvp != NULL && cnp != NULL && (cnp->cn_flags & MAKEENTRY) != 0 && feo != NULL && (feo->entry_valid != 0 || feo->entry_valid_nsec != 0)) { - struct timespec duration, now, timeout; + struct timespec timeout; ASSERT_VOP_LOCKED(*vpp, "fuse_vnode_get"); ASSERT_VOP_LOCKED(dvp, "fuse_vnode_get"); - getnanouptime(&now); - if (feo->entry_valid >= INT_MAX || - feo->entry_valid + now.tv_sec + 2 >= INT_MAX) { - timeout.tv_sec = INT_MAX; - } else { - duration.tv_sec = feo->entry_valid; - duration.tv_nsec = feo->entry_valid_nsec; - timespecadd(&duration, &now, &timeout); - } + fuse_validity_2_timespec(feo, &timeout); cache_enter_time(dvp, *vpp, cnp, &timeout, NULL); } diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c index 1cdfcb712817..3efa2f4a6cf4 100644 --- a/sys/fs/fuse/fuse_vnops.c +++ b/sys/fs/fuse/fuse_vnops.c @@ -706,6 +706,9 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) struct fuse_dispatcher fdi; enum fuse_opcode op; + struct fuse_entry_out *feo = NULL; + struct fuse_attr_out *fao = NULL; + struct fuse_attr *fattr = NULL; uint64_t nid; struct fuse_access_param facp; @@ -802,15 +805,18 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) if ((op == FUSE_LOOKUP) && !lookup_err) { /* lookup call succeeded */ nid = ((struct fuse_entry_out *)fdi.answ)->nodeid; - if (!nid) { - /* - * zero nodeid is the same as "not found", - * but it's also cacheable (which we keep - * keep on doing not as of writing this) - * See PR 236226 - */ + if (nid == 0) { + /* zero nodeid means ENOENT and cache it */ + struct timespec timeout; + fdi.answ_stat = ENOENT; lookup_err = ENOENT; + if (cnp->cn_flags & MAKEENTRY) { + feo = (struct fuse_entry_out *)fdi.answ; + fuse_validity_2_timespec(feo, &timeout); + cache_enter_time(dvp, *vpp, cnp, &timeout, + NULL); + } } else if (nid == FUSE_ROOT_ID) { lookup_err = EINVAL; } @@ -849,24 +855,7 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) err = EJUSTRETURN; goto out; } - /* Consider inserting name into cache. */ - /* - * No we can't use negative caching, as the fs - * changes are out of our control. - * False positives' falseness turns out just as things - * go by, but false negatives' falseness doesn't. - * (and aiding the caching mechanism with extra control - * mechanisms comes quite close to beating the whole purpose - * caching...) - */ -#if 0 - if ((cnp->cn_flags & MAKEENTRY) != 0) { - SDT_PROBE2(fuse, , vnops, trace, 1, - "inserting NULL into cache"); - cache_enter(dvp, NULL, cnp); - } -#endif err = ENOENT; goto out; @@ -874,9 +863,6 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) /* !lookup_err */ - struct fuse_entry_out *feo = NULL; - struct fuse_attr *fattr = NULL; - if (op == FUSE_GETATTR) { fattr = &((struct fuse_attr_out *)fdi.answ)->attr; } else { @@ -1040,47 +1026,16 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) } if (op == FUSE_GETATTR) { - struct fuse_attr_out *fao = - (struct fuse_attr_out*)fdi.answ; + fao = (struct fuse_attr_out*)fdi.answ; fuse_internal_cache_attrs(*vpp, &fao->attr, fao->attr_valid, fao->attr_valid_nsec, NULL); } else { - struct fuse_entry_out *feo = - (struct fuse_entry_out*)fdi.answ; + feo = (struct fuse_entry_out*)fdi.answ; fuse_internal_cache_attrs(*vpp, &feo->attr, feo->attr_valid, feo->attr_valid_nsec, NULL); } - - /* Insert name into cache if appropriate. */ - - /* - * Nooo, caching is evil. With caching, we can't avoid stale - * information taking over the playground (cached info is not - * just positive/negative, it does have qualitative aspects, - * too). And a (VOP/FUSE)_GETATTR is always thrown anyway, when - * walking down along cached path components, and that's not - * any cheaper than FUSE_LOOKUP. This might change with - * implementing kernel side attr caching, but... In Linux, - * lookup results are not cached, and the daemon is bombarded - * with FUSE_LOOKUPS on and on. This shows that by design, the - * daemon is expected to handle frequent lookup queries - * efficiently, do its caching in userspace, and so on. - * - * So just leave the name cache alone. - */ - - /* - * Well, now I know, Linux caches lookups, but with a - * timeout... So it's the same thing as attribute caching: - * we can deal with it when implement timeouts. - */ -#if 0 - if (cnp->cn_flags & MAKEENTRY) { - cache_enter(dvp, *vpp, cnp); - } -#endif } out: if (!lookup_err) { diff --git a/tests/sys/fs/fusefs/lookup.cc b/tests/sys/fs/fusefs/lookup.cc index 59c95da1b536..988f734673d5 100644 --- a/tests/sys/fs/fusefs/lookup.cc +++ b/tests/sys/fs/fusefs/lookup.cc @@ -169,8 +169,7 @@ TEST_F(Lookup, entry_cache) * If the daemon returns an error of 0 and an inode of 0, that's a flag for * "ENOENT and cache it" with the given entry_timeout */ -/* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236226 */ -TEST_F(Lookup, DISABLED_entry_cache_negative) +TEST_F(Lookup, entry_cache_negative) { struct timespec entry_valid = {.tv_sec = TIME_T_MAX, .tv_nsec = 0};