[PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[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 where if the server does not reply to a
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 systems running fuse servers will not be affected unless they
explicitly opt into the timeout.

v10:
https://lore.kernel.org/linux-fsdevel/20241214022827.1773071-1-joannelkoong@gmail.com/
Changes from v10 -> v11:
* Refactor check for request expiration (Sergey)
* Move workqueue cancellation to earlier in function (Jeff)
* Check fc->num_waiting as a shortcut in workqueue job (Etienne)

v9:
https://lore.kernel.org/linux-fsdevel/20241114191332.669127-1-joannelkoong@gmail.com/
Changes from v9 -> v10:
* Use delayed workqueues instead of timers (Sergey and Jeff)
* Change granularity to seconds instead of minutes (Sergey and Jeff)
* Use time_after() api for checking jiffies expiration (Sergey)
* Change timer check to run every 15 secs instead of every min
* Update documentation wording to be more clear

v8:
https://lore.kernel.org/linux-fsdevel/20241011191320.91592-1-joannelkoong@gmail.com/
Changes from v8 -> v9:
* Fix comment for u16 fs_parse_result, ULONG_MAX instead of U32_MAX, fix
  spacing (Bernd)

v7:
https://lore.kernel.org/linux-fsdevel/20241007184258.2837492-1-joannelkoong@gmail.com/
Changes from v7 -> v8:
* Use existing lists for checking expirations (Miklos)

v6:
https://lore.kernel.org/linux-fsdevel/20240830162649.3849586-1-joannelkoong@gmail.com/
Changes from v6 -> v7:
- Make timer per-connection instead of per-request (Miklos)
- Make default granularity of time minutes instead of seconds
- Removed the reviewed-bys since the interface of this has changed (now
  minutes, instead of seconds)

v5:
https://lore.kernel.org/linux-fsdevel/20240826203234.4079338-1-joannelkoong@gmail.com/
Changes from v5 -> v6:
- Gate sysctl.o behind CONFIG_SYSCTL in makefile (kernel test robot)
- Reword/clarify last sentence in cover letter (Miklos)

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 kernel-enforced timeout option for requests
  fuse: add default_request_timeout and max_request_timeout sysctls

 Documentation/admin-guide/sysctl/fs.rst | 25 ++++++++
 fs/fuse/dev.c                           | 85 +++++++++++++++++++++++++
 fs/fuse/fuse_i.h                        | 32 ++++++++++
 fs/fuse/inode.c                         | 35 ++++++++++
 fs/fuse/sysctl.c                        | 14 ++++
 5 files changed, 191 insertions(+)

-- 
2.43.5

[PATCH v11 1/2] fuse: add kernel-enforced timeout option for requests

[Joanne Koong]

There are situations where fuse servers can become unresponsive or
stuck, for example if the server is deadlocked. 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) for
requests where if the timeout elapses without the server responding to
the request, the connection will be automatically aborted.

Please note that these timeouts are not 100% precise. For example, the
request may take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond
the requested timeout due to internal implementation, in order to
mitigate overhead.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/fuse/fuse_i.h | 22 +++++++++++++
 fs/fuse/inode.c  | 23 +++++++++++++
 3 files changed, 130 insertions(+)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 27ccae63495d..bcf8a7994944 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -45,6 +45,87 @@ static struct fuse_dev *fuse_get_dev(struct file *file)
 	return READ_ONCE(file->private_data);
 }
 
+static bool request_expired(struct fuse_conn *fc, struct list_head *list)
+{
+	struct fuse_req *req;
+
+	req = list_first_entry_or_null(list, struct fuse_req, list);
+	if (!req)
+		return false;
+	return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout);
+}
+
+/*
+ * Check if any requests aren't being completed by the time the request timeout
+ * elapses. To do so, we:
+ * - check the fiq pending list
+ * - check the bg queue
+ * - check the fpq io and processing lists
+ *
+ * To make this fast, we only check against the head request on each list since
+ * these are generally queued in order of creation time (eg newer requests get
+ * queued to the tail). We might miss a few edge cases (eg requests transitioning
+ * between lists, re-sent requests at the head of the pending list having a
+ * later creation time than other requests on that list, etc.) but that is fine
+ * since if the request never gets fulfilled, it will eventually be caught.
+ */
+void fuse_check_timeout(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct fuse_conn *fc = container_of(dwork, struct fuse_conn,
+					    timeout.work);
+	struct fuse_iqueue *fiq = &fc->iq;
+	struct fuse_dev *fud;
+	struct fuse_pqueue *fpq;
+	bool expired = false;
+	int i;
+
+	if (!atomic_read(&fc->num_waiting))
+	    goto out;
+
+	spin_lock(&fiq->lock);
+	expired = request_expired(fc, &fiq->pending);
+	spin_unlock(&fiq->lock);
+	if (expired)
+		goto abort_conn;
+
+	spin_lock(&fc->bg_lock);
+	expired = request_expired(fc, &fc->bg_queue);
+	spin_unlock(&fc->bg_lock);
+	if (expired)
+		goto abort_conn;
+
+	spin_lock(&fc->lock);
+	if (!fc->connected) {
+		spin_unlock(&fc->lock);
+		return;
+	}
+	list_for_each_entry(fud, &fc->devices, entry) {
+		fpq = &fud->pq;
+		spin_lock(&fpq->lock);
+		if (request_expired(fc, &fpq->io))
+			goto fpq_abort;
+
+		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
+			if (request_expired(fc, &fpq->processing[i]))
+				goto fpq_abort;
+		}
+		spin_unlock(&fpq->lock);
+	}
+	spin_unlock(&fc->lock);
+
+out:
+	queue_delayed_work(system_wq, &fc->timeout.work,
+			   secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ));
+	return;
+
+fpq_abort:
+	spin_unlock(&fpq->lock);
+	spin_unlock(&fc->lock);
+abort_conn:
+	fuse_abort_conn(fc);
+}
+
 static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
 {
 	INIT_LIST_HEAD(&req->list);
@@ -53,6 +134,7 @@ 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;
+	req->create_time = jiffies;
 }
 
 static struct fuse_req *fuse_request_alloc(struct fuse_mount *fm, gfp_t flags)
