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. Thanks, Joanne > thanks, > Etienne > > > > > Thanks, > > Joanne
