[PATCH v5 0/2] fuse: add timeout option for requests

[PATCH v5 0/2] fuse: add timeout option for requests

[Joanne Koong]

There are situations where fuse servers can become unresponsive or
stuck, for example if the server is in a deadlock. Currently, there's
no good way to detect if a server is stuck and needs to be killed
manually.

This patchset adds a timeout option for requests where if the server does not
reply to the request by the time the timeout elapses, the connection will be
aborted. This patchset also adds two dynamically configurable fuse sysctls
"default_request_timeout" and "max_request_timeout" for controlling/enforcing
timeout behavior system-wide.

Existing fuse servers will not be affected unless they explicitly opt into the
timeout.

v4:
https://lore.kernel.org/linux-fsdevel/20240813232241.2369855-1-joannelkoong@gmail.com/
Changes from v4 -> v5:
- Change timeout behavior from aborting request to aborting connection (Miklos)
- Clarify wording for sysctl documentation (Jingbo)

v3:
https://lore.kernel.org/linux-fsdevel/20240808190110.3188039-1-joannelkoong@gmail.com/
Changes from v3 -> v4:
- Fix wording on some comments to make it more clear
- Use simpler logic for timer (eg remove extra if checks, use mod timer API)
  (Josef)
- Sanity-check should be on FR_FINISHING not FR_FINISHED (Jingbo)
- Fix comment for "processing queue", add req->fpq = NULL safeguard  (Bernd)

v2:
https://lore.kernel.org/linux-fsdevel/20240730002348.3431931-1-joannelkoong@gmail.com/
Changes from v2 -> v3:
- Disarm / rearm timer in dev_do_read to handle race conditions (Bernrd)
- Disarm timer in error handling for fatal interrupt (Yafang)
- Clean up do_fuse_request_end (Jingbo)
- Add timer for notify retrieve requests 
- Fix kernel test robot errors for #define no-op functions

v1:
https://lore.kernel.org/linux-fsdevel/20240717213458.1613347-1-joannelkoong@gmail.com/
Changes from v1 -> v2:
- Add timeout for background requests
- Handle resend race condition
- Add sysctls


Joanne Koong (2):
  fuse: add optional kernel-enforced timeout for requests
  fuse: add default_request_timeout and max_request_timeout sysctls

 Documentation/admin-guide/sysctl/fs.rst | 31 ++++++++++++++++++
 fs/fuse/Makefile                        |  2 +-
 fs/fuse/dev.c                           | 26 ++++++++++++++-
 fs/fuse/fuse_i.h                        | 24 ++++++++++++++
 fs/fuse/inode.c                         | 24 ++++++++++++++
 fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
 6 files changed, 147 insertions(+), 2 deletions(-)
 create mode 100644 fs/fuse/sysctl.c

-- 
2.43.5

[PATCH v5 1/2] fuse: add optional kernel-enforced timeout for requests

[Joanne Koong]

There are situations where fuse servers can become unresponsive or
stuck, for example if the server is in a deadlock. Currently, there's
no good way to detect if a server is stuck and needs to be killed
manually.

This commit adds an option for enforcing a timeout (in seconds) on
requests where if the timeout elapses without a reply from the server,
the connection will be automatically aborted.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev.c    | 26 +++++++++++++++++++++++++-
 fs/fuse/fuse_i.h |  8 ++++++++
 fs/fuse/inode.c  |  7 +++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 9eb191b5c4de..a4ec817074a2 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -40,6 +40,16 @@ static struct fuse_dev *fuse_get_dev(struct file *file)
 	return READ_ONCE(file->private_data);
 }
 
+static void fuse_request_timeout(struct timer_list *timer)
+{
+	struct fuse_req *req = container_of(timer, struct fuse_req, timer);
+	struct fuse_conn *fc = req->fm->fc;
+
+	req->timer.function = NULL;
+
+	fuse_abort_conn(fc);
+}
+
 static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
 {
 	INIT_LIST_HEAD(&req->list);
@@ -48,6 +58,8 @@ static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
 	refcount_set(&req->count, 1);
 	__set_bit(FR_PENDING, &req->flags);
 	req->fm = fm;
+	if (fm->fc->req_timeout)
+		timer_setup(&req->timer, fuse_request_timeout, 0);
 }
 
 static struct fuse_req *fuse_request_alloc(struct fuse_mount *fm, gfp_t flags)
