On Tue, Dec 17, 2024 at 3:37 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > On Tue, Dec 17, 2024 at 12:02 PM Etienne Martineau > <etmartin4313@xxxxxxxxx> wrote: > > > > On Mon, Dec 16, 2024 at 8:26 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > > > > > On Mon, Dec 16, 2024 at 2:09 PM Etienne Martineau > > > <etmartin4313@xxxxxxxxx> wrote: > > > > > > > > On Mon, Dec 16, 2024 at 1:21 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > > > > > > > > > On Mon, Dec 16, 2024 at 9:51 AM Etienne Martineau > > > > > <etmartin4313@xxxxxxxxx> wrote: > > > > > > > > > > > > On Mon, Dec 16, 2024 at 12:32 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > > > > > > > > > > > > > On Sat, Dec 14, 2024 at 4:10 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > On Fri, 2024-12-13 at 18:28 -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@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 > > > > > > > > > > > > > > > > > > static struct fuse_req *fuse_request_alloc(struct fuse_mount *fm, gfp_t flags) > > > > > > > > > @@ -2308,6 +2388,9 @@ void fuse_abort_conn(struct fuse_conn *fc) > > > > > > > > > spin_unlock(&fc->lock); > > > > > > > > > > > > > > > > > > end_requests(&to_end); > > > > > > > > > + > > > > > > > > > + if (fc->timeout.req_timeout) > > > > > > > > > + cancel_delayed_work(&fc->timeout.work); > > > > > > > > > > > > > > > > As Sergey pointed out, this should be a cancel_delayed_work_sync(). The > > > > > > > > workqueue job can still be running after cancel_delayed_work(), and > > > > > > > > since it requeues itself, this might not be enough to kill it > > > > > > > > completely. > > > > > > > > > > > > > > I don't think we need to synchronously cancel it when a connection is > > > > > > > aborted. The fuse_check_timeout() workqueue job can be simultaneously > > > > > > > running when cancel_delayed_work() is called and can requeue itself, > > > > > > > but then on the next trigger of the job, it will check whether the > > > > > > > connection was aborted (eg the if (!fc->connected)... return; lines in > > > > > > > fuse_check_timeout()) and will not requeue itself if the connection > > > > > > > was aborted. This seemed like the simplest / cleanest approach to me. > > > > > > > > > > > > > Is there a scenario where the next trigger of the job dereference > > > > > > struct fuse_conn *fc which already got freed because say the FUSE > > > > > > server has terminated? > > > > > > > > > > This isn't possible because the struct fuse_conn *fc gets freed only > > > > > after the call to "cancel_delayed_work_sync(&fc->timeout.work);" that > > > > > synchronously cancels the workqueue job. This happens in the > > > > > fuse_conn_put() function. > > > > > > > > > cancel_delayed_work_sync() won't prevent the work from re-queuing > > > > itself if it's already running. > > > > I think we need some flag like Sergey pointed out here > > > > https://lore.kernel.org/linux-fsdevel/CAMHPp_S2ANAguT6fYfNcXjTZxU14nh2Zv=5=8dG8qUnD3F8e7A@xxxxxxxxxxxxxx/T/#m543550031f31a9210996ccf815d5bc2a4290f540 > > > > Maybe we don't requeue when fc->count becomes 0? > > > > > > The connection will have been aborted when cancel_delayed_work_sync() > > > is called (otherwise we will have a lot of memory crashes/leaks). If > > > the fuse_check_timeout() workqueue job is running while > > > cancel_delayed_work_sync() is called, there's the "if (!fc->connected) > > > { ... return; }" path that returns and avoids requeueing. > > > > > I ran some tests and from what I see, calling > > cancel_delayed_work_sync() on a workqueue that is currently running > > and re-queueing itself is enough to kill it completely. For that > > reason I believe we don't even need the cancel_delayed_work() in > > fuse_abort_conn() because everything is taken care of by > > fuse_conn_put(); > > I think the cancel_delayed_work() in fuse_abort_conn() would still be > good to have. There are some instances where the connection gets > aborted but the connection doesn't get freed (eg user forgets to > unmount the fuse filesystem or the unmount only happens a lot later). > When the connection is aborted however, this will automatically cancel > the workqueue job on the next run (on the next run, the job won't > requeue itself if it sees that the connection was aborted) so we > technically don't need the cancel_delayed_work() because of this, but > imo it'd be good to minimize the number of workqueue jobs that get run > and canceling it asap is preferable. > Ok, it makes sense. Also in fuse_check_timeout() does it make sense to leverage fc->num_waiting to save some cycle in the function? Something like: diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index e97ba860ffcd..344af61124f4 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -97,6 +97,10 @@ void fuse_check_timeout(struct work_struct *work) spin_unlock(&fc->lock); return; } + if (!fc->num_waiting){ + spin_unlock(&fc->lock); + goto out; + } list_for_each_entry(fud, &fc->devices, entry) { fpq = &fud->pq; spin_lock(&fpq->lock); @@ -113,6 +117,7 @@ void fuse_check_timeout(struct work_struct *work) } spin_unlock(&fc->lock); +out: queue_delayed_work(system_wq, &fc->timeout.work, secs_to_jiffies(FUSE_TIMEOUT_TIMER_FREQ)); return; thanks Etienne > > Thanks, > Joanne >