@@ -2260,6 +2342,9 @@ void fuse_abort_conn(struct fuse_conn *fc)
 		LIST_HEAD(to_end);
 		unsigned int i;
 
+		if (fc->timeout.req_timeout)
+			cancel_delayed_work(&fc->timeout.work);
+
 		/* Background queuing checks fc->connected under bg_lock */
 		spin_lock(&fc->bg_lock);
 		fc->connected = 0;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 74744c6f2860..26eb00e5f043 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -438,6 +438,9 @@ struct fuse_req {
 
 	/** fuse_mount this request belongs to */
 	struct fuse_mount *fm;
+
+	/** When (in jiffies) the request was created */
+	unsigned long create_time;
 };
 
 struct fuse_iqueue;
@@ -528,6 +531,17 @@ struct fuse_pqueue {
 	struct list_head io;
 };
 
+/* Frequency (in seconds) of request timeout checks, if opted into */
+#define FUSE_TIMEOUT_TIMER_FREQ 15
+
+struct fuse_timeout {
+	/* Worker for checking if any requests have timed out */
+	struct delayed_work work;
+
+	/* Request timeout (in jiffies). 0 = no timeout */
+	unsigned long req_timeout;
+};
+
 /**
  * Fuse device instance
  */
@@ -574,6 +588,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 */
@@ -923,6 +939,9 @@ struct fuse_conn {
 	/** IDR for backing files ids */
 	struct idr backing_files_map;
 #endif
+
+	/** Only used if the connection enforces request timeouts */
+	struct fuse_timeout timeout;
 };
 
 /*
@@ -1191,6 +1210,9 @@ void fuse_request_end(struct fuse_req *req);
 void fuse_abort_conn(struct fuse_conn *fc);
 void fuse_wait_aborted(struct fuse_conn *fc);
 
+/* Check if any requests timed out */
+void fuse_check_timeout(struct work_struct *work);
+
 /**
  * Invalidate inode attributes
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 3ce4f4e81d09..02dac88d922e 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -765,6 +765,7 @@ enum {
 	OPT_ALLOW_OTHER,
 	OPT_MAX_READ,
 	OPT_BLKSIZE,
+	OPT_REQUEST_TIMEOUT,
 	OPT_ERR
 };
 
@@ -779,6 +780,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),
 	{}
 };
 
@@ -874,6 +876,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;
 	}
@@ -1004,6 +1010,8 @@ void fuse_conn_put(struct fuse_conn *fc)
 
 		if (IS_ENABLED(CONFIG_FUSE_DAX))
 			fuse_dax_conn_free(fc);
+		if (fc->timeout.req_timeout)
+			cancel_delayed_work_sync(&fc->timeout.work);
 		if (fiq->ops->release)
 			fiq->ops->release(fiq);
 		put_pid_ns(fc->pid_ns);
@@ -1723,6 +1731,20 @@ int fuse_init_fs_context_submount(struct fs_context *fsc)
 }
 EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
 
+static void fuse_init_fc_timeout(struct fuse_conn *fc, struct fuse_fs_context *ctx)
+{
+	if (ctx->req_timeout) {
+		if (check_mul_overflow(ctx->req_timeout, HZ, &fc->timeout.req_timeout))
+			fc->timeout.req_timeout = ULONG_MAX;
+
+		INIT_DELAYED_WORK(&fc->timeout.work, fuse_check_timeout);
+		queue_delayed_work(system_wq, &fc->timeout.work,
+				   secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ));
+	} else {
+		fc->timeout.req_timeout = 0;
+	}
+}
+
 int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 {
 	struct fuse_dev *fud = NULL;
@@ -1785,6 +1807,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	fc->destroy = ctx->destroy;
 	fc->no_control = ctx->no_control;
 	fc->no_force_umount = ctx->no_force_umount;
+	fuse_init_fc_timeout(fc, ctx);
 
 	err = -ENOMEM;
 	root = fuse_get_root_inode(sb, ctx->rootmode);
-- 
2.43.5

[PATCH v11 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. The upper bound on these sysctl
values is U32_MAX.

"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 specify a timeout, then
default_request_timeout will be ignored.

"max_request_timeout" sets the max amount of time the server may take to
reply to a request. 0 (default) indicates no maximum timeout. If
max_request_timeout is set and the fuse server attempts to set a
timeout greater than max_request_timeout, the system will use
max_request_timeout as the timeout. Similarly, if default_request_timeout
is greater than max_request_timeout, the system will use
max_request_timeout as the 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.

Please note that these timeouts are not 100% precise. The request may
take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond the set max
timeout due to how it's internally implemented.

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

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

$ echo 4294967295 | sudo tee /proc/sys/fs/fuse/default_request_timeout
4294967295

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

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

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

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Bernd Schubert <bschubert@ddn.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 Documentation/admin-guide/sysctl/fs.rst | 25 +++++++++++++++++++++++++
 fs/fuse/fuse_i.h                        | 10 ++++++++++
 fs/fuse/inode.c                         | 16 ++++++++++++++--
 fs/fuse/sysctl.c                        | 14 ++++++++++++++
 4 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
index f5ec6c9312e1..12169a5e19dd 100644
--- a/Documentation/admin-guide/sysctl/fs.rst
+++ b/Documentation/admin-guide/sysctl/fs.rst
@@ -347,3 +347,28 @@ filesystems:
 ``/proc/sys/fs/fuse/max_pages_limit`` is a read/write file for
 setting/getting the maximum number of pages that can be used for servicing
 requests in FUSE.
+
+``/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 no default timeout.
+The maximum value that can be set is U32_MAX.
+
+``/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 set to 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 a value
+greater than max_request_timeout, the system will use max_request_timeout as the
+timeout. 0 indicates no max request timeout. The maximum value that can be set
+is U32_MAX.
+
+For the timeouts, if the server does not respond to the request by the time
+the set timeout elapses, then the connection to the fuse server will be aborted.
+Please note that the timeouts are not 100% precise (eg you may set 60 seconds but
+the timeout may kick in after 70 seconds). The upper margin of error for the
+timeout is roughly FUSE_TIMEOUT_TIMER_FREQ seconds.
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 26eb00e5f043..310885b51087 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -46,6 +46,16 @@
 
 /** Maximum of max_pages received in init_out */
 extern unsigned int fuse_max_pages_limit;
