On Sun, Dec 15, 2024 at 7:08 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> 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>
I'm not sure this is going to work either.
What happens if say fuse_check_timeout() is running and is about to
requeue the work and
at the same time umount->fuse_abort_conn->cancel_delayed_work_sync() comes.
The cancel will correctly wait for the actual work to finish but won't
prevent it from getting
queued again no?
thanks,
Etienne
