LinuxLists.cc - [RFC PATCH 0/9] fuse: API for Checkpoint/Restore

2023-02-20 19:38:23

Subject: [RFC PATCH 0/9] fuse: API for Checkpoint/Restore

Hello everyone,

It would be great to hear your comments regarding this proof-of-concept Checkpoint/Restore API for FUSE.

Support of FUSE C/R is a challenging task for CRIU [1]. Last year I've given a brief talk on LPC 2022
about how we handle files C/R in CRIU and which blockers we have for FUSE filesystems. [2]

The main problem for CRIU is that we have to restore mount namespaces and memory mappings before the process tree.
It means that when CRIU is performing mount of fuse filesystem it can't use the original FUSE daemon from the
restorable process tree, but instead use a "fake daemon".

This leads to many other technical problems:
* "fake" daemon has to reply to FUSE_INIT request from the kernel and initialize fuse connection somehow.
This setup can be not consistent with the original daemon (protocol version, daemon capabilities/settings
like no_open, no_flush, readahead, and so on).
* each fuse request has a unique ID. It could confuse userspace if this unique ID sequence was reset.

We can workaround some issues and implement fragile and limited support of FUSE in CRIU but it doesn't make any sense, IMHO.
Btw, I've enumerated only CRIU restore-stage problems there. The dump stage is another story...

My proposal is not only about CRIU. The same interface can be useful for FUSE mounts recovery after daemon crashes.
LXC project uses LXCFS [3] as a procfs/cgroupfs/sysfs emulation layer for containers. We are using a scheme when
one LXCFS daemon handles all the work for all the containers and we use bindmounts to overmount particular
files/directories in procfs/cgroupfs/sysfs. If this single daemon crashes for some reason we are in trouble,
because we have to restart all the containers (fuse bindmounts become invalid after the crash).
The solution is fairly easy:
allow somehow to reinitialize the existing fuse connection and replace the daemon on the fly
This case is a little bit simpler than CRIU cause we don't need to care about the previously opened files
and other stuff, we are only interested in mounts.

Current PoC implementation was developed and tested with this "recovery case".
Right now I only have LXCFS patched and have nothing for CRIU. But I wanted to discuss this idea before going forward with CRIU.

Brief API/design description.

I've added two ioctl's:
* ioctl(FUSE_DEV_IOC_REINIT)
Performs fuse connection abort and then reinitializes all internal fuse structures as "brand new".
Then sends a FUSE_INIT request, so a new userspace daemon can reply to it and start processing fuse reqs.

* ioctl(FUSE_DEV_IOC_BM_REVAL)
A little bit hacky thing. Traverses all the bindmounts of existing fuse mount and performs relookup
of (struct vfsmount)->mnt_root dentries with the new daemon and reset them to new dentries.
Pretty useful for the recovery case (for instance, LXCFS).

Now about the dentry/inode invalidation mechanism.
* added the "fuse connection generation" concept.
When reinit is performed then connection generation is increased by 1.
Each fuse inode stores the generation of the connection it was allocated with.

* perform dentry revalidation if it has an old generation [fuse_dentry_revalidate]
The current implementation of fuse_dentry_revalidate follows a simple and elegant idea. When we
want to revalidate the dentry we just send a FUSE_LOOKUP request to the userspace
for the parent dentry with the name of the current dentry and check which attributes/inode id
it gets. If inode ids are the same and attributes (provided by the userspace) are valid
then we mark dentry valid and it continues to live (with inode).
I've only added a connection generation check to the condition when we have to perform revalidation
and added an inode connection generation reset (to actual connection gen) if the new userspace
daemon has looked up the same inode id (important for the CRIU case!).

Thank you for your attention and I'm waiting for your feedback :)

References:
[1] Support FUSE mountpoints https://github.com/checkpoint-restore/criu/issues/53
[2] Bringing up FUSE mounts C/R support https://lpc.events/event/16/contributions/1243/
[3] LXCFS https://github.com/lxc/lxcfs