+/*
+ * Default timeout (in seconds) for the server to reply to a request
+ * before the connection is aborted, if no timeout was specified on mount.
+ */
+extern unsigned int fuse_default_req_timeout;
+/*
+ * Max timeout (in seconds) for the server to reply to a request before
+ * the connection is aborted.
+ */
+extern unsigned int fuse_max_req_timeout;
 
 /** List of active connections */
 extern struct list_head fuse_conn_list;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 02dac88d922e..9f0be79eab74 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -36,6 +36,9 @@ DEFINE_MUTEX(fuse_mutex);
 static int set_global_limit(const char *val, const struct kernel_param *kp);
 
 unsigned int fuse_max_pages_limit = 256;
+/* default is no timeout */
+unsigned int fuse_default_req_timeout = 0;
+unsigned int fuse_max_req_timeout = 0;
 
 unsigned max_user_bgreq;
 module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
@@ -1733,8 +1736,17 @@ EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
 
 static void fuse_init_fc_timeout(struct fuse_conn *fc, struct fuse_fs_context *ctx)
 {
-	if (ctx->req_timeout) {
-		if (check_mul_overflow(ctx->req_timeout, HZ, &fc->timeout.req_timeout))
+	unsigned int timeout = ctx->req_timeout ?: fuse_default_req_timeout;
+
+	if (fuse_max_req_timeout) {
+		if (!timeout)
+			timeout = fuse_max_req_timeout;
+		else
+			timeout = min(timeout, fuse_max_req_timeout);
+	}
+
+	if (timeout) {
+		if (check_mul_overflow(timeout, HZ, &fc->timeout.req_timeout))
 			fc->timeout.req_timeout = ULONG_MAX;
 
 		INIT_DELAYED_WORK(&fc->timeout.work, fuse_check_timeout);
diff --git a/fs/fuse/sysctl.c b/fs/fuse/sysctl.c
index b272bb333005..5017059513f1 100644
--- a/fs/fuse/sysctl.c
+++ b/fs/fuse/sysctl.c
@@ -23,6 +23,20 @@ static struct ctl_table fuse_sysctl_table[] = {
 		.extra1		= SYSCTL_ONE,
 		.extra2		= &sysctl_fuse_max_pages_limit,
 	},
+	{
+		.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)
-- 
2.43.5

Re: [PATCH v11 1/2] fuse: add kernel-enforced timeout option for requests

[Jeff Layton]

On Wed, 2024-12-18 at 14:26 -0800, Joanne Koong wrote:
> There are situations where fuse servers can become unresponsive or
> stuck, for example if the server is deadlocked. 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) for
> requests where if the timeout elapses without the server responding to
> the request, the connection will be automatically aborted.
> 
> Please note that these timeouts are not 100% precise. For example, the
> request may take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond
> the requested timeout due to internal implementation, in order to
> mitigate overhead.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/fuse_i.h | 22 +++++++++++++
>  fs/fuse/inode.c  | 23 +++++++++++++
>  3 files changed, 130 insertions(+)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 27ccae63495d..bcf8a7994944 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -45,6 +45,87 @@ static struct fuse_dev *fuse_get_dev(struct file *file)
>  	return READ_ONCE(file->private_data);
>  }
>  
> +static bool request_expired(struct fuse_conn *fc, struct list_head *list)
> +{
> +	struct fuse_req *req;
> +
> +	req = list_first_entry_or_null(list, struct fuse_req, list);
> +	if (!req)
> +		return false;
> +	return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout);
> +}

