[PATCH 0/2] fuse: add no forget support

[PATCH 0/2] fuse: add no forget support

[yangyun]

FUSE_FORGET requests are not used in some cases but have an impact on the
system. So add no forget support.

Patch 1 simplifies the queueing process of FUSE_FORGET request when error happens, 
which Patch 2 depends on.

Patch 2 does the actual work about the no forget support.  

yangyun (2):
  fuse: replace fuse_queue_forget with fuse_force_forget if error
  fuse: add support for no forget requests

 fs/fuse/dev.c             | 25 ++++++++++++++++
 fs/fuse/dir.c             | 63 +++++++++------------------------------
 fs/fuse/fuse_i.h          | 26 ++++++++++++++++
 fs/fuse/inode.c           | 10 +++----
 fs/fuse/readdir.c         | 37 +++++------------------
 include/uapi/linux/fuse.h |  3 ++
 6 files changed, 81 insertions(+), 83 deletions(-)

-- 
2.33.0

[PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force_forget if error

[yangyun]

Most usecases for 'fuse_queue_forget' in the code are about reverting
the lookup count when error happens, except 'fuse_evict_inode' and
'fuse_cleanup_submount_lookup'. Even if there are no errors, it
still needs alloc 'struct fuse_forget_link'. It is useless, which
contributes to performance degradation and code mess to some extent.

'fuse_force_forget' does not need allocate 'struct fuse_forget_link'in
advance, and is only used by readdirplus before this patch for the reason
that we do not know how many 'fuse_forget_link' structures will be
allocated when error happens.

Signed-off-by: yangyun <yangyun50@huawei.com>
---
 fs/fuse/dev.c     | 19 +++++++++++++++
 fs/fuse/dir.c     | 59 +++++++++++------------------------------------
 fs/fuse/fuse_i.h  |  2 ++
 fs/fuse/readdir.c | 29 +++++------------------
 4 files changed, 40 insertions(+), 69 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 9eb191b5c4de..932356833b0d 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -252,6 +252,25 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
 	}
 }
 
+void fuse_force_forget(struct fuse_mount *fm, u64 nodeid)
+{
+	struct fuse_forget_in inarg;
+	FUSE_ARGS(args);
+
+	memset(&inarg, 0, sizeof(inarg));
+	inarg.nlookup = 1;
+	args.opcode = FUSE_FORGET;
+	args.nodeid = nodeid;
+	args.in_numargs = 1;
+	args.in_args[0].size = sizeof(inarg);
+	args.in_args[0].value = &inarg;
+	args.force = true;
+	args.noreply = true;
+
+	fuse_simple_request(fm, &args);
+	/* ignore errors */
+}
+
 static void flush_bg_queue(struct fuse_conn *fc)
 {
 	struct fuse_iqueue *fiq = &fc->iq;
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2b0d4781f394..6bfb3a128658 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -207,7 +207,6 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 		 (flags & (LOOKUP_EXCL | LOOKUP_REVAL | LOOKUP_RENAME_TARGET))) {
 		struct fuse_entry_out outarg;
 		FUSE_ARGS(args);
-		struct fuse_forget_link *forget;
 		u64 attr_version;
 
 		/* For negative dentries, always do a fresh lookup */
@@ -220,11 +219,6 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 
 		fm = get_fuse_mount(inode);
 
-		forget = fuse_alloc_forget();
-		ret = -ENOMEM;
-		if (!forget)
-			goto out;
-
 		attr_version = fuse_get_attr_version(fm->fc);
 
 		parent = dget_parent(entry);
@@ -239,15 +233,13 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 			fi = get_fuse_inode(inode);
 			if (outarg.nodeid != get_node_id(inode) ||
 			    (bool) IS_AUTOMOUNT(inode) != (bool) (outarg.attr.flags & FUSE_ATTR_SUBMOUNT)) {
-				fuse_queue_forget(fm->fc, forget,
-						  outarg.nodeid, 1);
+				fuse_force_forget(fm, outarg.nodeid);
 				goto invalid;
 			}
 			spin_lock(&fi->lock);
 			fi->nlookup++;
 			spin_unlock(&fi->lock);
 		}