Kind regards,
Alex

Cc: Miklos Szeredi <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Stéphane Graber <[email protected]>
Cc: Seth Forshee <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Andrei Vagin <[email protected]>
Cc: Pavel Tikhomirov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

Alexander Mikhalitsyn (9):
fuse: move FUSE_DEFAULT_* defines to fuse common header
fuse: add const qualifiers to common fuse helpers
fuse: add fuse connection generation
fuse: handle stale inode connection in fuse_queue_forget
fuse: move fuse connection flags to the separate structure
fuse: take fuse connection generation into account
fuse: add fuse device ioctl(FUSE_DEV_IOC_REINIT)
namespace: add sb_revalidate_bindmounts helper
fuse: add fuse device ioctl(FUSE_DEV_IOC_BM_REVAL)

fs/fuse/acl.c | 6 +-
fs/fuse/dev.c | 167 +++++++++++++++++++-
fs/fuse/dir.c | 38 ++---
fs/fuse/file.c | 85 +++++-----
fs/fuse/fuse_i.h | 281 ++++++++++++++++++++--------------
fs/fuse/inode.c | 62 ++++----
fs/fuse/readdir.c | 8 +-
fs/fuse/xattr.c | 18 +--
fs/namespace.c | 90 +++++++++++
include/linux/mnt_namespace.h | 3 +
include/uapi/linux/fuse.h | 2 +
11 files changed, 531 insertions(+), 229 deletions(-)

--
2.34.1

2023-02-20 19:38:26

by Aleksandr Mikhalitsyn

[permalink] [raw]

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 <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: StÃ©phane Graber <[email protected]>
> Cc: Seth Forshee <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Andrei Vagin <[email protected]>
> Cc: Pavel Tikhomirov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Alexander Mikhalitsyn <[email protected]>
> ---
> 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;

Assuming you have a malicious daemon that tries to cause bad behavior,
only allow one ioctl at at time? I.e. add a value that reinit is in
progress? And unset at the end of the function?

> +
> + if (fc->conn_gen + 1 < fc->conn_gen)
> + return -EOVERFLOW;
> +

Add a comment, like

/* Unsets fc->connected and fiq->connected and ensures that no new
requests can be queued */

?

> + fuse_abort_conn(fc);
> + fuse_wait_aborted(fc);
> +
> + spin_lock(&fc->lock);
> + if (fc->connected) {
> + spin_unlock(&fc->lock);
> + return -EINVAL;
> + }
> +
> + if (fc->conn_gen + 1 < fc->conn_gen) {
> + spin_unlock(&fc->lock);
> + return -EOVERFLOW;
> + }
> +
> + fc->conn_gen++;
> +
> + spin_lock(&fiq->lock);
> + if (request_pending(fiq) || fiq->forget_list_tail != &fiq->forget_list_head) {
> + spin_unlock(&fiq->lock);
> + spin_unlock(&fc->lock);
> + return -EINVAL;
> + }
> +
> + if (&fuse_dev_fiq_ops != fiq->ops) {
> + spin_unlock(&fiq->lock);
> + spin_unlock(&fc->lock);
> + return -EOPNOTSUPP;
> + }
> +
> + fiq->connected = 1;
> + spin_unlock(&fiq->lock);
> +
> + spin_lock(&fc->bg_lock);
> + if (!list_empty(&fc->bg_queue)) {
> + spin_unlock(&fc->bg_lock);
> + spin_unlock(&fc->lock);
> + return -EINVAL;
> + }
> +
> + fc->blocked = 0;
> + fc->max_background = FUSE_DEFAULT_MAX_BACKGROUND;
> + spin_unlock(&fc->bg_lock);
> +
> + list_for_each_entry(fud, &fc->devices, entry) {
> + struct fuse_pqueue *fpq = &fud->pq;
> +
> + spin_lock(&fpq->lock);
> + if (!list_empty(&fpq->io)) {
> + spin_unlock(&fpq->lock);
> + spin_unlock(&fc->lock);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
> + if (!list_empty(&fpq->processing[i])) {
> + spin_unlock(&fpq->lock);
> + spin_unlock(&fc->lock);
> + return -EINVAL;
> + }
> + }
> +
> + fpq->connected = 1;
> + spin_unlock(&fpq->lock);
> + }
> +
> + fuse_set_initialized(fc);