Shouldn't that be time_is_after_jiffies() ? This is going to return
true when you've not yet hit the timeout, no?

> +
> +/*
> + * Check if any requests aren't being completed by the time the request timeout
> + * elapses. To do so, we:
> + * - check the fiq pending list
> + * - check the bg queue
> + * - check the fpq io and processing lists
> + *
> + * To make this fast, we only check against the head request on each list since
> + * these are generally queued in order of creation time (eg newer requests get
> + * queued to the tail). We might miss a few edge cases (eg requests transitioning
> + * between lists, re-sent requests at the head of the pending list having a
> + * later creation time than other requests on that list, etc.) but that is fine
> + * since if the request never gets fulfilled, it will eventually be caught.
> + */
> +void fuse_check_timeout(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct fuse_conn *fc = container_of(dwork, struct fuse_conn,
> +					    timeout.work);
> +	struct fuse_iqueue *fiq = &fc->iq;
> +	struct fuse_dev *fud;
> +	struct fuse_pqueue *fpq;
> +	bool expired = false;
> +	int i;
> +
> +	if (!atomic_read(&fc->num_waiting))
> +	    goto out;
> +

This is fine for an initial pass, but it might be more interesting to
not queue the workqueue job at all when there is nothing in flight.

> +	spin_lock(&fiq->lock);
> +	expired = request_expired(fc, &fiq->pending);
> +	spin_unlock(&fiq->lock);
> +	if (expired)
> +		goto abort_conn;
> +
> +	spin_lock(&fc->bg_lock);
> +	expired = request_expired(fc, &fc->bg_queue);
> +	spin_unlock(&fc->bg_lock);
> +	if (expired)
> +		goto abort_conn;
> +
> +	spin_lock(&fc->lock);
> +	if (!fc->connected) {
> +		spin_unlock(&fc->lock);
> +		return;
> +	}
> +	list_for_each_entry(fud, &fc->devices, entry) {
> +		fpq = &fud->pq;
> +		spin_lock(&fpq->lock);
> +		if (request_expired(fc, &fpq->io))
> +			goto fpq_abort;
> +
> +		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
> +			if (request_expired(fc, &fpq->processing[i]))
> +				goto fpq_abort;
> +		}
> +		spin_unlock(&fpq->lock);
> +	}
> +	spin_unlock(&fc->lock);
> +
> +out:
> +	queue_delayed_work(system_wq, &fc->timeout.work,
> +			   secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ));
> +	return;
> +
> +fpq_abort:
> +	spin_unlock(&fpq->lock);
> +	spin_unlock(&fc->lock);
> +abort_conn:
> +	fuse_abort_conn(fc);
> +}
> +
>  static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
>  {
>  	INIT_LIST_HEAD(&req->list);
> @@ -53,6 +134,7 @@ 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;
> +	req->create_time = jiffies;
>  }
>  
>  static struct fuse_req *fuse_request_alloc(struct fuse_mount *fm, gfp_t flags)
> @@ -2260,6 +2342,9 @@ void fuse_abort_conn(struct fuse_conn *fc)
>  		LIST_HEAD(to_end);
>  		unsigned int i;
>  
> +		if (fc->timeout.req_timeout)
> +			cancel_delayed_work(&fc->timeout.work);
> +
>  		/* Background queuing checks fc->connected under bg_lock */
>  		spin_lock(&fc->bg_lock);
>  		fc->connected = 0;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 74744c6f2860..26eb00e5f043 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -438,6 +438,9 @@ struct fuse_req {
>  
>  	/** fuse_mount this request belongs to */
>  	struct fuse_mount *fm;
> +
> +	/** When (in jiffies) the request was created */
> +	unsigned long create_time;
>  };
>  
>  struct fuse_iqueue;
> @@ -528,6 +531,17 @@ struct fuse_pqueue {
>  	struct list_head io;
>  };
>  
> +/* Frequency (in seconds) of request timeout checks, if opted into */
> +#define FUSE_TIMEOUT_TIMER_FREQ 15
> +
> +struct fuse_timeout {
> +	/* Worker for checking if any requests have timed out */
> +	struct delayed_work work;
> +
> +	/* Request timeout (in jiffies). 0 = no timeout */
> +	unsigned long req_timeout;
> +};
> +
>  /**
>   * Fuse device instance
>   */
> @@ -574,6 +588,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 */
> @@ -923,6 +939,9 @@ struct fuse_conn {
>  	/** IDR for backing files ids */
>  	struct idr backing_files_map;
>  #endif
> +
> +	/** Only used if the connection enforces request timeouts */
> +	struct fuse_timeout timeout;
>  };
>  
>  /*
> @@ -1191,6 +1210,9 @@ void fuse_request_end(struct fuse_req *req);
>  void fuse_abort_conn(struct fuse_conn *fc);
>  void fuse_wait_aborted(struct fuse_conn *fc);
>  
> +/* Check if any requests timed out */
> +void fuse_check_timeout(struct work_struct *work);
> +
>  /**
>   * Invalidate inode attributes
>   */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 3ce4f4e81d09..02dac88d922e 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -765,6 +765,7 @@ enum {
>  	OPT_ALLOW_OTHER,
>  	OPT_MAX_READ,
>  	OPT_BLKSIZE,
> +	OPT_REQUEST_TIMEOUT,
>  	OPT_ERR
>  };
>  
> @@ -779,6 +780,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),
>  	{}
>  };
>  
> @@ -874,6 +876,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;
>  	}
> @@ -1004,6 +1010,8 @@ void fuse_conn_put(struct fuse_conn *fc)
>  
>  		if (IS_ENABLED(CONFIG_FUSE_DAX))
>  			fuse_dax_conn_free(fc);
> +		if (fc->timeout.req_timeout)
> +			cancel_delayed_work_sync(&fc->timeout.work);
>  		if (fiq->ops->release)
>  			fiq->ops->release(fiq);
>  		put_pid_ns(fc->pid_ns);
> @@ -1723,6 +1731,20 @@ int fuse_init_fs_context_submount(struct fs_context *fsc)
>  }
>  EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
>  
> +static void fuse_init_fc_timeout(struct fuse_conn *fc, struct fuse_fs_context *ctx)
> +{
> +	if (ctx->req_timeout) {
> +		if (check_mul_overflow(ctx->req_timeout, HZ, &fc->timeout.req_timeout))
> +			fc->timeout.req_timeout = ULONG_MAX;
> +
> +		INIT_DELAYED_WORK(&fc->timeout.work, fuse_check_timeout);
> +		queue_delayed_work(system_wq, &fc->timeout.work,
> +				   secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ));
> +	} else {
> +		fc->timeout.req_timeout = 0;
> +	}
> +}
> +
>  int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  {
>  	struct fuse_dev *fud = NULL;
> @@ -1785,6 +1807,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	fc->destroy = ctx->destroy;
>  	fc->no_control = ctx->no_control;
>  	fc->no_force_umount = ctx->no_force_umount;
> +	fuse_init_fc_timeout(fc, ctx);
>  
>  	err = -ENOMEM;
>  	root = fuse_get_root_inode(sb, ctx->rootmode);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v11 1/2] fuse: add kernel-enforced timeout option for requests