-		kfree(forget);
 		if (ret == -ENOMEM || ret == -EINTR)
 			goto out;
 		if (ret || fuse_invalid_attr(&outarg.attr) ||
@@ -365,7 +357,6 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
 {
 	struct fuse_mount *fm = get_fuse_mount_super(sb);
 	FUSE_ARGS(args);
-	struct fuse_forget_link *forget;
 	u64 attr_version;
 	int err;
 
@@ -374,23 +365,17 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
 	if (name->len > FUSE_NAME_MAX)
 		goto out;
 
-
-	forget = fuse_alloc_forget();
-	err = -ENOMEM;
-	if (!forget)
-		goto out;
-
 	attr_version = fuse_get_attr_version(fm->fc);
 
 	fuse_lookup_init(fm->fc, &args, nodeid, name, outarg);
 	err = fuse_simple_request(fm, &args);
 	/* Zero nodeid is same as -ENOENT, but with valid timeout */
 	if (err || !outarg->nodeid)
-		goto out_put_forget;
+		goto out;
 
 	err = -EIO;
 	if (fuse_invalid_attr(&outarg->attr))
-		goto out_put_forget;
+		goto out;
 	if (outarg->nodeid == FUSE_ROOT_ID && outarg->generation != 0) {
 		pr_warn_once("root generation should be zero\n");
 		outarg->generation = 0;
@@ -401,13 +386,11 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
 			   attr_version);
 	err = -ENOMEM;
 	if (!*inode) {
-		fuse_queue_forget(fm->fc, forget, outarg->nodeid, 1);
+		fuse_force_forget(fm, outarg->nodeid);
 		goto out;
 	}
 	err = 0;
 
- out_put_forget:
-	kfree(forget);
  out:
 	return err;
 }
@@ -617,7 +600,6 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	struct inode *inode;
 	struct fuse_mount *fm = get_fuse_mount(dir);
 	FUSE_ARGS(args);
-	struct fuse_forget_link *forget;
 	struct fuse_create_in inarg;
 	struct fuse_open_out *outopenp;
 	struct fuse_entry_out outentry;
@@ -628,15 +610,10 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	/* Userspace expects S_IFREG in create mode */
 	BUG_ON((mode & S_IFMT) != S_IFREG);
 
-	forget = fuse_alloc_forget();
-	err = -ENOMEM;
-	if (!forget)
-		goto out_err;
-
 	err = -ENOMEM;
 	ff = fuse_file_alloc(fm, true);
 	if (!ff)
-		goto out_put_forget_req;
+		goto out_err;
 
 	if (!fm->fc->dont_mask)
 		mode &= ~current_umask();
@@ -670,7 +647,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 
 	err = get_create_ext(&args, dir, entry, mode);
 	if (err)
-		goto out_put_forget_req;
+		goto out_err;
 
 	err = fuse_simple_request(fm, &args);
 	free_ext_value(&args);
@@ -690,11 +667,10 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	if (!inode) {
 		flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
 		fuse_sync_release(NULL, ff, flags);
-		fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
+		fuse_force_forget(fm, outentry.nodeid);
 		err = -ENOMEM;
 		goto out_err;
 	}
-	kfree(forget);
 	d_instantiate(entry, inode);
 	fuse_change_entry_timeout(entry, &outentry);
 	fuse_dir_changed(dir);
@@ -716,8 +692,6 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 
 out_free_ff:
 	fuse_file_free(ff);
-out_put_forget_req:
-	kfree(forget);
 out_err:
 	return err;
 }
@@ -782,15 +756,10 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	struct inode *inode;
 	struct dentry *d;
 	int err;
-	struct fuse_forget_link *forget;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
 
-	forget = fuse_alloc_forget();
-	if (!forget)
-		return -ENOMEM;
-
 	memset(&outarg, 0, sizeof(outarg));
 	args->nodeid = get_node_id(dir);
 	args->out_numargs = 1;
@@ -800,28 +769,27 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	if (args->opcode != FUSE_LINK) {
 		err = get_create_ext(args, dir, entry, mode);
 		if (err)
-			goto out_put_forget_req;
+			goto out_err;
 	}
 
 	err = fuse_simple_request(fm, args);
 	free_ext_value(args);
 	if (err)