@@ -283,6 +295,9 @@ void fuse_request_end(struct fuse_req *req)
 	struct fuse_conn *fc = fm->fc;
 	struct fuse_iqueue *fiq = &fc->iq;
 
+	if (req->timer.function)
+		timer_delete_sync(&req->timer);
+
 	if (test_and_set_bit(FR_FINISHED, &req->flags))
 		goto put_request;
 
@@ -393,6 +408,8 @@ static void request_wait_answer(struct fuse_req *req)
 		if (test_bit(FR_PENDING, &req->flags)) {
 			list_del(&req->list);
 			spin_unlock(&fiq->lock);
+			if (req->timer.function)
+				timer_delete_sync(&req->timer);
 			__fuse_put_request(req);
 			req->out.h.error = -EINTR;
 			return;
@@ -409,7 +426,8 @@ static void request_wait_answer(struct fuse_req *req)
 
 static void __fuse_request_send(struct fuse_req *req)
 {
-	struct fuse_iqueue *fiq = &req->fm->fc->iq;
+	struct fuse_conn *fc = req->fm->fc;
+	struct fuse_iqueue *fiq = &fc->iq;
 
 	BUG_ON(test_bit(FR_BACKGROUND, &req->flags));
 	spin_lock(&fiq->lock);
@@ -421,6 +439,8 @@ static void __fuse_request_send(struct fuse_req *req)
 		/* acquire extra reference, since request is still needed
 		   after fuse_request_end() */
 		__fuse_get_request(req);
+		if (req->timer.function)
+			mod_timer(&req->timer, jiffies + fc->req_timeout);
 		queue_request_and_unlock(fiq, req);
 
 		request_wait_answer(req);
@@ -539,6 +559,8 @@ static bool fuse_request_queue_background(struct fuse_req *req)
 		if (fc->num_background == fc->max_background)
 			fc->blocked = 1;
 		list_add_tail(&req->list, &fc->bg_queue);
+		if (req->timer.function)
+			mod_timer(&req->timer, jiffies + fc->req_timeout);
 		flush_bg_queue(fc);
 		queued = true;
 	}
@@ -594,6 +616,8 @@ static int fuse_simple_notify_reply(struct fuse_mount *fm,
 
 	spin_lock(&fiq->lock);
 	if (fiq->connected) {
+		if (req->timer.function)
+			mod_timer(&req->timer, jiffies + fm->fc->req_timeout);
 		queue_request_and_unlock(fiq, req);
 	} else {
 		err = -ENODEV;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f23919610313..97dacafa4289 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -435,6 +435,9 @@ struct fuse_req {
 
 	/** fuse_mount this request belongs to */
 	struct fuse_mount *fm;
+
+	/** timer for request replies, if timeout option is enabled */
+	struct timer_list timer;
 };
 
 struct fuse_iqueue;
@@ -574,6 +577,8 @@ struct fuse_fs_context {
 	enum fuse_dax_mode dax_mode;
 	unsigned int max_read;
 	unsigned int blksize;
+	/*  Request timeout (in seconds). 0 = no timeout (infinite wait) */
+	unsigned int req_timeout;
 	const char *subtype;
 
 	/* DAX device, may be NULL */
@@ -633,6 +638,9 @@ struct fuse_conn {
 	/** Constrain ->max_pages to this value during feature negotiation */
 	unsigned int max_pages_limit;
 
+	/* Request timeout (in jiffies). 0 = no timeout (infinite wait) */
+	unsigned long req_timeout;
+
 	/** Input queue */
 	struct fuse_iqueue iq;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 99e44ea7d875..9e69006fc026 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -733,6 +733,7 @@ enum {
 	OPT_ALLOW_OTHER,
 	OPT_MAX_READ,
 	OPT_BLKSIZE,
+	OPT_REQUEST_TIMEOUT,
 	OPT_ERR
 };
 
@@ -747,6 +748,7 @@ static const struct fs_parameter_spec fuse_fs_parameters[] = {
 	fsparam_u32	("max_read",		OPT_MAX_READ),
 	fsparam_u32	("blksize",		OPT_BLKSIZE),
 	fsparam_string	("subtype",		OPT_SUBTYPE),
+	fsparam_u32	("request_timeout",	OPT_REQUEST_TIMEOUT),
 	{}
 };
 
@@ -830,6 +832,10 @@ static int fuse_parse_param(struct fs_context *fsc, struct fs_parameter *param)
 		ctx->blksize = result.uint_32;
 		break;
 
+	case OPT_REQUEST_TIMEOUT:
+		ctx->req_timeout = result.uint_32;
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -1724,6 +1730,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	fc->group_id = ctx->group_id;
 	fc->legacy_opts_show = ctx->legacy_opts_show;
 	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
+	fc->req_timeout = ctx->req_timeout * HZ;
 	fc->destroy = ctx->destroy;
 	fc->no_control = ctx->no_control;
 	fc->no_force_umount = ctx->no_force_umount;
-- 
2.43.5

[PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

Introduce two new sysctls, "default_request_timeout" and
"max_request_timeout". These control how long (in seconds) a server can
take to reply to a request. If the server does not reply by the timeout,
then the connection will be aborted.

"default_request_timeout" sets the default timeout if no timeout is
specified by the fuse server on mount. 0 (default) indicates no default
timeout should be enforced. If the server did set a timeout, then
default_request_timeout will be ignored.

"max_request_timeout" sets the upper bound on how long the server may
take to reply to a request. 0 (default) indicates no maximum timeout. If
the max_request_timeout is set and the fuse server attempts to set a
timeout greater than max_request_timeout, the system will default to
max_request_timeout. Similarly, if default_request_timeout is greater
than max_request_timeout, the system will default to
max_request_timeout. If the server does not request a timeout and
default_request_timeout is set to 0 but max_request_timeout is set, then
the timeout will be max_request_timeout.

$ sysctl -a | grep fuse
fs.fuse.default_request_timeout = 0
fs.fuse.max_request_timeout = 0

$ echo 0x100000000 | sudo tee /proc/sys/fs/fuse/default_request_timeout
tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument

$ echo 0xFFFFFFFF | sudo tee /proc/sys/fs/fuse/default_request_timeout
0xFFFFFFFF

$ sysctl -a | grep fuse
fs.fuse.default_request_timeout = 4294967295
fs.fuse.max_request_timeout = 0

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Bernd Schubert <bschubert@ddn.com>
---
 Documentation/admin-guide/sysctl/fs.rst | 31 ++++++++++++++++++
 fs/fuse/Makefile                        |  2 +-
 fs/fuse/fuse_i.h                        | 16 ++++++++++
 fs/fuse/inode.c                         | 19 ++++++++++-
 fs/fuse/sysctl.c                        | 42 +++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 fs/fuse/sysctl.c

diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
index 47499a1742bd..3d5a2b5cbba0 100644
--- a/Documentation/admin-guide/sysctl/fs.rst
+++ b/Documentation/admin-guide/sysctl/fs.rst
@@ -332,3 +332,34 @@ Each "watch" costs roughly 90 bytes on a 32-bit kernel, and roughly 160 bytes
 on a 64-bit one.
 The current default value for ``max_user_watches`` is 4% of the
 available low memory, divided by the "watch" cost in bytes.
+
+5. /proc/sys/fs/fuse - Configuration options for FUSE filesystems
+=====================================================================
+
+This directory contains the following configuration options for FUSE
+filesystems:
+
+``/proc/sys/fs/fuse/default_request_timeout`` is a read/write file for
+setting/getting the default timeout (in seconds) for a fuse server to
+reply to a kernel-issued request in the event where the server did not
+specify a timeout at mount. If the server set a timeout,
+then default_request_timeout will be ignored.  The default
+"default_request_timeout" is set to 0. 0 indicates a no-op (eg
+requests will not have a default request timeout set if no timeout was
+specified by the server).
+
+``/proc/sys/fs/fuse/max_request_timeout`` is a read/write file for
+setting/getting the maximum timeout (in seconds) for a fuse server to
+reply to a kernel-issued request. A value greater than 0 automatically opts
+the server into a timeout that will be at most "max_request_timeout", even if
+the server did not specify a timeout and default_request_timeout is set to 0.
+If max_request_timeout is greater than 0 and the server set a timeout greater
+than max_request_timeout or default_request_timeout is set to greater than
+max_request_timeout, the system will use max_request_timeout as the timeout.
+0 indicates a no-op (eg requests will not have an upper bound on the timeout
+and if the server did not request a timeout and default_request_timeout was not
+set, there will be no timeout).
+
+Please note that for the timeout options, if the server does not respond to
+the request by the time the timeout elapses, then the connection to the fuse
+server will be aborted.
diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
index 6e0228c6d0cb..cd4ef3e08ebf 100644
--- a/fs/fuse/Makefile
+++ b/fs/fuse/Makefile
@@ -7,7 +7,7 @@ obj-$(CONFIG_FUSE_FS) += fuse.o
 obj-$(CONFIG_CUSE) += cuse.o
 obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
 
-fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
+fuse-y := dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o sysctl.o
 fuse-y += iomode.o
 fuse-$(CONFIG_FUSE_DAX) += dax.o
 fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 97dacafa4289..04daf366735d 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -47,6 +47,14 @@
 /** Number of dentries for each connection in the control filesystem */
 #define FUSE_CTL_NUM_DENTRIES 5
 
+/*
+ * Default timeout (in seconds) for the server to reply to a request
+ * if no timeout was specified on mount
+ */
+extern u32 fuse_default_req_timeout;
+/** Max timeout (in seconds) for the server to reply to a request */
+extern u32 fuse_max_req_timeout;
+
 /** List of active connections */
 extern struct list_head fuse_conn_list;
 
@@ -1480,4 +1488,12 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
 				      size_t len, unsigned int flags);
 ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma);
 
+#ifdef CONFIG_SYSCTL
+int fuse_sysctl_register(void);
+void fuse_sysctl_unregister(void);
+#else
+static inline int fuse_sysctl_register(void) { return 0; }
+static inline void fuse_sysctl_unregister(void) { return; }
+#endif /* CONFIG_SYSCTL */
+
 #endif /* _FS_FUSE_I_H */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 9e69006fc026..cf333448f2d3 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -35,6 +35,10 @@ DEFINE_MUTEX(fuse_mutex);
 
 static int set_global_limit(const char *val, const struct kernel_param *kp);
 
+/* default is no timeout */
+u32 fuse_default_req_timeout = 0;
+u32 fuse_max_req_timeout = 0;
+
 unsigned max_user_bgreq;
 module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
 		  &max_user_bgreq, 0644);
@@ -1678,6 +1682,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	struct fuse_conn *fc = fm->fc;
 	struct inode *root;
 	struct dentry *root_dentry;
+	u32 req_timeout;
 	int err;
 
 	err = -EINVAL;
@@ -1730,10 +1735,16 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	fc->group_id = ctx->group_id;
 	fc->legacy_opts_show = ctx->legacy_opts_show;
 	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
-	fc->req_timeout = ctx->req_timeout * HZ;
 	fc->destroy = ctx->destroy;
 	fc->no_control = ctx->no_control;
 	fc->no_force_umount = ctx->no_force_umount;
+	req_timeout = ctx->req_timeout ?: fuse_default_req_timeout;
+	if (!fuse_max_req_timeout)
+		fc->req_timeout = req_timeout * HZ;
+	else if (!req_timeout)
+		fc->req_timeout = fuse_max_req_timeout * HZ;
+	else
+		fc->req_timeout = min(req_timeout, fuse_max_req_timeout) * HZ;
 
 	err = -ENOMEM;
 	root = fuse_get_root_inode(sb, ctx->rootmode);
@@ -2046,8 +2057,14 @@ static int __init fuse_fs_init(void)
 	if (err)
 		goto out3;
 
+	err = fuse_sysctl_register();
+	if (err)
+		goto out4;
+
 	return 0;
 
+ out4:
+	unregister_filesystem(&fuse_fs_type);
  out3:
 	unregister_fuseblk();
  out2:
diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
new file mode 100644
index 000000000000..c87bb0ecbfa9
--- /dev/null
+++ b/fs/fuse/sysctl.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+* linux/fs/fuse/fuse_sysctl.c
+*
+* Sysctl interface to fuse parameters
+*/
+#include <linux/sysctl.h>
+
+#include "fuse_i.h"
+
+static struct ctl_table_header *fuse_table_header;
+
+static struct ctl_table fuse_sysctl_table[] = {
+	{
+		.procname	= "default_request_timeout",
+		.data		= &fuse_default_req_timeout,
+		.maxlen		= sizeof(fuse_default_req_timeout),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+	{
+		.procname	= "max_request_timeout",
+		.data		= &fuse_max_req_timeout,
+		.maxlen		= sizeof(fuse_max_req_timeout),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+};
+
+int fuse_sysctl_register(void)
+{
+	fuse_table_header = register_sysctl("fs/fuse", fuse_sysctl_table);
+	if (!fuse_table_header)
+		return -ENOMEM;
+	return 0;
+}
+
+void fuse_sysctl_unregister(void)
+{
+	unregister_sysctl_table(fuse_table_header);
+	fuse_table_header = NULL;
+}
-- 
2.43.5

Re: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[kernel test robot]

Hi Joanne,

kernel test robot noticed the following build errors:

[auto build test ERROR on mszeredi-fuse/for-next]
[also build test ERROR on linus/master v6.11-rc5 next-20240827]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/fuse-add-optional-kernel-enforced-timeout-for-requests/20240827-043354
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
patch link:    https://lore.kernel.org/r/20240826203234.4079338-3-joannelkoong%40gmail.com
patch subject: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls
config: arc-randconfig-002-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408280419.yuu33o7t-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/fuse/sysctl.c:30:5: error: redefinition of 'fuse_sysctl_register'
      30 | int fuse_sysctl_register(void)
         |     ^~~~~~~~~~~~~~~~~~~~
   In file included from fs/fuse/sysctl.c:9:
   fs/fuse/fuse_i.h:1495:19: note: previous definition of 'fuse_sysctl_register' with type 'int(void)'
    1495 | static inline int fuse_sysctl_register(void) { return 0; }
         |                   ^~~~~~~~~~~~~~~~~~~~
>> fs/fuse/sysctl.c:38:6: error: redefinition of 'fuse_sysctl_unregister'
      38 | void fuse_sysctl_unregister(void)
         |      ^~~~~~~~~~~~~~~~~~~~~~
   fs/fuse/fuse_i.h:1496:20: note: previous definition of 'fuse_sysctl_unregister' with type 'void(void)'
    1496 | static inline void fuse_sysctl_unregister(void) { return; }
         |                    ^~~~~~~~~~~~~~~~~~~~~~


vim +/fuse_sysctl_register +30 fs/fuse/sysctl.c

    29	
  > 30	int fuse_sysctl_register(void)
    31	{
    32		fuse_table_header = register_sysctl("fs/fuse", fuse_sysctl_table);
    33		if (!fuse_table_header)
    34			return -ENOMEM;
    35		return 0;
    36	}
    37	
  > 38	void fuse_sysctl_unregister(void)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Joanne Koong]

On Tue, Aug 27, 2024 at 2:52 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Joanne,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on mszeredi-fuse/for-next]
> [also build test ERROR on linus/master v6.11-rc5 next-20240827]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/fuse-add-optional-kernel-enforced-timeout-for-requests/20240827-043354
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
> patch link:    https://lore.kernel.org/r/20240826203234.4079338-3-joannelkoong%40gmail.com
> patch subject: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls
> config: arc-randconfig-002-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/config)
> compiler: arceb-elf-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202408280419.yuu33o7t-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> >> fs/fuse/sysctl.c:30:5: error: redefinition of 'fuse_sysctl_register'
>       30 | int fuse_sysctl_register(void)
>          |     ^~~~~~~~~~~~~~~~~~~~
>    In file included from fs/fuse/sysctl.c:9:
>    fs/fuse/fuse_i.h:1495:19: note: previous definition of 'fuse_sysctl_register' with type 'int(void)'
>     1495 | static inline int fuse_sysctl_register(void) { return 0; }
>          |                   ^~~~~~~~~~~~~~~~~~~~
> >> fs/fuse/sysctl.c:38:6: error: redefinition of 'fuse_sysctl_unregister'
>       38 | void fuse_sysctl_unregister(void)
>          |      ^~~~~~~~~~~~~~~~~~~~~~
>    fs/fuse/fuse_i.h:1496:20: note: previous definition of 'fuse_sysctl_unregister' with type 'void(void)'
>     1496 | static inline void fuse_sysctl_unregister(void) { return; }
>          |                    ^~~~~~~~~~~~~~~~~~~~~~
>

I see. In the Makefile, the sysctl.o needs to be gated by CONFIG_SYSCTL
eg
fuse-$(CONFIG_SYSCTL) += sysctl.o

I'll wait a bit to see if there are more comments on this patchset
before submitting v6.

>
> vim +/fuse_sysctl_register +30 fs/fuse/sysctl.c
>
>     29
>   > 30  int fuse_sysctl_register(void)
>     31  {
>     32          fuse_table_header = register_sysctl("fs/fuse", fuse_sysctl_table);
>     33          if (!fuse_table_header)
>     34                  return -ENOMEM;
>     35          return 0;
>     36  }
>     37
>   > 38  void fuse_sysctl_unregister(void)
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Yafang Shao]

On Wed, Aug 28, 2024 at 11:51 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Tue, Aug 27, 2024 at 2:52 PM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Joanne,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on mszeredi-fuse/for-next]
> > [also build test ERROR on linus/master v6.11-rc5 next-20240827]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/fuse-add-optional-kernel-enforced-timeout-for-requests/20240827-043354
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
> > patch link:    https://lore.kernel.org/r/20240826203234.4079338-3-joannelkoong%40gmail.com
> > patch subject: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls
> > config: arc-randconfig-002-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/config)
> > compiler: arceb-elf-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202408280419.yuu33o7t-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> > >> fs/fuse/sysctl.c:30:5: error: redefinition of 'fuse_sysctl_register'
> >       30 | int fuse_sysctl_register(void)
> >          |     ^~~~~~~~~~~~~~~~~~~~
> >    In file included from fs/fuse/sysctl.c:9:
> >    fs/fuse/fuse_i.h:1495:19: note: previous definition of 'fuse_sysctl_register' with type 'int(void)'
> >     1495 | static inline int fuse_sysctl_register(void) { return 0; }
> >          |                   ^~~~~~~~~~~~~~~~~~~~
> > >> fs/fuse/sysctl.c:38:6: error: redefinition of 'fuse_sysctl_unregister'
> >       38 | void fuse_sysctl_unregister(void)
> >          |      ^~~~~~~~~~~~~~~~~~~~~~
> >    fs/fuse/fuse_i.h:1496:20: note: previous definition of 'fuse_sysctl_unregister' with type 'void(void)'
> >     1496 | static inline void fuse_sysctl_unregister(void) { return; }
> >          |                    ^~~~~~~~~~~~~~~~~~~~~~
> >
>
> I see. In the Makefile, the sysctl.o needs to be gated by CONFIG_SYSCTL
> eg
> fuse-$(CONFIG_SYSCTL) += sysctl.o
>
> I'll wait a bit to see if there are more comments on this patchset
> before submitting v6.