[Jeff Layton]

On Thu, 2024-12-19 at 07:19 -0500, Jeff Layton wrote:
> On Wed, 2024-12-18 at 14:26 -0800, Joanne Koong wrote:
> > There are situations where fuse servers can become unresponsive or
> > stuck, for example if the server is deadlocked. 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) for
> > requests where if the timeout elapses without the server responding to
> > the request, the connection will be automatically aborted.
> > 
> > Please note that these timeouts are not 100% precise. For example, the
> > request may take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond
> > the requested timeout due to internal implementation, in order to
> > mitigate overhead.
> > 
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/dev.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/fuse/fuse_i.h | 22 +++++++++++++
> >  fs/fuse/inode.c  | 23 +++++++++++++
> >  3 files changed, 130 insertions(+)
> > 
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 27ccae63495d..bcf8a7994944 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -45,6 +45,87 @@ static struct fuse_dev *fuse_get_dev(struct file *file)
> >  	return READ_ONCE(file->private_data);
> >  }
> >  
> > +static bool request_expired(struct fuse_conn *fc, struct list_head *list)
> > +{
> > +	struct fuse_req *req;
> > +
> > +	req = list_first_entry_or_null(list, struct fuse_req, list);
> > +	if (!req)
> > +		return false;
> > +	return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout);
> > +}
> 
> Shouldn't that be time_is_after_jiffies() ? This is going to return
> true when you've not yet hit the timeout, no?
> 

My mistake, I misinterpreted the time_is_before_jiffies() macro. You
can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v11 1/2] fuse: add kernel-enforced timeout option for requests

[Etienne Martineau]

On Wed, Dec 18, 2024 at 5:27 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> There are situations where fuse servers can become unresponsive or
> stuck, for example if the server is deadlocked. 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) for
> requests where if the timeout elapses without the server responding to
> the request, the connection will be automatically aborted.
>
> Please note that these timeouts are not 100% precise. For example, the
> request may take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond
> the requested timeout due to internal implementation, in order to
> mitigate overhead.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

This solution seems to be holding fine with my TC. However, personally
as a non-fuse person, I feel that it's hard to judge if the check
timeout logic is doing the right thing in every scenario and more
specifically for my use-case where I'm going to backport that logic to
older stable branches.

So for that reason I've been trying to figure out a simpler solution
and I stumbled on something last night which is giving good results so
far. Just a basic time distribution sliding window. Very
straightforward, no list, no timer scale issue and low overhead.
Sending that patch as a RFC to see if we can leverage that trick
maybe... OR something along those lines?

https://lore.kernel.org/linux-fsdevel/20241219204149.11958-2-etmartin4313@gmail.com/T/#mcfa362bf41860e151177a2aa49eee8a141324477

PS I don't want to put a monkey wrench into this series and again this
solution is holding fine for me. I'm just looking for something more
straightforward if possible.

thanks,
Etienne