-		goto out_put_forget_req;
+		goto out_err;
 
 	err = -EIO;
 	if (invalid_nodeid(outarg.nodeid) || fuse_invalid_attr(&outarg.attr))
-		goto out_put_forget_req;
+		goto out_err;
 
 	if ((outarg.attr.mode ^ mode) & S_IFMT)
-		goto out_put_forget_req;
+		goto out_err;
 
 	inode = fuse_iget(dir->i_sb, outarg.nodeid, outarg.generation,
 			  &outarg.attr, ATTR_TIMEOUT(&outarg), 0);
 	if (!inode) {
-		fuse_queue_forget(fm->fc, forget, outarg.nodeid, 1);
+		fuse_force_forget(fm, outarg.nodeid);
 		return -ENOMEM;
 	}
-	kfree(forget);
 
 	d_drop(entry);
 	d = d_splice_alias(inode, entry);
@@ -837,10 +805,9 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	fuse_dir_changed(dir);
 	return 0;
 
- out_put_forget_req:
+ out_err:
 	if (err == -EEXIST)
 		fuse_invalidate_entry(entry);
-	kfree(forget);
 	return err;
 }
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f23919610313..b9a5b8ec0de5 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1051,6 +1051,8 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
 void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
 		       u64 nodeid, u64 nlookup);
 
+void fuse_force_forget(struct fuse_mount *fm, u64 nodeid);
+
 struct fuse_forget_link *fuse_alloc_forget(void);
 
 struct fuse_forget_link *fuse_dequeue_forget(struct fuse_iqueue *fiq,
diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c
index 0377b6dc24c8..39f01ac31f7c 100644
--- a/fs/fuse/readdir.c
+++ b/fs/fuse/readdir.c
@@ -262,27 +262,6 @@ static int fuse_direntplus_link(struct file *file,
 	return 0;
 }
 
-static void fuse_force_forget(struct file *file, u64 nodeid)
-{
-	struct inode *inode = file_inode(file);
-	struct fuse_mount *fm = get_fuse_mount(inode);
-	struct fuse_forget_in inarg;
-	FUSE_ARGS(args);
-
-	memset(&inarg, 0, sizeof(inarg));
-	inarg.nlookup = 1;
-	args.opcode = FUSE_FORGET;
-	args.nodeid = nodeid;
-	args.in_numargs = 1;
-	args.in_args[0].size = sizeof(inarg);
-	args.in_args[0].value = &inarg;
-	args.force = true;
-	args.noreply = true;
-
-	fuse_simple_request(fm, &args);
-	/* ignore errors */
-}
-
 static int parse_dirplusfile(char *buf, size_t nbytes, struct file *file,
 			     struct dir_context *ctx, u64 attr_version)
 {
@@ -320,8 +299,12 @@ static int parse_dirplusfile(char *buf, size_t nbytes, struct file *file,
 		nbytes -= reclen;
 
 		ret = fuse_direntplus_link(file, direntplus, attr_version);
-		if (ret)
-			fuse_force_forget(file, direntplus->entry_out.nodeid);
+		if (ret) {
+			struct inode *inode = file_inode(file);
+			struct fuse_mount *fm = get_fuse_mount(inode);
+
+			fuse_force_forget(fm, direntplus->entry_out.nodeid);
+		}
 	}
 
 	return 0;
-- 
2.33.0

[PATCH 2/2] fuse: add support for no forget requests

[yangyun]

FUSE_FORGET requests are not used if the fuse file system does not
implement the forget operation in userspace (e.g., fuse file system
does not cache any inodes).

However, the kernel is invisible to the userspace implementation and
always sends FUSE_FORGET requests, which can lead to performance
degradation because of useless contex switch and memory copy in some
cases (e.g., many inodes are evicted from icache which was described
in commit 07e77dca8a1f ("fuse: separate queue for FORGET requests")).

Just like 'no_interrupt' in 'struct fuse_conn', we add 'no_forget'.
But since FUSE_FORGET request does not have a reply from userspce,
we can not use ENOSYS to reflect the 'no_forget' assignment. So add
the FUSE_NO_FORGET_SUPPORT init flag.

Besides, if no_forget is enabled, 'nlookup' in 'struct fuse_inode'
does not used and its value change can be disabled which are protected
by spin_lock to reduce lock contention.

Signed-off-by: yangyun <yangyun50@huawei.com>
---
 fs/fuse/dev.c             |  6 ++++++
 fs/fuse/dir.c             |  4 +---
 fs/fuse/fuse_i.h          | 24 ++++++++++++++++++++++++
 fs/fuse/inode.c           | 10 +++++-----
 fs/fuse/readdir.c         |  8 ++------
 include/uapi/linux/fuse.h |  3 +++
 6 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 932356833b0d..10890db9426b 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -238,6 +238,9 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
 {
 	struct fuse_iqueue *fiq = &fc->iq;
 
+	if (fc->no_forget)
+		return;
+
 	forget->forget_one.nodeid = nodeid;
 	forget->forget_one.nlookup = nlookup;
 
@@ -257,6 +260,9 @@ void fuse_force_forget(struct fuse_mount *fm, u64 nodeid)
 	struct fuse_forget_in inarg;
 	FUSE_ARGS(args);
 
+	if (fm->fc->no_forget)
+		return;
+
 	memset(&inarg, 0, sizeof(inarg));
 	inarg.nlookup = 1;
 	args.opcode = FUSE_FORGET;
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 6bfb3a128658..833225ed1d4f 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -236,9 +236,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 				fuse_force_forget(fm, outarg.nodeid);
 				goto invalid;
 			}
-			spin_lock(&fi->lock);
-			fi->nlookup++;
-			spin_unlock(&fi->lock);
+			fuse_nlookup_inc_if_enabled(fm->fc, fi);
 		}
 		if (ret == -ENOMEM || ret == -EINTR)
 			goto out;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index b9a5b8ec0de5..924d6b0ad700 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -860,6 +860,9 @@ struct fuse_conn {
 	/** Passthrough support for read/write IO */
 	unsigned int passthrough:1;
 
+	/** Do not send FORGET request */
+	unsigned int no_forget:1;
+
 	/** Maximum stack depth for passthrough backing files */
 	int max_stack_depth;
 
@@ -1029,6 +1032,27 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
 	rcu_read_unlock();
 }
 
+static inline void fuse_nlookup_inc_if_enabled(struct fuse_conn *fc, struct fuse_inode *fi)
+{
+	if (fc->no_forget)
+		return;
+
+	spin_lock(&fi->lock);
+	fi->nlookup++;
+	spin_unlock(&fi->lock);
+}
+
+static inline void fuse_nlookup_dec_if_enabled(struct fuse_conn *fc, struct fuse_inode *fi)
+{
+	if (fc->no_forget)
+		return;
+
+	spin_lock(&fi->lock);
+	fi->nlookup--;
+	spin_lock(&fi->lock);
+}
+
+
 /** Device operations */
 extern const struct file_operations fuse_dev_operations;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 99e44ea7d875..277dc9479505 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -483,9 +483,7 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 		}
 	}
 	fi = get_fuse_inode(inode);
