Re: [PATCH 06/13] fuse: Add an interval ring stop worker/monitor

From: Bernd Schubert <bschubert@ddn.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Dharmendra Singh <dsingh@ddn.com>,
	Amir Goldstein <amir73il@gmail.com>,
	"fuse-devel@lists.sourceforge.net"
	<fuse-devel@lists.sourceforge.net>,
	Ming Lei <ming.lei@redhat.com>,
	Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Subject: Re: [PATCH 06/13] fuse: Add an interval ring stop worker/monitor
Date: Thu, 23 Mar 2023 13:18:21 +0000	[thread overview]
Message-ID: <e0febe95-6d35-636d-1668-84ef16b87370@ddn.com> (raw)
In-Reply-To: <CAJfpeguvCNUEbcy6VQzVJeNOsnNqfDS=LyRaGvSiDTGerB+iuw@mail.gmail.com>

On 3/23/23 13:35, Miklos Szeredi wrote:
> On Thu, 23 Mar 2023 at 12:04, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> Thanks for looking at these patches!
>>
>> I'm adding in Ming Lei, as I had taken several ideas from ublkm I guess
>> I also should also explain in the commit messages and code why it is
>> done that way.
>>
>> On 3/23/23 11:27, Miklos Szeredi wrote:
>>> On Tue, 21 Mar 2023 at 02:11, Bernd Schubert <bschubert@ddn.com> wrote:
>>>>
>>>> This adds a delayed work queue that runs in intervals
>>>> to check and to stop the ring if needed. Fuse connection
>>>> abort now waits for this worker to complete.
>>>
>>> This seems like a hack.   Can you explain what the problem is?
>>>
>>> The first thing I notice is that you store a reference to the task
>>> that initiated the ring creation.  This already looks fishy, as the
>>> ring could well survive the task (thread) that created it, no?
>>
>> You mean the currently ongoing work, where the daemon can be restarted?
>> Daemon restart will need some work with ring communication, I will take
>> care of that once we have agreed on an approach. [Also added in Alexsandre].
>>
>> fuse_uring_stop_mon() checks if the daemon process is exiting and and
>> looks at fc->ring.daemon->flags & PF_EXITING - this is what the process
>> reference is for.
> 
> Okay, so you are saying that the lifetime of the ring is bound to the
> lifetime of the thread that created it?
> 
> Why is that?
> 
> I'ts much more common to bind a lifetime of an object to that of an
> open file.  io_uring_setup() will do that for example.
> 
> It's much easier to hook into the destruction of an open file, than
> into the destruction of a process (as you've observed). And the way
> you do it is even more confusing as the ring is destroyed not when the
> process is destroyed, but when a specific thread is destroyed, making
> this a thread specific behavior that is probably best avoided.
> 
> So the obvious solution would be to destroy the ring(s) in
> fuse_dev_release().  Why wouldn't that work?
> 

I _think_ I had tried it at the beginning and run into issues and then 
switched the ublk approach. Going to try again now.

Thanks,
Bernd