Hello Joanne,

I noticed a change in behavior between versions v5 and v4 during my
hellofuse test.

- Setup:
  1. Set fs.fuse.default_request_timeout to 10.
  2. Start the hellofuse daemon, with FUSE mounted on /tmp/fuse/.
  3. Run `cat /tmp/fuse/hello` and kill it within 10 seconds to
trigger a Timer expired event.
  4. Run `cat /tmp/fuse/hello` again.

- v4:
  After the Timer expired event occurs, running `cat /tmp/fuse/hello`
again is successful.

- v5:
  Running `cat /tmp/fuse/hello` fails with the error: "Transport
endpoint is not connected."

I believe this behavior in v5 is unintended, correct?

--
Regards



Yafang

Re: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Jingbo Xu]

Hi, Yafang,

On 8/29/24 11:58 AM, Yafang Shao wrote:
> On Wed, Aug 28, 2024 at 11:51 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>>
>> On Tue, Aug 27, 2024 at 2:52 PM kernel test robot <lkp@intel.com> wrote:
>>>
>>> Hi Joanne,
>>>
>>> kernel test robot noticed the following build errors:
>>>
>>> [auto build test ERROR on mszeredi-fuse/for-next]
>>> [also build test ERROR on linus/master v6.11-rc5 next-20240827]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/fuse-add-optional-kernel-enforced-timeout-for-requests/20240827-043354
>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
>>> patch link:    https://lore.kernel.org/r/20240826203234.4079338-3-joannelkoong%40gmail.com
>>> patch subject: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls
>>> config: arc-randconfig-002-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/config)
>>> compiler: arceb-elf-gcc (GCC) 13.2.0
>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202408280419.yuu33o7t-lkp@intel.com/
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> fs/fuse/sysctl.c:30:5: error: redefinition of 'fuse_sysctl_register'
>>>       30 | int fuse_sysctl_register(void)
>>>          |     ^~~~~~~~~~~~~~~~~~~~
>>>    In file included from fs/fuse/sysctl.c:9:
>>>    fs/fuse/fuse_i.h:1495:19: note: previous definition of 'fuse_sysctl_register' with type 'int(void)'
>>>     1495 | static inline int fuse_sysctl_register(void) { return 0; }
>>>          |                   ^~~~~~~~~~~~~~~~~~~~
>>>>> fs/fuse/sysctl.c:38:6: error: redefinition of 'fuse_sysctl_unregister'
>>>       38 | void fuse_sysctl_unregister(void)
>>>          |      ^~~~~~~~~~~~~~~~~~~~~~
>>>    fs/fuse/fuse_i.h:1496:20: note: previous definition of 'fuse_sysctl_unregister' with type 'void(void)'
>>>     1496 | static inline void fuse_sysctl_unregister(void) { return; }
>>>          |                    ^~~~~~~~~~~~~~~~~~~~~~
>>>
>>
>> I see. In the Makefile, the sysctl.o needs to be gated by CONFIG_SYSCTL
>> eg
>> fuse-$(CONFIG_SYSCTL) += sysctl.o
>>
>> I'll wait a bit to see if there are more comments on this patchset
>> before submitting v6.
> 
> Hello Joanne,
> 
> I noticed a change in behavior between versions v5 and v4 during my
> hellofuse test.
> 
> - Setup:
>   1. Set fs.fuse.default_request_timeout to 10.
>   2. Start the hellofuse daemon, with FUSE mounted on /tmp/fuse/.
>   3. Run `cat /tmp/fuse/hello` and kill it within 10 seconds to
> trigger a Timer expired event.
>   4. Run `cat /tmp/fuse/hello` again.
> 
> - v4:
>   After the Timer expired event occurs, running `cat /tmp/fuse/hello`
> again is successful.
> 
> - v5:
>   Running `cat /tmp/fuse/hello` fails with the error: "Transport
> endpoint is not connected."
> 
> I believe this behavior in v5 is unintended, correct?
> 