>  fs/fuse/dev.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/fuse_i.h | 22 +++++++++++++
>  fs/fuse/inode.c  | 23 +++++++++++++
>  3 files changed, 130 insertions(+)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 27ccae63495d..bcf8a7994944 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -45,6 +45,87 @@ static struct fuse_dev *fuse_get_dev(struct file *file)
>         return READ_ONCE(file->private_data);
>  }
>
> +static bool request_expired(struct fuse_conn *fc, struct list_head *list)
> +{
> +       struct fuse_req *req;
> +
> +       req = list_first_entry_or_null(list, struct fuse_req, list);
> +       if (!req)
> +               return false;
> +       return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout);
> +}
> +
> +/*
> + * Check if any requests aren't being completed by the time the request timeout
> + * elapses. To do so, we:
> + * - check the fiq pending list
> + * - check the bg queue
> + * - check the fpq io and processing lists
> + *
> + * To make this fast, we only check against the head request on each list since
> + * these are generally queued in order of creation time (eg newer requests get
> + * queued to the tail). We might miss a few edge cases (eg requests transitioning
> + * between lists, re-sent requests at the head of the pending list having a
> + * later creation time than other requests on that list, etc.) but that is fine
> + * since if the request never gets fulfilled, it will eventually be caught.
> + */
> +void fuse_check_timeout(struct work_struct *work)
> +{
> +       struct delayed_work *dwork = to_delayed_work(work);
> +       struct fuse_conn *fc = container_of(dwork, struct fuse_conn,
> +                                           timeout.work);
> +       struct fuse_iqueue *fiq = &fc->iq;
> +       struct fuse_dev *fud;
> +       struct fuse_pqueue *fpq;
> +       bool expired = false;
> +       int i;
> +
> +       if (!atomic_read(&fc->num_waiting))
> +           goto out;
> +
> +       spin_lock(&fiq->lock);
> +       expired = request_expired(fc, &fiq->pending);
> +       spin_unlock(&fiq->lock);
> +       if (expired)
> +               goto abort_conn;
> +
> +       spin_lock(&fc->bg_lock);
> +       expired = request_expired(fc, &fc->bg_queue);
> +       spin_unlock(&fc->bg_lock);
> +       if (expired)
> +               goto abort_conn;
> +
> +       spin_lock(&fc->lock);
> +       if (!fc->connected) {
> +               spin_unlock(&fc->lock);
> +               return;
> +       }
> +       list_for_each_entry(fud, &fc->devices, entry) {
> +               fpq = &fud->pq;
> +               spin_lock(&fpq->lock);
> +               if (request_expired(fc, &fpq->io))
> +                       goto fpq_abort;
> +
> +               for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
> +                       if (request_expired(fc, &fpq->processing[i]))
> +                               goto fpq_abort;
> +               }
> +               spin_unlock(&fpq->lock);
> +       }
> +       spin_unlock(&fc->lock);
> +
> +out:
> +       queue_delayed_work(system_wq, &fc->timeout.work,
> +                          secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ));
> +       return;
> +
> +fpq_abort:
> +       spin_unlock(&fpq->lock);
> +       spin_unlock(&fc->lock);
> +abort_conn:
> +       fuse_abort_conn(fc);
> +}
> +
>  static void fuse_request_init(struct fuse_mount *fm, struct fuse_req *req)
>  {
>         INIT_LIST_HEAD(&req->list);
> @@ -53,6 +134,7 @@ 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;
> +       req->create_time = jiffies;
>  }
>
>  static struct fuse_req *fuse_request_alloc(struct fuse_mount *fm, gfp_t flags)
> @@ -2260,6 +2342,9 @@ void fuse_abort_conn(struct fuse_conn *fc)
>                 LIST_HEAD(to_end);
>                 unsigned int i;
>
> +               if (fc->timeout.req_timeout)
> +                       cancel_delayed_work(&fc->timeout.work);
> +
>                 /* Background queuing checks fc->connected under bg_lock */
>                 spin_lock(&fc->bg_lock);
>                 fc->connected = 0;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 74744c6f2860..26eb00e5f043 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -438,6 +438,9 @@ struct fuse_req {
>
>         /** fuse_mount this request belongs to */
>         struct fuse_mount *fm;
> +
> +       /** When (in jiffies) the request was created */
> +       unsigned long create_time;
>  };
>
>  struct fuse_iqueue;
> @@ -528,6 +531,17 @@ struct fuse_pqueue {
>         struct list_head io;
>  };
>
> +/* Frequency (in seconds) of request timeout checks, if opted into */
> +#define FUSE_TIMEOUT_TIMER_FREQ 15
> +
> +struct fuse_timeout {
> +       /* Worker for checking if any requests have timed out */
> +       struct delayed_work work;
> +
> +       /* Request timeout (in jiffies). 0 = no timeout */
> +       unsigned long req_timeout;
> +};
> +
>  /**
>   * Fuse device instance
>   */
> @@ -574,6 +588,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 */
> @@ -923,6 +939,9 @@ struct fuse_conn {
>         /** IDR for backing files ids */
>         struct idr backing_files_map;
>  #endif
> +
> +       /** Only used if the connection enforces request timeouts */
> +       struct fuse_timeout timeout;
>  };
>
>  /*
> @@ -1191,6 +1210,9 @@ void fuse_request_end(struct fuse_req *req);
>  void fuse_abort_conn(struct fuse_conn *fc);
>  void fuse_wait_aborted(struct fuse_conn *fc);
>
> +/* Check if any requests timed out */
> +void fuse_check_timeout(struct work_struct *work);
> +
>  /**
>   * Invalidate inode attributes
>   */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 3ce4f4e81d09..02dac88d922e 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -765,6 +765,7 @@ enum {
>         OPT_ALLOW_OTHER,
>         OPT_MAX_READ,
>         OPT_BLKSIZE,
> +       OPT_REQUEST_TIMEOUT,
>         OPT_ERR
>  };
>
> @@ -779,6 +780,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),
>         {}
>  };
>
> @@ -874,6 +876,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;
>         }
> @@ -1004,6 +1010,8 @@ void fuse_conn_put(struct fuse_conn *fc)
>
>                 if (IS_ENABLED(CONFIG_FUSE_DAX))
>                         fuse_dax_conn_free(fc);
> +               if (fc->timeout.req_timeout)
> +                       cancel_delayed_work_sync(&fc->timeout.work);
>                 if (fiq->ops->release)
>                         fiq->ops->release(fiq);
>                 put_pid_ns(fc->pid_ns);
> @@ -1723,6 +1731,20 @@ int fuse_init_fs_context_submount(struct fs_context *fsc)
>  }
>  EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
>
> +static void fuse_init_fc_timeout(struct fuse_conn *fc, struct fuse_fs_context *ctx)
> +{
> +       if (ctx->req_timeout) {
> +               if (check_mul_overflow(ctx->req_timeout, HZ, &fc->timeout.req_timeout))
> +                       fc->timeout.req_timeout = ULONG_MAX;
> +
> +               INIT_DELAYED_WORK(&fc->timeout.work, fuse_check_timeout);
> +               queue_delayed_work(system_wq, &fc->timeout.work,
> +                                  secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ));
> +       } else {
> +               fc->timeout.req_timeout = 0;
> +       }
> +}
> +
>  int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  {
>         struct fuse_dev *fud = NULL;
> @@ -1785,6 +1807,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>         fc->destroy = ctx->destroy;
>         fc->no_control = ctx->no_control;
>         fc->no_force_umount = ctx->no_force_umount;
> +       fuse_init_fc_timeout(fc, ctx);
>
>         err = -ENOMEM;
>         root = fuse_get_root_inode(sb, ctx->rootmode);
> --
> 2.43.5
>

Re: [PATCH v11 1/2] fuse: add kernel-enforced timeout option for requests

[Sergey Senozhatsky]

On (24/12/18 14:26), Joanne Koong wrote:
> There are situations where fuse servers can become unresponsive or
> stuck, for example if the server is deadlocked. 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) for
> requests where if the timeout elapses without the server responding to
> the request, the connection will be automatically aborted.
> 
> Please note that these timeouts are not 100% precise. For example, the
> request may take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond
> the requested timeout due to internal implementation, in order to
> mitigate overhead.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

