On Sun, 2024-12-15 at 17:25 +0900, Sergey Senozhatsky wrote:
> On (24/12/14 07:09), Jeff Layton wrote:
> > > +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;
> > > +
> [..]
> > > +
> > > +fpq_abort:
> > > + spin_unlock(&fpq->lock);
> > > + spin_unlock(&fc->lock);
> > > +abort_conn:
> > > + fuse_abort_conn(fc);
> > > +}
> > > +
> > > @@ -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().
>
> My worry here is that fuse_abort_conn() can also be called from the
> deferred work handler, I'm not sure if we can cancel_delayed_work_sync()
> from within the same WQ context, sounds deadlock-ish:
>
> WQ -> fuse_check_timeout() -> fuse_abort_conn() -> cancel_delayed_work_sync()
>
> When fuse_abort_conn() is called from somewhere else (umount, etc.) then
> we can safely sync(), but fuse_check_timeout() is different.
>
Very good point.
> Maybe fuse_abort_conn() can become __fuse_abort_conn(), which
> fuse_check_timeout() will call directly, for the rest fuse_abort_conn()
> can be something like:
>
> static void __fuse_abort_conn()
> {
> ....
> }
>
> void fuse_abort_conn()
> {
> cancel_delayed_work_sync()
> __fuse_abort_conn();
> }
That seems like a reasonable solution. It already doesn't requeue the
job when calling fuse_abort_conn(), so that should work.
--
Jeff Layton <jlayton@xxxxxxxxxx>