I think v5 has changed the per-request timeout to per-connection timeout
according to Miklos's suggestion.  That is, once timedout, the whole
connection will be aborted.


-- 
Thanks,
Jingbo

Re: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls

[Yafang Shao]

On Thu, Aug 29, 2024 at 2:38 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> Hi, Yafang,
>
> On 8/29/24 11:58 AM, Yafang Shao wrote:
> > On Wed, Aug 28, 2024 at 11:51 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >>
> >> On Tue, Aug 27, 2024 at 2:52 PM kernel test robot <lkp@intel.com> wrote:
> >>>
> >>> Hi Joanne,
> >>>
> >>> kernel test robot noticed the following build errors:
> >>>
> >>> [auto build test ERROR on mszeredi-fuse/for-next]
> >>> [also build test ERROR on linus/master v6.11-rc5 next-20240827]
> >>> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >>> And when submitting patch, we suggest to use '--base' as documented in
> >>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>>
> >>> url:    https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/fuse-add-optional-kernel-enforced-timeout-for-requests/20240827-043354
> >>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
> >>> patch link:    https://lore.kernel.org/r/20240826203234.4079338-3-joannelkoong%40gmail.com
> >>> patch subject: [PATCH v5 2/2] fuse: add default_request_timeout and max_request_timeout sysctls
> >>> config: arc-randconfig-002-20240827 (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/config)
> >>> compiler: arceb-elf-gcc (GCC) 13.2.0
> >>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280419.yuu33o7t-lkp@intel.com/reproduce)
> >>>
> >>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >>> the same patch/commit), kindly add following tags
> >>> | Reported-by: kernel test robot <lkp@intel.com>
> >>> | Closes: https://lore.kernel.org/oe-kbuild-all/202408280419.yuu33o7t-lkp@intel.com/
> >>>
> >>> All errors (new ones prefixed by >>):
> >>>
> >>>>> fs/fuse/sysctl.c:30:5: error: redefinition of 'fuse_sysctl_register'
> >>>       30 | int fuse_sysctl_register(void)
> >>>          |     ^~~~~~~~~~~~~~~~~~~~
> >>>    In file included from fs/fuse/sysctl.c:9:
> >>>    fs/fuse/fuse_i.h:1495:19: note: previous definition of 'fuse_sysctl_register' with type 'int(void)'
> >>>     1495 | static inline int fuse_sysctl_register(void) { return 0; }
> >>>          |                   ^~~~~~~~~~~~~~~~~~~~
> >>>>> fs/fuse/sysctl.c:38:6: error: redefinition of 'fuse_sysctl_unregister'
> >>>       38 | void fuse_sysctl_unregister(void)
> >>>          |      ^~~~~~~~~~~~~~~~~~~~~~
> >>>    fs/fuse/fuse_i.h:1496:20: note: previous definition of 'fuse_sysctl_unregister' with type 'void(void)'
> >>>     1496 | static inline void fuse_sysctl_unregister(void) { return; }
> >>>          |                    ^~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>
> >> I see. In the Makefile, the sysctl.o needs to be gated by CONFIG_SYSCTL
> >> eg
> >> fuse-$(CONFIG_SYSCTL) += sysctl.o
> >>
> >> I'll wait a bit to see if there are more comments on this patchset
> >> before submitting v6.
> >
> > Hello Joanne,
> >
> > I noticed a change in behavior between versions v5 and v4 during my
> > hellofuse test.
> >
> > - Setup:
> >   1. Set fs.fuse.default_request_timeout to 10.
> >   2. Start the hellofuse daemon, with FUSE mounted on /tmp/fuse/.
> >   3. Run `cat /tmp/fuse/hello` and kill it within 10 seconds to
> > trigger a Timer expired event.
> >   4. Run `cat /tmp/fuse/hello` again.
> >
> > - v4:
> >   After the Timer expired event occurs, running `cat /tmp/fuse/hello`
> > again is successful.
> >
> > - v5:
> >   Running `cat /tmp/fuse/hello` fails with the error: "Transport
> > endpoint is not connected."
> >
> > I believe this behavior in v5 is unintended, correct?
> >
>
> I think v5 has changed the per-request timeout to per-connection timeout
> according to Miklos's suggestion.  That is, once timedout, the whole
> connection will be aborted.