I'm not sure about this, why not the common way via FUSE_INIT reply?

> +
> + /* Background queuing checks fc->connected under bg_lock */
> + spin_lock(&fc->bg_lock);
> + fc->connected = 1;
> + spin_unlock(&fc->bg_lock);
> +
> + fc->aborted = false;
> + fc->abort_err = 0;
> +
> + /* nullify all the flags */
> + memset(&fc->flags, 0, sizeof(struct fuse_conn_flags));
> +
> + spin_unlock(&fc->lock);
> +
> + down_read(&fc->killsb);
> + if (!list_empty(&fc->mounts)) {
> + struct fuse_mount *fm;
> +
> + fm = list_first_entry(&fc->mounts, struct fuse_mount, fc_entry);
> + if (!fm->sb) {
> + up_read(&fc->killsb);
> + return -EINVAL;
> + }
> +
> + fuse_send_init(fm);
> + }
> + up_read(&fc->killsb);
> +
> + return 0;
> +}
> +
> void fuse_wait_aborted(struct fuse_conn *fc)
> {
> /* matches implicit memory barrier in fuse_drop_waiting() */
> @@ -2282,6 +2388,32 @@ static long fuse_dev_ioctl(struct file *file, unsigned int cmd,
> }
> }
> break;
> + case FUSE_DEV_IOC_REINIT:
> + struct fuse_conn *fc;
> +
> + if (!checkpoint_restore_ns_capable(file->f_cred->user_ns))
> + return -EPERM;
> +
> + res = -EINVAL;
> + fud = fuse_get_dev(file);
> +
> + /*
> + * Only fuse mounts with an already initialized fuse
> + * connection are supported
> + */
> + if (file->f_op == &fuse_dev_operations && fud) {
> + mutex_lock(&fuse_mutex);
> + fc = fud->fc;
> + if (fc)
> + fc = fuse_conn_get(fc);
> + mutex_unlock(&fuse_mutex);
> +
> + if (fc) {
> + res = fuse_reinit_conn(fc);
> + fuse_conn_put(fc);
> + }
> + }
> + break;
> default:
> res = -ENOTTY;
> break;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 1b9d0dfae72d..3dac67b25eae 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -989,6 +989,7 @@ struct fuse_notify_retrieve_in {
> /* Device ioctls: */
> #define FUSE_DEV_IOC_MAGIC 229
> #define FUSE_DEV_IOC_CLONE _IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t)
> +#define FUSE_DEV_IOC_REINIT _IO(FUSE_DEV_IOC_MAGIC, 0)
>
> struct fuse_lseek_in {
> uint64_t fh;

2023-03-03 19:27:18

by Bernd Schubert

[permalink] [raw]

On 4/3/23 16:51, Aleksandr Mikhalitsyn wrote:
> On Mon, Mar 6, 2023 at 3:09 PM Aleksandr Mikhalitsyn
> <[email protected]> wrote:
>>
>> On Fri, Mar 3, 2023 at 8:26 PM Bernd Schubert
>> <[email protected]> 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 <[email protected]>
>>>> Cc: Al Viro <[email protected]>
>>>> Cc: Amir Goldstein <[email protected]>
>>>> Cc: StÃ©phane Graber <[email protected]>
>>>> Cc: Seth Forshee <[email protected]>
>>>> Cc: Christian Brauner <[email protected]>
>>>> Cc: Andrei Vagin <[email protected]>
>>>> Cc: Pavel Tikhomirov <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Signed-off-by: Alexander Mikhalitsyn <[email protected]>
>>>> ---
>>>> 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