On Sun, Dec 15, 2024 at 6:35 PM Etienne Martineau
<etmartin4313@xxxxxxxxx> wrote:
>
> On Fri, Dec 13, 2024 at 9:29 PM Joanne Koong <joannelkoong@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > fs/fuse/dev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
> > fs/fuse/fuse_i.h | 22 +++++++++++++
> > fs/fuse/inode.c | 23 ++++++++++++++
> > 3 files changed, 128 insertions(+)
> >
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 27ccae63495d..e97ba860ffcd 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -45,6 +45,85 @@ 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 fuse_req *req)
> > +{
> > + 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_req *req;
> > + struct fuse_dev *fud;
> > + struct fuse_pqueue *fpq;
> > + bool expired = false;
> > + int i;
> > +
> > + spin_lock(&fiq->lock);
> > + req = list_first_entry_or_null(&fiq->pending, struct fuse_req, list);
> > + if (req)
> > + expired = request_expired(fc, req);
> > + spin_unlock(&fiq->lock);
> > + if (expired)
> > + goto abort_conn;
> > +
> > + spin_lock(&fc->bg_lock);
> > + req = list_first_entry_or_null(&fc->bg_queue, struct fuse_req, list);
> > + if (req)
> > + expired = request_expired(fc, req);
> > + 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);
>
> Can fuse_dev_release() run concurrently to this path here?
> If yes say fuse_dev_release() comes in first, grab the fpq->lock and
> splice the
> fpq->processing[i] list into &to_end and release the fpq->lock which
> unblock this
> path.
>
> Then here we start checking req off the fpq->processing[i] list which is
> getting evicted on the other side by fuse_dev_release->end_requests(&to_end);
>
> Maybe we need a cancel_delayed_work_sync() at the beginning of
> fuse_dev_release ?
Yes, fuse_dev_release() can run concurrently to this path here. If
fuse_dev_release() comes in first, grabs the fpq->lock and splices the
fpq->processing[i] lists into &to_end, then releases the fpq->lock,
and then this fuse_check_timeout() grabs the fpq->lock, it'll see no
requests on the fpq->processing[i] lists. When the requests are
spliced onto the to_end list in fuse_dev_release(), they are removed
from the &fpq->processing[i] list.
For that reason I don't think we need a cancel_delayed_work_sync() at
the beginning of fuse_dev_release(), but also a connection can have
multiple devs associated with it and the workqueue job is
per-connection and not per-device.
Thanks,
Joanne
> Thanks
> Etienne
>
> > + req = list_first_entry_or_null(&fpq->io, struct fuse_req, list);
> > + if (req && request_expired(fc, req))
> > + goto fpq_abort;
> > +
> > + for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
> > + req = list_first_entry_or_null(&fpq->processing[i], struct fuse_req, list);
> > + if (req && request_expired(fc, req))
> > + goto fpq_abort;
> > + }
> > + spin_unlock(&fpq->lock);
> > + }
> > + spin_unlock(&fc->lock);
> > +
> > + 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);
> > +}
> > +