FWIW
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

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

[Sergey Senozhatsky]

On (24/12/18 14:26), Joanne Koong wrote:
> 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. The upper bound on these sysctl
> values is U32_MAX.
> 
> "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 specify a timeout, then
> default_request_timeout will be ignored.
> 
> "max_request_timeout" sets the max amount of time the server may take to
> reply to a request. 0 (default) indicates no maximum timeout. If
> max_request_timeout is set and the fuse server attempts to set a
> timeout greater than max_request_timeout, the system will use
> max_request_timeout as the timeout. Similarly, if default_request_timeout
> is greater than max_request_timeout, the system will use
> max_request_timeout as the 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.
> 
> Please note that these timeouts are not 100% precise. The request may
> take roughly an extra FUSE_TIMEOUT_TIMER_FREQ seconds beyond the set max
> timeout due to how it's internally implemented.
> 
> $ sysctl -a | grep fuse.default_request_timeout
> fs.fuse.default_request_timeout = 0
> 
> $ echo 4294967296 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> tee: /proc/sys/fs/fuse/default_request_timeout: Invalid argument
> 
> $ echo 4294967295 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> 4294967295
> 
> $ sysctl -a | grep fuse.default_request_timeout
> fs.fuse.default_request_timeout = 4294967295
> 
> $ echo 0 | sudo tee /proc/sys/fs/fuse/default_request_timeout
> 0
> 
> $ sysctl -a | grep fuse.default_request_timeout
> fs.fuse.default_request_timeout = 0
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

FWIW
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[Joanne Koong]

On Wed, Dec 18, 2024 at 2:27 PM 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 where if the server does not reply to a
> 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 systems running fuse servers will not be affected unless they
> explicitly opt into the timeout.

Miklos, is this patchset acceptable for your tree?

Thanks,
Joanne