-	spin_lock(&fi->lock);
-	fi->nlookup++;
-	spin_unlock(&fi->lock);
+	fuse_nlookup_inc_if_enabled(fc, fi);
 done:
 	fuse_change_attributes(inode, attr, NULL, attr_valid, attr_version);
 
@@ -1331,6 +1329,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 			}
 			if (flags & FUSE_NO_EXPORT_SUPPORT)
 				fm->sb->s_export_op = &fuse_export_fid_operations;
+			if (flags & FUSE_NO_FORGET_SUPPORT)
+				fc->no_forget = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1378,7 +1378,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
 		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
 		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
-		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND;
+		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_NO_FORGET_SUPPORT;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;
@@ -1593,7 +1593,7 @@ static int fuse_fill_super_submount(struct super_block *sb,
 	 * that, though, so undo it here.
 	 */
 	fi = get_fuse_inode(root);
-	fi->nlookup--;
+	fuse_nlookup_dec_if_enabled(fm->fc, get_fuse_inode(root));
 
 	sb->s_d_op = &fuse_dentry_operations;
 	sb->s_root = d_make_root(root);
diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c
index 39f01ac31f7c..13922662e07a 100644
--- a/fs/fuse/readdir.c
+++ b/fs/fuse/readdir.c
@@ -218,9 +218,7 @@ static int fuse_direntplus_link(struct file *file,
 		}
 
 		fi = get_fuse_inode(inode);
-		spin_lock(&fi->lock);
-		fi->nlookup++;
-		spin_unlock(&fi->lock);
+		fuse_nlookup_inc_if_enabled(fc, fi);
 
 		forget_all_cached_acls(inode);
 		fuse_change_attributes(inode, &o->attr, NULL,
@@ -247,9 +245,7 @@ static int fuse_direntplus_link(struct file *file,
 			if (!IS_ERR(inode)) {
 				struct fuse_inode *fi = get_fuse_inode(inode);
 
-				spin_lock(&fi->lock);
-				fi->nlookup--;
-				spin_unlock(&fi->lock);
+				fuse_nlookup_dec_if_enabled(fc, fi);
 			}
 			return PTR_ERR(dentry);
 		}
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index d08b99d60f6f..bf660880bc7a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -217,6 +217,7 @@
  *  - add backing_id to fuse_open_out, add FOPEN_PASSTHROUGH open flag
  *  - add FUSE_NO_EXPORT_SUPPORT init flag
  *  - add FUSE_NOTIFY_RESEND, add FUSE_HAS_RESEND init flag
+ *  - add FUSE_NO_FORGET_SUPPORT init flag
  */
 
 #ifndef _LINUX_FUSE_H
@@ -421,6 +422,7 @@ struct fuse_file_lock {
  * FUSE_NO_EXPORT_SUPPORT: explicitly disable export support
  * FUSE_HAS_RESEND: kernel supports resending pending requests, and the high bit
  *		    of the request ID indicates resend requests
+ * FUSE_NO_FORGET_SUPPORT: disable forget requests
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -463,6 +465,7 @@ struct fuse_file_lock {
 #define FUSE_PASSTHROUGH	(1ULL << 37)
 #define FUSE_NO_EXPORT_SUPPORT	(1ULL << 38)
 #define FUSE_HAS_RESEND		(1ULL << 39)
+#define FUSE_NO_FORGET_SUPPORT  (1ULL << 40)
 
 /* Obsolete alias for FUSE_DIRECT_IO_ALLOW_MMAP */
 #define FUSE_DIRECT_IO_RELAX	FUSE_DIRECT_IO_ALLOW_MMAP
-- 
2.33.0

Re: [PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force_forget if error

[Josef Bacik]

On Fri, Jul 26, 2024 at 04:37:51PM +0800, yangyun wrote:
> Most usecases for 'fuse_queue_forget' in the code are about reverting
> the lookup count when error happens, except 'fuse_evict_inode' and
> 'fuse_cleanup_submount_lookup'. Even if there are no errors, it
> still needs alloc 'struct fuse_forget_link'. It is useless, which
> contributes to performance degradation and code mess to some extent.
> 
> 'fuse_force_forget' does not need allocate 'struct fuse_forget_link'in
> advance, and is only used by readdirplus before this patch for the reason
> that we do not know how many 'fuse_forget_link' structures will be
> allocated when error happens.
> 
> Signed-off-by: yangyun <yangyun50@huawei.com>

Forcing file systems to have their forget suddenly be synchronous in a lot of
cases is going to be a perf regression for them.

In some of these cases a synchronous forget is probably ok, as you say a lot of
them are error cases.  However d_revalidate() isn't.  That's us trying to figure
out if what we have in cache matches the file systems view of the inode, and if
it doesn't we're going to do a re-lookup, so we don't necessarily care for a
synchronous forget in this case.  Think of an NFS fuse client where the file got
renamed on the backend and now we're telling the kernel this is the inode we
have.  Forcing us to do a synchronous response now is going to be much more
performance impacting than it was pre-this patch.

A better approach would be to make the allocation optional based on the
->no_forget flag.  Thanks,

Josef

Re: [PATCH 2/2] fuse: add support for no forget requests

[Josef Bacik]

On Fri, Jul 26, 2024 at 04:37:52PM +0800, yangyun wrote:
> FUSE_FORGET requests are not used if the fuse file system does not
> implement the forget operation in userspace (e.g., fuse file system
> does not cache any inodes).
> 
> However, the kernel is invisible to the userspace implementation and
> always sends FUSE_FORGET requests, which can lead to performance
> degradation because of useless contex switch and memory copy in some
> cases (e.g., many inodes are evicted from icache which was described
> in commit 07e77dca8a1f ("fuse: separate queue for FORGET requests")).
> 
> Just like 'no_interrupt' in 'struct fuse_conn', we add 'no_forget'.
> But since FUSE_FORGET request does not have a reply from userspce,
> we can not use ENOSYS to reflect the 'no_forget' assignment. So add
> the FUSE_NO_FORGET_SUPPORT init flag.
> 
> Besides, if no_forget is enabled, 'nlookup' in 'struct fuse_inode'
> does not used and its value change can be disabled which are protected
> by spin_lock to reduce lock contention.
> 
> Signed-off-by: yangyun <yangyun50@huawei.com>
> ---
>  fs/fuse/dev.c             |  6 ++++++
>  fs/fuse/dir.c             |  4 +---
>  fs/fuse/fuse_i.h          | 24 ++++++++++++++++++++++++
>  fs/fuse/inode.c           | 10 +++++-----
>  fs/fuse/readdir.c         |  8 ++------
>  include/uapi/linux/fuse.h |  3 +++
>  6 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 932356833b0d..10890db9426b 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -238,6 +238,9 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
>  {
>  	struct fuse_iqueue *fiq = &fc->iq;
>  
> +	if (fc->no_forget)
> +		return;
> +
>  	forget->forget_one.nodeid = nodeid;
>  	forget->forget_one.nlookup = nlookup;
>  
> @@ -257,6 +260,9 @@ void fuse_force_forget(struct fuse_mount *fm, u64 nodeid)
>  	struct fuse_forget_in inarg;
>  	FUSE_ARGS(args);
>  
> +	if (fm->fc->no_forget)
> +		return;
> +
>  	memset(&inarg, 0, sizeof(inarg));
>  	inarg.nlookup = 1;
>  	args.opcode = FUSE_FORGET;
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 6bfb3a128658..833225ed1d4f 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -236,9 +236,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
>  				fuse_force_forget(fm, outarg.nodeid);
>  				goto invalid;
>  			}
> -			spin_lock(&fi->lock);
> -			fi->nlookup++;
> -			spin_unlock(&fi->lock);
> +			fuse_nlookup_inc_if_enabled(fm->fc, fi);
>  		}
>  		if (ret == -ENOMEM || ret == -EINTR)
>  			goto out;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index b9a5b8ec0de5..924d6b0ad700 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -860,6 +860,9 @@ struct fuse_conn {
>  	/** Passthrough support for read/write IO */
>  	unsigned int passthrough:1;
>  
> +	/** Do not send FORGET request */
> +	unsigned int no_forget:1;
> +
>  	/** Maximum stack depth for passthrough backing files */
>  	int max_stack_depth;
>  
> @@ -1029,6 +1032,27 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
>  	rcu_read_unlock();
>  }
>  
> +static inline void fuse_nlookup_inc_if_enabled(struct fuse_conn *fc, struct fuse_inode *fi)
> +{
> +	if (fc->no_forget)
> +		return;
> +
> +	spin_lock(&fi->lock);
> +	fi->nlookup++;
> +	spin_unlock(&fi->lock);
> +}
> +
> +static inline void fuse_nlookup_dec_if_enabled(struct fuse_conn *fc, struct fuse_inode *fi)
> +{
> +	if (fc->no_forget)
> +		return;
> +
> +	spin_lock(&fi->lock);
> +	fi->nlookup--;
> +	spin_lock(&fi->lock);
> +}

This naming scheme is overly verbose, you can simply have

fuse_inc_nlookup()
fuse_dec_nlookup()

Thanks,

Josef

Re: [PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force_forget if error

[yangyun]

Hi Josef,

Thanks for the comment.


On Fri, Jul 26, 2024 at 11:39:08AM -0400, Josef Bacik wrote:
> On Fri, Jul 26, 2024 at 04:37:51PM +0800, yangyun wrote:
> > Most usecases for 'fuse_queue_forget' in the code are about reverting
> > the lookup count when error happens, except 'fuse_evict_inode' and
> > 'fuse_cleanup_submount_lookup'. Even if there are no errors, it
> > still needs alloc 'struct fuse_forget_link'. It is useless, which
> > contributes to performance degradation and code mess to some extent.
> > 
> > 'fuse_force_forget' does not need allocate 'struct fuse_forget_link'in
> > advance, and is only used by readdirplus before this patch for the reason
> > that we do not know how many 'fuse_forget_link' structures will be
> > allocated when error happens.
> > 
> > Signed-off-by: yangyun <yangyun50@huawei.com>
> 
> Forcing file systems to have their forget suddenly be synchronous in a lot of
> cases is going to be a perf regression for them.

Sorry for that I didn't notice that 'fuse_force_forget' is synchronous. Actually,
I'm not on purpose to make forget be synchronous, just want to reuse the already 
known function 'fuse_force_forget' to avoid useless memory alloc in some cases.

And thank you for the reminder regarding the performance impact of synchronization.

> 
> In some of these cases a synchronous forget is probably ok, as you say a lot of
> them are error cases.  However d_revalidate() isn't.  That's us trying to figure
> out if what we have in cache matches the file systems view of the inode, and if
> it doesn't we're going to do a re-lookup, so we don't necessarily care for a
> synchronous forget in this case.  Think of an NFS fuse client where the file got
> renamed on the backend and now we're telling the kernel this is the inode we
> have.  Forcing us to do a synchronous response now is going to be much more
> performance impacting than it was pre-this patch.

Yeah, this is definitely a performance impacting case. Thank for your adivce.

> 
> A better approach would be to make the allocation optional based on the
> ->no_forget flag.  Thanks,

The reason that I don't make the allocataion optional is that even the no_forget flag
is disabled, there are still many useless memory allocations if no errors. And The code
is also a bit messy because of those allocations.

Since forget is not necessarily synchronous (In my opinion, the pre-this patch use of 
synchronous 'fuse_force_forget' is an error case and also not necessarily synchronous), 
what about just changing the 'fuse_force_forget' to be asynchronous?

By this way, all the forget requests are asynchronous (less impact on performance) and 
we doesn't need to allocate useless memory in advance. 

> 
> Josef

Thank you once again for your time and advice.

Re: [PATCH 2/2] fuse: add support for no forget requests

[yangyun]

On Fri, Jul 26, 2024 at 11:40:17AM -0400, Josef Bacik wrote:
> On Fri, Jul 26, 2024 at 04:37:52PM +0800, yangyun wrote:
> > FUSE_FORGET requests are not used if the fuse file system does not
> > implement the forget operation in userspace (e.g., fuse file system
> > does not cache any inodes).
> > 
> > However, the kernel is invisible to the userspace implementation and
> > always sends FUSE_FORGET requests, which can lead to performance
> > degradation because of useless contex switch and memory copy in some
> > cases (e.g., many inodes are evicted from icache which was described
> > in commit 07e77dca8a1f ("fuse: separate queue for FORGET requests")).
> > 
> > Just like 'no_interrupt' in 'struct fuse_conn', we add 'no_forget'.
> > But since FUSE_FORGET request does not have a reply from userspce,
> > we can not use ENOSYS to reflect the 'no_forget' assignment. So add
> > the FUSE_NO_FORGET_SUPPORT init flag.
> > 
> > Besides, if no_forget is enabled, 'nlookup' in 'struct fuse_inode'
> > does not used and its value change can be disabled which are protected
> > by spin_lock to reduce lock contention.
> > 
> > Signed-off-by: yangyun <yangyun50@huawei.com>
> > ---
> >  fs/fuse/dev.c             |  6 ++++++
> >  fs/fuse/dir.c             |  4 +---
> >  fs/fuse/fuse_i.h          | 24 ++++++++++++++++++++++++
> >  fs/fuse/inode.c           | 10 +++++-----
> >  fs/fuse/readdir.c         |  8 ++------
> >  include/uapi/linux/fuse.h |  3 +++
> >  6 files changed, 41 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 932356833b0d..10890db9426b 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -238,6 +238,9 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
> >  {
> >  	struct fuse_iqueue *fiq = &fc->iq;
> >  
> > +	if (fc->no_forget)
> > +		return;
> > +
> >  	forget->forget_one.nodeid = nodeid;
> >  	forget->forget_one.nlookup = nlookup;
> >  
> > @@ -257,6 +260,9 @@ void fuse_force_forget(struct fuse_mount *fm, u64 nodeid)
> >  	struct fuse_forget_in inarg;
> >  	FUSE_ARGS(args);
> >  
> > +	if (fm->fc->no_forget)
> > +		return;
> > +
> >  	memset(&inarg, 0, sizeof(inarg));
> >  	inarg.nlookup = 1;
> >  	args.opcode = FUSE_FORGET;
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 6bfb3a128658..833225ed1d4f 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -236,9 +236,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
> >  				fuse_force_forget(fm, outarg.nodeid);
> >  				goto invalid;
> >  			}
> > -			spin_lock(&fi->lock);
> > -			fi->nlookup++;
> > -			spin_unlock(&fi->lock);
> > +			fuse_nlookup_inc_if_enabled(fm->fc, fi);
> >  		}
> >  		if (ret == -ENOMEM || ret == -EINTR)
> >  			goto out;
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index b9a5b8ec0de5..924d6b0ad700 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -860,6 +860,9 @@ struct fuse_conn {
> >  	/** Passthrough support for read/write IO */
> >  	unsigned int passthrough:1;
> >  
> > +	/** Do not send FORGET request */
> > +	unsigned int no_forget:1;
> > +
> >  	/** Maximum stack depth for passthrough backing files */
> >  	int max_stack_depth;
> >  
> > @@ -1029,6 +1032,27 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket)
> >  	rcu_read_unlock();
> >  }
> >  
> > +static inline void fuse_nlookup_inc_if_enabled(struct fuse_conn *fc, struct fuse_inode *fi)
> > +{
> > +	if (fc->no_forget)
> > +		return;
> > +
> > +	spin_lock(&fi->lock);
> > +	fi->nlookup++;
> > +	spin_unlock(&fi->lock);
> > +}
> > +
> > +static inline void fuse_nlookup_dec_if_enabled(struct fuse_conn *fc, struct fuse_inode *fi)
> > +{
> > +	if (fc->no_forget)
> > +		return;
> > +
> > +	spin_lock(&fi->lock);
> > +	fi->nlookup--;
> > +	spin_lock(&fi->lock);
> > +}
> 
> This naming scheme is overly verbose, you can simply have
> 
> fuse_inc_nlookup()
> fuse_dec_nlookup()

Thanks for your advice.
I will modify this in the next version. 

> 
> Thanks,
> 
> Josef

Re: [PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force_forget if error

[Miklos Szeredi]

On Sat, 27 Jul 2024 at 12:06, yangyun <yangyun50@huawei.com> wrote:
> Since forget is not necessarily synchronous (In my opinion, the pre-this patch use of
> synchronous 'fuse_force_forget' is an error case and also not necessarily synchronous),
> what about just changing the 'fuse_force_forget' to be asynchronous?

Even less impact would be to move the allocation inside
fuse_force_forget (make it GFP_NOFAIL) and still use the
fuse_queue_forget() function to send the forget as e.g. virtiofs
handles them differently from regular requests.

Thanks,
Miklos

Re:[PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force_forget if error

[yangyun]

On Thu, Aug 22, 2024 at 05:26:01PM +0200, Miklos Szeredi wrote:
> On Sat, 27 Jul 2024 at 12:06, yangyun <yangyun50@huawei.com> wrote:
> > Since forget is not necessarily synchronous (In my opinion, the pre-this patch use of
> > synchronous 'fuse_force_forget' is an error case and also not necessarily synchronous),
> > what about just changing the 'fuse_force_forget' to be asynchronous?
> 
> Even less impact would be to move the allocation inside
> fuse_force_forget (make it GFP_NOFAIL) and still use the
> fuse_queue_forget() function to send the forget as e.g. virtiofs
> handles them differently from regular requests.

fuse_force_forget uses the fuse_simple_request with args.force=true and it does not need allocation outside originally, so it is strange for what you said "move the allocation inside fuse_force_forge".

I think what you mean is moving the allocation inside fuse_queue_forget, not fuse_force_forget. This can make sense.

Thanks for your advice. I will update this patch.

> 
> Thanks,
> Miklos