Understood. Thanks for your explanation.

-- 
Regards
Yafang

Re: [PATCH v5 0/2] fuse: add timeout option for requests

[Miklos Szeredi]

On Mon, 26 Aug 2024 at 22:33, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> There are situations where fuse servers can become unresponsive or
> stuck, for example if the server is in a deadlock. Currently, there's
> no good way to detect if a server is stuck and needs to be killed
> manually.
>
> This patchset adds a timeout option for requests where if the server does not
> reply to the request by the time the timeout elapses, the connection will be
> aborted. This patchset also adds two dynamically configurable fuse sysctls
> "default_request_timeout" and "max_request_timeout" for controlling/enforcing
> timeout behavior system-wide.
>
> Existing fuse servers will not be affected unless they explicitly opt into the
> timeout.

This last paragraph seems to contradict the purpose of the system-wide
maximum timeout.  If the server can opt out of timeouts, then
enforcing a maximum timeout is pointless.

Am I missing something?

Thanks,
Miklos

Re: [PATCH v5 0/2] fuse: add timeout option for requests

[Joanne Koong]

On Mon, Aug 26, 2024 at 11:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 26 Aug 2024 at 22:33, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > There are situations where fuse servers can become unresponsive or
> > stuck, for example if the server is in a deadlock. Currently, there's
> > no good way to detect if a server is stuck and needs to be killed
> > manually.
> >
> > This patchset adds a timeout option for requests where if the server does not
> > reply to the request by the time the timeout elapses, the connection will be
> > aborted. This patchset also adds two dynamically configurable fuse sysctls
> > "default_request_timeout" and "max_request_timeout" for controlling/enforcing
> > timeout behavior system-wide.
> >
> > Existing fuse servers will not be affected unless they explicitly opt into the
> > timeout.
>
> This last paragraph seems to contradict the purpose of the system-wide
> maximum timeout.  If the server can opt out of timeouts, then
> enforcing a maximum timeout is pointless.
>
> Am I missing something?

Ah by that last paragraph, my intention was to convey that for
existing systems that run fuse servers, pre-existing behavior will
stay as is and they don't have to worry about any functional changes
from this patchset if they don't explicitly opt into it. I'll change
this wording to "Existing systems running fuse servers will not be
affected unless they explicitly opt into the timeout".


Thanks,
Joanne
>
> Thanks,
> Miklos