>
> v10:
> https://lore.kernel.org/linux-fsdevel/20241214022827.1773071-1-joannelkoong@gmail.com/
> Changes from v10 -> v11:
> * Refactor check for request expiration (Sergey)
> * Move workqueue cancellation to earlier in function (Jeff)
> * Check fc->num_waiting as a shortcut in workqueue job (Etienne)
>
> v9:
> https://lore.kernel.org/linux-fsdevel/20241114191332.669127-1-joannelkoong@gmail.com/
> Changes from v9 -> v10:
> * Use delayed workqueues instead of timers (Sergey and Jeff)
> * Change granularity to seconds instead of minutes (Sergey and Jeff)
> * Use time_after() api for checking jiffies expiration (Sergey)
> * Change timer check to run every 15 secs instead of every min
> * Update documentation wording to be more clear
>
> v8:
> https://lore.kernel.org/linux-fsdevel/20241011191320.91592-1-joannelkoong@gmail.com/
> Changes from v8 -> v9:
> * Fix comment for u16 fs_parse_result, ULONG_MAX instead of U32_MAX, fix
>   spacing (Bernd)
>
> v7:
> https://lore.kernel.org/linux-fsdevel/20241007184258.2837492-1-joannelkoong@gmail.com/
> Changes from v7 -> v8:
> * Use existing lists for checking expirations (Miklos)
>
> v6:
> https://lore.kernel.org/linux-fsdevel/20240830162649.3849586-1-joannelkoong@gmail.com/
> Changes from v6 -> v7:
> - Make timer per-connection instead of per-request (Miklos)
> - Make default granularity of time minutes instead of seconds
> - Removed the reviewed-bys since the interface of this has changed (now
>   minutes, instead of seconds)
>
> v5:
> https://lore.kernel.org/linux-fsdevel/20240826203234.4079338-1-joannelkoong@gmail.com/
> Changes from v5 -> v6:
> - Gate sysctl.o behind CONFIG_SYSCTL in makefile (kernel test robot)
> - Reword/clarify last sentence in cover letter (Miklos)
>
> 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 kernel-enforced timeout option for requests
>   fuse: add default_request_timeout and max_request_timeout sysctls
>
>  Documentation/admin-guide/sysctl/fs.rst | 25 ++++++++
>  fs/fuse/dev.c                           | 85 +++++++++++++++++++++++++
>  fs/fuse/fuse_i.h                        | 32 ++++++++++
>  fs/fuse/inode.c                         | 35 ++++++++++
>  fs/fuse/sysctl.c                        | 14 ++++
>  5 files changed, 191 insertions(+)
>
> --
> 2.43.5
>

Re: [PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[Miklos Szeredi]

On Wed, 15 Jan 2025 at 20:41, Joanne Koong <joannelkoong@gmail.com> wrote:

> Miklos, is this patchset acceptable for your tree?

Looks good generally.

I wonder why you chose to use a mount option instead of an FUSE_INIT param?

Nowadays the new mount API allows feature negotiation (i.e. it's
possible to check whether an option is supported by the kernel or
not), just like FUSE_INIT, so the two interfaces are more or less
equivalent.  But we've not added mount options to fuse for a long
time...

Thanks,
Miklos

Re: [PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[Joanne Koong]

On Thu, Jan 16, 2025 at 4:11 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 15 Jan 2025 at 20:41, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> > Miklos, is this patchset acceptable for your tree?
>
> Looks good generally.
>
> I wonder why you chose to use a mount option instead of an FUSE_INIT param?

I think it was because I had wanted to cover the case where the init
request itself times out.

Maybe that's overkill and not worth worrying about. Interface wise, I
definitely prefer passing it through init as that's much simpler for
users than having to snprintf it to fuse_opt.

I'll push out v12 and change this to go through FUSE_INIT instead of
as a mount option.

Thanks,
Joanne

>
> Nowadays the new mount API allows feature negotiation (i.e. it's
> possible to check whether an option is supported by the kernel or
> not), just like FUSE_INIT, so the two interfaces are more or less
> equivalent.  But we've not added mount options to fuse for a long
> time...

I

>
> Thanks,
> Miklos

Re: [PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[Jingbo Xu]

On 1/16/25 8:11 PM, Miklos Szeredi wrote:
> On Wed, 15 Jan 2025 at 20:41, Joanne Koong <joannelkoong@gmail.com> wrote:
> 
>> Miklos, is this patchset acceptable for your tree?
> 
> Looks good generally.
> 
> I wonder why you chose to use a mount option instead of an FUSE_INIT param?

IMO this timeout mechanism has no dependence on the implementation on
the server side, and it's self-contained by the kernel side.  Thus it's
adequate to negotiate through the mount option instead of the INIT
feature negotiation.  Although the FUSE mount instance is generally
mounted by the fuse server itself in the libfuse implementation.  IOW
INIT feature negotiation is required only when the server side shall opt in.

-- 
Thanks,
Jingbo

Re: [PATCH v11 0/2] fuse: add kernel-enforced request timeout option

[Joanne Koong]

On Thu, Jan 16, 2025 at 5:41 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
>
>
> On 1/16/25 8:11 PM, Miklos Szeredi wrote:
> > On Wed, 15 Jan 2025 at 20:41, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> >> Miklos, is this patchset acceptable for your tree?
> >
> > Looks good generally.
> >
> > I wonder why you chose to use a mount option instead of an FUSE_INIT param?
>
> IMO this timeout mechanism has no dependence on the implementation on
> the server side, and it's self-contained by the kernel side.  Thus it's
> adequate to negotiate through the mount option instead of the INIT
> feature negotiation.  Although the FUSE mount instance is generally
> mounted by the fuse server itself in the libfuse implementation.  IOW
> INIT feature negotiation is required only when the server side shall opt in.

My understanding (maybe wrong) is that fuse servers only get mounted
through the libfuse interface (eg fuse_session_mount()) which is
directly called from the server code, so with either the mount option
flag or INIT, both would require server-side changes. Am I wrong in
this understanding? Is there a way to mount it through the command
line? I know the fusermount3 utility exists, but AFAIU, that only
supports unmounting.

Thanks,
Joanne
>
> --
> Thanks,
> Jingbo