On Mon, 3 Feb 2025 at 20:57, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > Add a new file to the "connections" directory that shows how long (in > seconds) the oldest fuse_req in the processing hash or pending queue has > been waiting. > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > --- > This is based on top of Joanne's timeout patches, as it requires the > "create_time" field in fuse_req. We have some internal detection of > hung fuse server processes that relies on seeing elevated values in the > "waiting" sysfs file. The problem with that method is that it can't > detect when highly serialized workloads on a FUSE mount are hung. This > adds another metric that we can use to detect this situation. > --- > Changes in v2: > - use list_first_entry_or_null() when checking hash lists > - take fiq->lock when checking pending list > - ensure that if there are no waiting reqs, that the output will be 0 > - use time_before() to compare jiffies values > - no need to hold fc->lock when walking pending queue > - Link to v1: https://lore.kernel.org/r/20250203-fuse-sysfs-v1-1-36faa01f2338@xxxxxxxxxx > --- > fs/fuse/control.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > fs/fuse/fuse_i.h | 2 +- > 2 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/control.c b/fs/fuse/control.c > index 2a730d88cc3bdb50ea1f8a3185faad5f05fc6e74..b27f2120499826040af77d7662d2dad0e9f37ee6 100644 > --- a/fs/fuse/control.c > +++ b/fs/fuse/control.c > @@ -180,6 +180,57 @@ static ssize_t fuse_conn_congestion_threshold_write(struct file *file, > return ret; > } > > +/* Show how long (in s) the oldest request has been waiting */ > +static ssize_t fuse_conn_oldest_read(struct file *file, char __user *buf, > + size_t len, loff_t *ppos) > +{ > + char tmp[32]; > + size_t size; > + unsigned long now = jiffies; > + unsigned long oldest = now; > + > + if (!*ppos) { > + struct fuse_conn *fc = fuse_ctl_file_conn_get(file); > + struct fuse_iqueue *fiq = &fc->iq; > + struct fuse_dev *fud; > + struct fuse_req *req; > + > + if (!fc) > + return 0; > + > + spin_lock(&fc->lock); > + list_for_each_entry(fud, &fc->devices, entry) { > + struct fuse_pqueue *fpq = &fud->pq; > + int i; > + > + spin_lock(&fpq->lock); > + for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) { > + /* > + * Only check the first request in the queue. The > + * assumption is that the one at the head of the list > + * will always be the oldest. > + */ > + req = list_first_entry_or_null(&fpq->processing[i], > + struct fuse_req, list); > + if (req && time_before(req->create_time, oldest)) > + oldest = req->create_time; Couldn't this be merged with the timeout expiry code? I.e. implement get_oldest_req_time() helper, the result of which could be compared against req_timeout. > + } > + spin_unlock(&fpq->lock); > + } > + spin_unlock(&fc->lock); > + > + spin_lock(&fiq->lock); > + req = list_first_entry_or_null(&fiq->pending, struct fuse_req, list); > + if (req && time_before(req->create_time, oldest)) > + oldest = req->create_time; > + spin_unlock(&fiq->lock); > + > + fuse_conn_put(fc); > + } > + size = sprintf(tmp, "%ld\n", (now - oldest)/HZ); now - oldest will always be zero if *ppos != 0. It would be much more logical to return an error for *ppos != 0, then to return success with a nonsense value. use_conn_limit_write() already does so, and existing read callbacks could be changed to do the same with a very slight risk of a regression. But for a new one, I don't think there's any worries. Thanks, Miklos Thanks, Miklos
