Re: [RFC PATCH 7/9] fuse: add fuse device ioctl(FUSE_DEV_IOC_REINIT)

[Bernd Schubert]

On 4/3/23 16:51, Aleksandr Mikhalitsyn wrote:
> On Mon, Mar 6, 2023 at 3:09 PM Aleksandr Mikhalitsyn
> <aleksandr.mikhalitsyn@xxxxxxxxxxxxx> wrote:
>>
>> On Fri, Mar 3, 2023 at 8:26 PM Bernd Schubert
>> <bernd.schubert@xxxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> On 2/20/23 20:37, Alexander Mikhalitsyn wrote:
>>>> This ioctl aborts fuse connection and then reinitializes it,
>>>> sends FUSE_INIT request to allow a new userspace daemon
>>>> to pick up the fuse connection.
>>>>
>>>> Cc: Miklos Szeredi <mszeredi@xxxxxxxxxx>
>>>> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>>>> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
>>>> Cc: Stéphane Graber <stgraber@xxxxxxxxxx>
>>>> Cc: Seth Forshee <sforshee@xxxxxxxxxx>
>>>> Cc: Christian Brauner <brauner@xxxxxxxxxx>
>>>> Cc: Andrei Vagin <avagin@xxxxxxxxx>
>>>> Cc: Pavel Tikhomirov <ptikhomirov@xxxxxxxxxxxxx>
>>>> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>> Cc: criu@xxxxxxxxxx
>>>> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@xxxxxxxxxxxxx>
>>>> ---
>>>>    fs/fuse/dev.c             | 132 ++++++++++++++++++++++++++++++++++++++
>>>>    include/uapi/linux/fuse.h |   1 +
>>>>    2 files changed, 133 insertions(+)
>>>>
>>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>>> index 737764c2295e..0f53ffd63957 100644
>>>> --- a/fs/fuse/dev.c
>>>> +++ b/fs/fuse/dev.c
>>>> @@ -2187,6 +2187,112 @@ void fuse_abort_conn(struct fuse_conn *fc)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(fuse_abort_conn);
>>>>
>>>> +static int fuse_reinit_conn(struct fuse_conn *fc)
>>>> +{
>>>> +     struct fuse_iqueue *fiq = &fc->iq;
>>>> +     struct fuse_dev *fud;
>>>> +     unsigned int i;
>>>> +
>>>> +     if (fc->conn_gen + 1 < fc->conn_gen)
>>>> +             return -EOVERFLOW;
>>>> +
>>>> +     fuse_abort_conn(fc);
>>>> +     fuse_wait_aborted(fc);
>>>
>>> Shouldn't this also try to flush all data first?
> 
> Dear Bernd,
> 
> I've reviewed this place 2nd time and I'm not sure that we have to
> perform any flushing there, because userspace daemon can be dead or
> stuck.
> Technically, if userspace knows that daemon is alive then it can call
> fsync/sync before doing reinit.
> 
> What do you think about it?

Hello Alex,

sorry for my late reply.

Hmm, I just fear that fsync/sync is a bit racy, what is if a user would 
write data after the sync and that would get silently removed by 
fuse_abort_conn()? Isn't what we want:

ioctl
    refuse new requests -> unset fc->initialized
    flush all fc queues (fc->iq.pending, fc->bg_queue, I guess with your 
current patches we do not need to handle forget)
    fuse_abort_conn


So what is missing is the information if the daemon is still running - 
take a daemon reference and then check for PF_EXITING, as in my uring 
patches? Miklos has some objections for that, though.


The alternative would be to mount read-only, then sync, then do the 
ioctl and remount back. I don't know what needs to be done to get 
remount working, though. Just handle it in libfuse mount.fuse and send 
the mount syscall?


As I wrote before, at DDN we want to have run time daemon restart - I'm 
also not opposed to entirely give up on the flush and to just work on a 
restart protocol to make the new daemon to the old state (opened files 
and lookup/forget count). In principle we could even transfer that in 
userspace from one daemon to the other?


Thanks,
Bernd

PS: Will look at the new patches later this week.


Thanks,
Bernd