Re: [PATCH v10 1/2] fuse: add kernel-enforced timeout option for requests

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>