[for-6.1 v3 0/3] virtiofsd: Add support for FUSE_SYNCFS request

[for-6.1 v3 0/3] virtiofsd: Add support for FUSE_SYNCFS request

[Greg Kurz]

FUSE_SYNCFS allows the client to flush the host page cache.
This isn't available in upstream linux yet, but the following
tree can be used to test:

https://gitlab.com/gkurz/linux/-/tree/virtio-fs-sync

v3: - track submounts and do per-submount syncfs() (Vivek)
    - based on new version of FUSE_SYNCFS (still not upstream)
      https://listman.redhat.com/archives/virtio-fs/2021-May/msg00025.html

v2: - based on new version of FUSE_SYNCFS
      https://listman.redhat.com/archives/virtio-fs/2021-April/msg00166.html
    - propagate syncfs() errors to client (Vivek)

Greg Kurz (3):
  Update linux headers to 5.13-rc1 + FUSE_SYNCFS
  virtiofsd: Track mounts
  virtiofsd: Add support for FUSE_SYNCFS request

 .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.h   |  35 -
 include/standard-headers/drm/drm_fourcc.h     |  23 +-
 include/standard-headers/linux/ethtool.h      | 109 ++-
 include/standard-headers/linux/fuse.h         |  27 +-
 include/standard-headers/linux/input.h        |   2 +-
 include/standard-headers/linux/virtio_ids.h   |   2 +
 .../standard-headers/rdma/vmw_pvrdma-abi.h    |   7 +
 linux-headers/asm-generic/unistd.h            |  13 +-
 linux-headers/asm-mips/unistd_n32.h           | 752 +++++++--------
 linux-headers/asm-mips/unistd_n64.h           | 704 +++++++-------
 linux-headers/asm-mips/unistd_o32.h           | 844 ++++++++---------
 linux-headers/asm-powerpc/kvm.h               |   2 +
 linux-headers/asm-powerpc/unistd_32.h         | 857 +++++++++---------
 linux-headers/asm-powerpc/unistd_64.h         | 801 ++++++++--------
 linux-headers/asm-s390/unistd_32.h            |   5 +
 linux-headers/asm-s390/unistd_64.h            |   5 +
 linux-headers/asm-x86/kvm.h                   |   1 +
 linux-headers/asm-x86/unistd_32.h             |   5 +
 linux-headers/asm-x86/unistd_64.h             |   5 +
 linux-headers/asm-x86/unistd_x32.h            |   5 +
 linux-headers/linux/kvm.h                     | 134 +++
 linux-headers/linux/userfaultfd.h             |  36 +-
 linux-headers/linux/vfio.h                    |  35 +
 tools/virtiofsd/fuse_lowlevel.c               |  11 +
 tools/virtiofsd/fuse_lowlevel.h               |  12 +
 tools/virtiofsd/passthrough_ll.c              |  80 +-
 tools/virtiofsd/passthrough_seccomp.c         |   1 +
 27 files changed, 2465 insertions(+), 2048 deletions(-)

-- 
2.26.3

[for-6.1 v3 2/3] virtiofsd: Track mounts

[Greg Kurz]

The upcoming implementation of ->sync_fs() needs to know about all
submounts in order to call syncfs() on all of them.

Track every inode that comes up with a new mount id in a GHashTable.
If the mount id isn't available, e.g. no statx() on the host, fallback
on the device id for the key. This is done during lookup because we
only care for the submounts that the client knows about. The inode
is removed from the hash table when ultimately unreferenced. This
can happen on a per-mount basis when the client posts a FUSE_FORGET
request or for all submounts at once with FUSE_DESTROY.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tools/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 1553d2ef454f..dc940a1d048b 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -117,6 +117,7 @@ struct lo_inode {
     GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
 
     mode_t filetype;
+    bool is_mnt;
 };
 
 struct lo_cred {
@@ -163,6 +164,7 @@ struct lo_data {
     bool use_statx;
     struct lo_inode root;
     GHashTable *inodes; /* protected by lo->mutex */
+    GHashTable *mnt_inodes; /* protected by lo->mutex */
     struct lo_map ino_map; /* protected by lo->mutex */
     struct lo_map dirp_map; /* protected by lo->mutex */
     struct lo_map fd_map; /* protected by lo->mutex */
@@ -968,6 +970,31 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
     return 0;
 }
 
+static uint64_t mnt_inode_key(struct lo_inode *inode)
+{
+    /* Prefer mnt_id, fallback on dev */
+    return inode->key.mnt_id ? inode->key.mnt_id : inode->key.dev;
+}
+
+static void add_mnt_inode(struct lo_data *lo, struct lo_inode *inode)
+{
+    uint64_t mnt_key = mnt_inode_key(inode);
+
+    if (!g_hash_table_contains(lo->mnt_inodes, &mnt_key)) {
+        inode->is_mnt = true;
+        g_hash_table_insert(lo->mnt_inodes, &mnt_key, inode);
+    }
+}
+
+static void remove_mnt_inode(struct lo_data *lo, struct lo_inode *inode)
+{
+    uint64_t mnt_key = mnt_inode_key(inode);
+
+    if (inode->is_mnt) {
+        g_hash_table_remove(lo->mnt_inodes, &mnt_key);
+    }
+}
+
 /*
  * Increments nlookup on the inode on success. unref_inode_lolocked() must be
  * called eventually to decrement nlookup again. If inodep is non-NULL, the
@@ -1054,10 +1081,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
         pthread_mutex_lock(&lo->mutex);
         inode->fuse_ino = lo_add_inode_mapping(req, inode);
         g_hash_table_insert(lo->inodes, &inode->key, inode);
+        add_mnt_inode(lo, inode);
         pthread_mutex_unlock(&lo->mutex);
     }
     e->ino = inode->fuse_ino;
 
+    fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli%s\n", (unsigned long long)parent,
+             name, (unsigned long long)e->ino, inode->is_mnt ? " (mount)" : "");
+
     /* Transfer ownership of inode pointer to caller or drop it */
     if (inodep) {
         *inodep = inode;
@@ -1067,9 +1098,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
 
     lo_inode_put(lo, &dir);
 
-    fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli\n", (unsigned long long)parent,
-             name, (unsigned long long)e->ino);
-
     return 0;
 
 out_err:
@@ -1479,6 +1507,7 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
             g_hash_table_destroy(inode->posix_locks);
             pthread_mutex_destroy(&inode->plock_mutex);
         }
+        remove_mnt_inode(lo, inode);
         /* Drop our refcount from lo_do_lookup() */
         lo_inode_put(lo, &inode);
     }
@@ -3129,6 +3158,7 @@ static void lo_destroy(void *userdata)
     struct lo_data *lo = (struct lo_data *)userdata;
 
     pthread_mutex_lock(&lo->mutex);
+    g_hash_table_remove_all(lo->mnt_inodes);
     while (true) {
         GHashTableIter iter;
         gpointer key, value;
@@ -3659,6 +3689,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
         root->posix_locks = g_hash_table_new_full(
             g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
     }
+    add_mnt_inode(lo, root);
 }
 
 static guint lo_key_hash(gconstpointer key)
@@ -3678,6 +3709,10 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
 
 static void fuse_lo_data_cleanup(struct lo_data *lo)
 {
+    if (lo->mnt_inodes) {
+        g_hash_table_destroy(lo->mnt_inodes);
+    }
+
     if (lo->inodes) {
         g_hash_table_destroy(lo->inodes);
     }
@@ -3739,6 +3774,7 @@ int main(int argc, char *argv[])
     lo.root.fd = -1;
     lo.root.fuse_ino = FUSE_ROOT_ID;
     lo.cache = CACHE_AUTO;
+    lo.mnt_inodes = g_hash_table_new(g_int64_hash, g_int64_equal);
 
     /*
      * Set up the ino map like this:
-- 
2.26.3

[for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Greg Kurz]

Honor the expected behavior of syncfs() to synchronously flush all data
and metadata on linux systems. Simply loop on all known submounts and
call syncfs() on them.

Note that syncfs() might suffer from a time penalty if the submounts
are being hammered by some unrelated workload on the host. The only
solution to avoid that is to avoid shared submounts.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tools/virtiofsd/fuse_lowlevel.c       | 11 ++++++++
 tools/virtiofsd/fuse_lowlevel.h       | 12 +++++++++
 tools/virtiofsd/passthrough_ll.c      | 38 +++++++++++++++++++++++++++
 tools/virtiofsd/passthrough_seccomp.c |  1 +
 4 files changed, 62 insertions(+)

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 58e32fc96369..3be95ec903c9 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1870,6 +1870,16 @@ static void do_lseek(fuse_req_t req, fuse_ino_t nodeid,
     }
 }
 
+static void do_syncfs(fuse_req_t req, fuse_ino_t nodeid,
+                      struct fuse_mbuf_iter *iter)
+{
+    if (req->se->op.syncfs) {
+        req->se->op.syncfs(req);
+    } else {
+        fuse_reply_err(req, ENOSYS);
+    }
+}
+
 static void do_init(fuse_req_t req, fuse_ino_t nodeid,
                     struct fuse_mbuf_iter *iter)
 {
@@ -2267,6 +2277,7 @@ static struct {
     [FUSE_RENAME2] = { do_rename2, "RENAME2" },
     [FUSE_COPY_FILE_RANGE] = { do_copy_file_range, "COPY_FILE_RANGE" },
     [FUSE_LSEEK] = { do_lseek, "LSEEK" },
+    [FUSE_SYNCFS] = { do_syncfs, "SYNCFS" },
 };
 
 #define FUSE_MAXOP (sizeof(fuse_ll_ops) / sizeof(fuse_ll_ops[0]))
diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
index 3bf786b03485..890c520b195a 100644
--- a/tools/virtiofsd/fuse_lowlevel.h
+++ b/tools/virtiofsd/fuse_lowlevel.h
@@ -1225,6 +1225,18 @@ struct fuse_lowlevel_ops {
      */
     void (*lseek)(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
                   struct fuse_file_info *fi);
+
+    /**
+     * Synchronize file system content
+     *
+     * If this request is answered with an error code of ENOSYS,
+     * this is treated as success and future calls to syncfs() will
+     * succeed automatically without being sent to the filesystem
+     * process.
+     *
+     * @param req request handle
+     */
+    void (*syncfs)(fuse_req_t req);
 };
 
 /**
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index dc940a1d048b..289900c6d274 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3153,6 +3153,43 @@ static void lo_lseek(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
     }
 }
 
+static void lo_syncfs(fuse_req_t req)
+{
+    struct lo_data *lo = lo_data(req);
+    GHashTableIter iter;
+    gpointer key, value;
+    int err = 0;
+
+    pthread_mutex_lock(&lo->mutex);
+
+    g_hash_table_iter_init(&iter, lo->mnt_inodes);
+    while (g_hash_table_iter_next(&iter, &key, &value)) {
+        struct lo_inode *inode = value;
+        int fd;
+
+        fuse_log(FUSE_LOG_DEBUG, "lo_syncfs(ino=%" PRIu64 ")\n",
+                 inode->fuse_ino);
+
+        fd = lo_inode_open(lo, inode, O_RDONLY);
+        if (fd < 0) {
+            err = -fd;
+            break;
+        }
+
+        if (syncfs(fd) < 0) {
+            err = errno;
+            close(fd);
+            break;
+        }
+
+        close(fd);
+    }
+
+    pthread_mutex_unlock(&lo->mutex);
+
+    fuse_reply_err(req, err);
+}
+
 static void lo_destroy(void *userdata)
 {
     struct lo_data *lo = (struct lo_data *)userdata;
@@ -3214,6 +3251,7 @@ static struct fuse_lowlevel_ops lo_oper = {
     .copy_file_range = lo_copy_file_range,
 #endif
     .lseek = lo_lseek,
+    .syncfs = lo_syncfs,
     .destroy = lo_destroy,
 };
 
diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index 62441cfcdb95..343188447901 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -107,6 +107,7 @@ static const int syscall_allowlist[] = {
     SCMP_SYS(set_robust_list),
     SCMP_SYS(setxattr),
     SCMP_SYS(symlinkat),
+    SCMP_SYS(syncfs),
     SCMP_SYS(time), /* Rarely needed, except on static builds */
     SCMP_SYS(tgkill),
     SCMP_SYS(unlinkat),
-- 
2.26.3

Re: [for-6.1 v3 0/3] virtiofsd: Add support for FUSE_SYNCFS request

[Greg Kurz]

Of course, I forgot to drop the for-6.1 prefix in git publish...

On Mon, 10 May 2021 17:55:36 +0200
Greg Kurz <groug@kaod.org> wrote:

> FUSE_SYNCFS allows the client to flush the host page cache.
> This isn't available in upstream linux yet, but the following
> tree can be used to test:
> 
> https://gitlab.com/gkurz/linux/-/tree/virtio-fs-sync
> 
> v3: - track submounts and do per-submount syncfs() (Vivek)
>     - based on new version of FUSE_SYNCFS (still not upstream)
>       https://listman.redhat.com/archives/virtio-fs/2021-May/msg00025.html
> 
> v2: - based on new version of FUSE_SYNCFS
>       https://listman.redhat.com/archives/virtio-fs/2021-April/msg00166.html
>     - propagate syncfs() errors to client (Vivek)
> 
> Greg Kurz (3):
>   Update linux headers to 5.13-rc1 + FUSE_SYNCFS
>   virtiofsd: Track mounts
>   virtiofsd: Add support for FUSE_SYNCFS request
> 
>  .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.h   |  35 -
>  include/standard-headers/drm/drm_fourcc.h     |  23 +-
>  include/standard-headers/linux/ethtool.h      | 109 ++-
>  include/standard-headers/linux/fuse.h         |  27 +-
>  include/standard-headers/linux/input.h        |   2 +-
>  include/standard-headers/linux/virtio_ids.h   |   2 +
>  .../standard-headers/rdma/vmw_pvrdma-abi.h    |   7 +
>  linux-headers/asm-generic/unistd.h            |  13 +-
>  linux-headers/asm-mips/unistd_n32.h           | 752 +++++++--------
>  linux-headers/asm-mips/unistd_n64.h           | 704 +++++++-------
>  linux-headers/asm-mips/unistd_o32.h           | 844 ++++++++---------
>  linux-headers/asm-powerpc/kvm.h               |   2 +
>  linux-headers/asm-powerpc/unistd_32.h         | 857 +++++++++---------
>  linux-headers/asm-powerpc/unistd_64.h         | 801 ++++++++--------
>  linux-headers/asm-s390/unistd_32.h            |   5 +
>  linux-headers/asm-s390/unistd_64.h            |   5 +
>  linux-headers/asm-x86/kvm.h                   |   1 +
>  linux-headers/asm-x86/unistd_32.h             |   5 +
>  linux-headers/asm-x86/unistd_64.h             |   5 +
>  linux-headers/asm-x86/unistd_x32.h            |   5 +
>  linux-headers/linux/kvm.h                     | 134 +++
>  linux-headers/linux/userfaultfd.h             |  36 +-
>  linux-headers/linux/vfio.h                    |  35 +
>  tools/virtiofsd/fuse_lowlevel.c               |  11 +
>  tools/virtiofsd/fuse_lowlevel.h               |  12 +
>  tools/virtiofsd/passthrough_ll.c              |  80 +-
>  tools/virtiofsd/passthrough_seccomp.c         |   1 +
>  27 files changed, 2465 insertions(+), 2048 deletions(-)
> 

Re: [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Vivek Goyal]

On Mon, May 10, 2021 at 05:55:39PM +0200, Greg Kurz wrote:
> Honor the expected behavior of syncfs() to synchronously flush all data
> and metadata on linux systems. Simply loop on all known submounts and
> call syncfs() on them.
> 
> Note that syncfs() might suffer from a time penalty if the submounts
> are being hammered by some unrelated workload on the host. The only
> solution to avoid that is to avoid shared submounts.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  tools/virtiofsd/fuse_lowlevel.c       | 11 ++++++++
>  tools/virtiofsd/fuse_lowlevel.h       | 12 +++++++++
>  tools/virtiofsd/passthrough_ll.c      | 38 +++++++++++++++++++++++++++
>  tools/virtiofsd/passthrough_seccomp.c |  1 +
>  4 files changed, 62 insertions(+)
> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 58e32fc96369..3be95ec903c9 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1870,6 +1870,16 @@ static void do_lseek(fuse_req_t req, fuse_ino_t nodeid,
>      }
>  }
>  
> +static void do_syncfs(fuse_req_t req, fuse_ino_t nodeid,
> +                      struct fuse_mbuf_iter *iter)
> +{
> +    if (req->se->op.syncfs) {
> +        req->se->op.syncfs(req);
> +    } else {
> +        fuse_reply_err(req, ENOSYS);
> +    }
> +}
> +
>  static void do_init(fuse_req_t req, fuse_ino_t nodeid,
>                      struct fuse_mbuf_iter *iter)
>  {
> @@ -2267,6 +2277,7 @@ static struct {
>      [FUSE_RENAME2] = { do_rename2, "RENAME2" },
>      [FUSE_COPY_FILE_RANGE] = { do_copy_file_range, "COPY_FILE_RANGE" },
>      [FUSE_LSEEK] = { do_lseek, "LSEEK" },
> +    [FUSE_SYNCFS] = { do_syncfs, "SYNCFS" },
>  };
>  
>  #define FUSE_MAXOP (sizeof(fuse_ll_ops) / sizeof(fuse_ll_ops[0]))
> diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> index 3bf786b03485..890c520b195a 100644
> --- a/tools/virtiofsd/fuse_lowlevel.h
> +++ b/tools/virtiofsd/fuse_lowlevel.h
> @@ -1225,6 +1225,18 @@ struct fuse_lowlevel_ops {
>       */
>      void (*lseek)(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
>                    struct fuse_file_info *fi);
> +
> +    /**
> +     * Synchronize file system content
> +     *
> +     * If this request is answered with an error code of ENOSYS,
> +     * this is treated as success and future calls to syncfs() will
> +     * succeed automatically without being sent to the filesystem
> +     * process.
> +     *
> +     * @param req request handle
> +     */
> +    void (*syncfs)(fuse_req_t req);
>  };
>  
>  /**
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index dc940a1d048b..289900c6d274 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3153,6 +3153,43 @@ static void lo_lseek(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
>      }
>  }
>  
> +static void lo_syncfs(fuse_req_t req)
> +{
> +    struct lo_data *lo = lo_data(req);
> +    GHashTableIter iter;
> +    gpointer key, value;
> +    int err = 0;
> +
> +    pthread_mutex_lock(&lo->mutex);
> +
> +    g_hash_table_iter_init(&iter, lo->mnt_inodes);
> +    while (g_hash_table_iter_next(&iter, &key, &value)) {
> +        struct lo_inode *inode = value;
> +        int fd;
> +
> +        fuse_log(FUSE_LOG_DEBUG, "lo_syncfs(ino=%" PRIu64 ")\n",
> +                 inode->fuse_ino);
> +
> +        fd = lo_inode_open(lo, inode, O_RDONLY);
> +        if (fd < 0) {
> +            err = -fd;
> +            break;
> +        }
> +
> +        if (syncfs(fd) < 0) {

I don't have a good feeling about calling syncfs() with lo->mutex held.
This seems to be that global mutex which is held at so many places
and will serialize everything else. I think we agreed that syncfs()
can take 10s of seconds if fs is busy. And that means we will stall
other filesystem operations too.

So will be good if we can call syncfs() outside the lock. May be prepare
a list of inodes which are there, take a reference and drop the lock.
call syncfs and then drop the reference on inode.

Vivek

> +            err = errno;
> +            close(fd);
> +            break;
> +        }
> +
> +        close(fd);
> +    }
> +
> +    pthread_mutex_unlock(&lo->mutex);
> +
> +    fuse_reply_err(req, err);
> +}
> +
>  static void lo_destroy(void *userdata)
>  {
>      struct lo_data *lo = (struct lo_data *)userdata;
> @@ -3214,6 +3251,7 @@ static struct fuse_lowlevel_ops lo_oper = {
>      .copy_file_range = lo_copy_file_range,
>  #endif
>      .lseek = lo_lseek,
> +    .syncfs = lo_syncfs,
>      .destroy = lo_destroy,
>  };
>  
> diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> index 62441cfcdb95..343188447901 100644
> --- a/tools/virtiofsd/passthrough_seccomp.c
> +++ b/tools/virtiofsd/passthrough_seccomp.c
> @@ -107,6 +107,7 @@ static const int syscall_allowlist[] = {
>      SCMP_SYS(set_robust_list),
>      SCMP_SYS(setxattr),
>      SCMP_SYS(symlinkat),
> +    SCMP_SYS(syncfs),
>      SCMP_SYS(time), /* Rarely needed, except on static builds */
>      SCMP_SYS(tgkill),
>      SCMP_SYS(unlinkat),
> -- 
> 2.26.3
> 

Re: [for-6.1 v3 2/3] virtiofsd: Track mounts

[Vivek Goyal]

On Mon, May 10, 2021 at 05:55:38PM +0200, Greg Kurz wrote:
> The upcoming implementation of ->sync_fs() needs to know about all
> submounts in order to call syncfs() on all of them.
> 
> Track every inode that comes up with a new mount id in a GHashTable.
> If the mount id isn't available, e.g. no statx() on the host, fallback
> on the device id for the key. This is done during lookup because we
> only care for the submounts that the client knows about. The inode
> is removed from the hash table when ultimately unreferenced. This
> can happen on a per-mount basis when the client posts a FUSE_FORGET
> request or for all submounts at once with FUSE_DESTROY.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  tools/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 1553d2ef454f..dc940a1d048b 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -117,6 +117,7 @@ struct lo_inode {
>      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
>  
>      mode_t filetype;
> +    bool is_mnt;
>  };
>  
>  struct lo_cred {
> @@ -163,6 +164,7 @@ struct lo_data {
>      bool use_statx;
>      struct lo_inode root;
>      GHashTable *inodes; /* protected by lo->mutex */
> +    GHashTable *mnt_inodes; /* protected by lo->mutex */
>      struct lo_map ino_map; /* protected by lo->mutex */
>      struct lo_map dirp_map; /* protected by lo->mutex */
>      struct lo_map fd_map; /* protected by lo->mutex */
> @@ -968,6 +970,31 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
>      return 0;
>  }
>  
> +static uint64_t mnt_inode_key(struct lo_inode *inode)
> +{
> +    /* Prefer mnt_id, fallback on dev */
> +    return inode->key.mnt_id ? inode->key.mnt_id : inode->key.dev;
> +}
> +
> +static void add_mnt_inode(struct lo_data *lo, struct lo_inode *inode)
> +{
> +    uint64_t mnt_key = mnt_inode_key(inode);
> +
> +    if (!g_hash_table_contains(lo->mnt_inodes, &mnt_key)) {
> +        inode->is_mnt = true;
> +        g_hash_table_insert(lo->mnt_inodes, &mnt_key, inode);
> +    }
> +}
> +
> +static void remove_mnt_inode(struct lo_data *lo, struct lo_inode *inode)
> +{
> +    uint64_t mnt_key = mnt_inode_key(inode);
> +
> +    if (inode->is_mnt) {
> +        g_hash_table_remove(lo->mnt_inodes, &mnt_key);
> +    }

What if same filesystem is mounted at two different mount points. Say at
foo/ and bar/. Now when we lookup foo, we will add that inode to 
hash table. But when we lookup bar, we will not add it. Now say foo
is unmounted, and inode is going away, then we will remove super block
of fs from hash table (while it is still mounted at bar/ ?). Do I
understand it right?

If yes, we probably will need another refcounted object to keep track
of all the instances of the same fs?

Thanks
Vivek


> +}
> +

>  /*
>   * Increments nlookup on the inode on success. unref_inode_lolocked() must be
>   * called eventually to decrement nlookup again. If inodep is non-NULL, the
> @@ -1054,10 +1081,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>          pthread_mutex_lock(&lo->mutex);
>          inode->fuse_ino = lo_add_inode_mapping(req, inode);
>          g_hash_table_insert(lo->inodes, &inode->key, inode);
> +        add_mnt_inode(lo, inode);
>          pthread_mutex_unlock(&lo->mutex);
>      }
>      e->ino = inode->fuse_ino;
>  
> +    fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli%s\n", (unsigned long long)parent,
> +             name, (unsigned long long)e->ino, inode->is_mnt ? " (mount)" : "");
> +
>      /* Transfer ownership of inode pointer to caller or drop it */
>      if (inodep) {
>          *inodep = inode;
> @@ -1067,9 +1098,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>  
>      lo_inode_put(lo, &dir);
>  
> -    fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli\n", (unsigned long long)parent,
> -             name, (unsigned long long)e->ino);
> -
>      return 0;
>  
>  out_err:
> @@ -1479,6 +1507,7 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
>              g_hash_table_destroy(inode->posix_locks);
>              pthread_mutex_destroy(&inode->plock_mutex);
>          }
> +        remove_mnt_inode(lo, inode);
>          /* Drop our refcount from lo_do_lookup() */
>          lo_inode_put(lo, &inode);
>      }
> @@ -3129,6 +3158,7 @@ static void lo_destroy(void *userdata)
>      struct lo_data *lo = (struct lo_data *)userdata;
>  
>      pthread_mutex_lock(&lo->mutex);
> +    g_hash_table_remove_all(lo->mnt_inodes);
>      while (true) {
>          GHashTableIter iter;
>          gpointer key, value;
> @@ -3659,6 +3689,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
>          root->posix_locks = g_hash_table_new_full(
>              g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
>      }
> +    add_mnt_inode(lo, root);
>  }
>  
>  static guint lo_key_hash(gconstpointer key)
> @@ -3678,6 +3709,10 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
>  
>  static void fuse_lo_data_cleanup(struct lo_data *lo)
>  {
> +    if (lo->mnt_inodes) {
> +        g_hash_table_destroy(lo->mnt_inodes);
> +    }
> +
>      if (lo->inodes) {
>          g_hash_table_destroy(lo->inodes);
>      }
> @@ -3739,6 +3774,7 @@ int main(int argc, char *argv[])
>      lo.root.fd = -1;
>      lo.root.fuse_ino = FUSE_ROOT_ID;
>      lo.cache = CACHE_AUTO;
> +    lo.mnt_inodes = g_hash_table_new(g_int64_hash, g_int64_equal);
>  
>      /*
>       * Set up the ino map like this:
> -- 
> 2.26.3
> 

Re: [for-6.1 v3 2/3] virtiofsd: Track mounts

[Greg Kurz]

On Mon, 10 May 2021 15:18:39 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Mon, May 10, 2021 at 05:55:38PM +0200, Greg Kurz wrote:
> > The upcoming implementation of ->sync_fs() needs to know about all
> > submounts in order to call syncfs() on all of them.
> > 
> > Track every inode that comes up with a new mount id in a GHashTable.
> > If the mount id isn't available, e.g. no statx() on the host, fallback
> > on the device id for the key. This is done during lookup because we
> > only care for the submounts that the client knows about. The inode
> > is removed from the hash table when ultimately unreferenced. This
> > can happen on a per-mount basis when the client posts a FUSE_FORGET
> > request or for all submounts at once with FUSE_DESTROY.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 1553d2ef454f..dc940a1d048b 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -117,6 +117,7 @@ struct lo_inode {
> >      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
> >  
> >      mode_t filetype;
> > +    bool is_mnt;
> >  };
> >  
> >  struct lo_cred {
> > @@ -163,6 +164,7 @@ struct lo_data {
> >      bool use_statx;
> >      struct lo_inode root;
> >      GHashTable *inodes; /* protected by lo->mutex */
> > +    GHashTable *mnt_inodes; /* protected by lo->mutex */
> >      struct lo_map ino_map; /* protected by lo->mutex */
> >      struct lo_map dirp_map; /* protected by lo->mutex */
> >      struct lo_map fd_map; /* protected by lo->mutex */
> > @@ -968,6 +970,31 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> >      return 0;
> >  }
> >  
> > +static uint64_t mnt_inode_key(struct lo_inode *inode)
> > +{
> > +    /* Prefer mnt_id, fallback on dev */
> > +    return inode->key.mnt_id ? inode->key.mnt_id : inode->key.dev;
> > +}
> > +
> > +static void add_mnt_inode(struct lo_data *lo, struct lo_inode *inode)
> > +{
> > +    uint64_t mnt_key = mnt_inode_key(inode);
> > +
> > +    if (!g_hash_table_contains(lo->mnt_inodes, &mnt_key)) {
> > +        inode->is_mnt = true;
> > +        g_hash_table_insert(lo->mnt_inodes, &mnt_key, inode);
> > +    }
> > +}
> > +
> > +static void remove_mnt_inode(struct lo_data *lo, struct lo_inode *inode)
> > +{
> > +    uint64_t mnt_key = mnt_inode_key(inode);
> > +
> > +    if (inode->is_mnt) {
> > +        g_hash_table_remove(lo->mnt_inodes, &mnt_key);
> > +    }
> 
> What if same filesystem is mounted at two different mount points. Say at
> foo/ and bar/. Now when we lookup foo, we will add that inode to 

e.g.

mount --bind /var/tmp ${virtiofs_path}/foo
mount --bind /var/tmp ${virtiofs_path}/bar

?

> hash table. But when we lookup bar, we will not add it. Now say foo
> is unmounted, and inode is going away, then we will remove super block
> of fs from hash table (while it is still mounted at bar/ ?). Do I
> understand it right?

If the host provides mount ids in statx(), each of these has a different
mnt_id and is thus considered as a distinct super block.

On a host with an older kernel, the fallback on dev_t would cause them
to be considered the same indeed. But in this case, foo and bar are
already confounded in the inode list, i.e. we can't have one removed
and the other one still around AFAICT.

If virtiofsd still has an inode for foo, it cannot be unmounted in the
host. The client needs to forget the inode first, in which case it
seems we don't care anymore for foo's super block.

> 
> If yes, we probably will need another refcounted object to keep track
> of all the instances of the same fs?
> 

I wanted to do that at first but it seemed unnecessary in the end,
because I couldn't come up with a scenario where we could evict
a super block while some inode under it is still referenced by the
client.

Anyway, I need to respin the patch because I misused the glib
hash table API, i.e. use g_int64_*() functions on unallocated
keys while I should use g_direct_*() to do this optimization.
This has the effect of never removing super blocks and ending
with stale inode pointers in the hash. This is what caused
the EBADF error with syncfs() you reported on irc.

> Thanks
> Vivek
> 

Cheers,

--
Greg

> 
> > +}
> > +
> 
> >  /*
> >   * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> >   * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > @@ -1054,10 +1081,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> >          pthread_mutex_lock(&lo->mutex);
> >          inode->fuse_ino = lo_add_inode_mapping(req, inode);
> >          g_hash_table_insert(lo->inodes, &inode->key, inode);
> > +        add_mnt_inode(lo, inode);
> >          pthread_mutex_unlock(&lo->mutex);
> >      }
> >      e->ino = inode->fuse_ino;
> >  
> > +    fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli%s\n", (unsigned long long)parent,
> > +             name, (unsigned long long)e->ino, inode->is_mnt ? " (mount)" : "");
> > +
> >      /* Transfer ownership of inode pointer to caller or drop it */
> >      if (inodep) {
> >          *inodep = inode;
> > @@ -1067,9 +1098,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> >  
> >      lo_inode_put(lo, &dir);
> >  
> > -    fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli\n", (unsigned long long)parent,
> > -             name, (unsigned long long)e->ino);
> > -
> >      return 0;
> >  
> >  out_err:
> > @@ -1479,6 +1507,7 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
> >              g_hash_table_destroy(inode->posix_locks);
> >              pthread_mutex_destroy(&inode->plock_mutex);
> >          }
> > +        remove_mnt_inode(lo, inode);
> >          /* Drop our refcount from lo_do_lookup() */
> >          lo_inode_put(lo, &inode);
> >      }
> > @@ -3129,6 +3158,7 @@ static void lo_destroy(void *userdata)
> >      struct lo_data *lo = (struct lo_data *)userdata;
> >  
> >      pthread_mutex_lock(&lo->mutex);
> > +    g_hash_table_remove_all(lo->mnt_inodes);
> >      while (true) {
> >          GHashTableIter iter;
> >          gpointer key, value;
> > @@ -3659,6 +3689,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
> >          root->posix_locks = g_hash_table_new_full(
> >              g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> >      }
> > +    add_mnt_inode(lo, root);
> >  }
> >  
> >  static guint lo_key_hash(gconstpointer key)
> > @@ -3678,6 +3709,10 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
> >  
> >  static void fuse_lo_data_cleanup(struct lo_data *lo)
> >  {
> > +    if (lo->mnt_inodes) {
> > +        g_hash_table_destroy(lo->mnt_inodes);
> > +    }
> > +
> >      if (lo->inodes) {
> >          g_hash_table_destroy(lo->inodes);
> >      }
> > @@ -3739,6 +3774,7 @@ int main(int argc, char *argv[])
> >      lo.root.fd = -1;
> >      lo.root.fuse_ino = FUSE_ROOT_ID;
> >      lo.cache = CACHE_AUTO;
> > +    lo.mnt_inodes = g_hash_table_new(g_int64_hash, g_int64_equal);
> >  
> >      /*
> >       * Set up the ino map like this:
> > -- 
> > 2.26.3
> > 
> 

Re: [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Greg Kurz]

On Mon, 10 May 2021 15:15:02 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Mon, May 10, 2021 at 05:55:39PM +0200, Greg Kurz wrote:
> > Honor the expected behavior of syncfs() to synchronously flush all data
> > and metadata on linux systems. Simply loop on all known submounts and
> > call syncfs() on them.
> > 
> > Note that syncfs() might suffer from a time penalty if the submounts
> > are being hammered by some unrelated workload on the host. The only
> > solution to avoid that is to avoid shared submounts.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  tools/virtiofsd/fuse_lowlevel.c       | 11 ++++++++
> >  tools/virtiofsd/fuse_lowlevel.h       | 12 +++++++++
> >  tools/virtiofsd/passthrough_ll.c      | 38 +++++++++++++++++++++++++++
> >  tools/virtiofsd/passthrough_seccomp.c |  1 +
> >  4 files changed, 62 insertions(+)
> > 
> > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > index 58e32fc96369..3be95ec903c9 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.c
> > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > @@ -1870,6 +1870,16 @@ static void do_lseek(fuse_req_t req, fuse_ino_t nodeid,
> >      }
> >  }
> >  
> > +static void do_syncfs(fuse_req_t req, fuse_ino_t nodeid,
> > +                      struct fuse_mbuf_iter *iter)
> > +{
> > +    if (req->se->op.syncfs) {
> > +        req->se->op.syncfs(req);
> > +    } else {
> > +        fuse_reply_err(req, ENOSYS);
> > +    }
> > +}
> > +
> >  static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> >                      struct fuse_mbuf_iter *iter)
> >  {
> > @@ -2267,6 +2277,7 @@ static struct {
> >      [FUSE_RENAME2] = { do_rename2, "RENAME2" },
> >      [FUSE_COPY_FILE_RANGE] = { do_copy_file_range, "COPY_FILE_RANGE" },
> >      [FUSE_LSEEK] = { do_lseek, "LSEEK" },
> > +    [FUSE_SYNCFS] = { do_syncfs, "SYNCFS" },
> >  };
> >  
> >  #define FUSE_MAXOP (sizeof(fuse_ll_ops) / sizeof(fuse_ll_ops[0]))
> > diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> > index 3bf786b03485..890c520b195a 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.h
> > +++ b/tools/virtiofsd/fuse_lowlevel.h
> > @@ -1225,6 +1225,18 @@ struct fuse_lowlevel_ops {
> >       */
> >      void (*lseek)(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
> >                    struct fuse_file_info *fi);
> > +
> > +    /**
> > +     * Synchronize file system content
> > +     *
> > +     * If this request is answered with an error code of ENOSYS,
> > +     * this is treated as success and future calls to syncfs() will
> > +     * succeed automatically without being sent to the filesystem
> > +     * process.
> > +     *
> > +     * @param req request handle
> > +     */
> > +    void (*syncfs)(fuse_req_t req);
> >  };
> >  
> >  /**
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index dc940a1d048b..289900c6d274 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -3153,6 +3153,43 @@ static void lo_lseek(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
> >      }
> >  }
> >  
> > +static void lo_syncfs(fuse_req_t req)
> > +{
> > +    struct lo_data *lo = lo_data(req);
> > +    GHashTableIter iter;
> > +    gpointer key, value;
> > +    int err = 0;
> > +
> > +    pthread_mutex_lock(&lo->mutex);
> > +
> > +    g_hash_table_iter_init(&iter, lo->mnt_inodes);
> > +    while (g_hash_table_iter_next(&iter, &key, &value)) {
> > +        struct lo_inode *inode = value;
> > +        int fd;
> > +
> > +        fuse_log(FUSE_LOG_DEBUG, "lo_syncfs(ino=%" PRIu64 ")\n",
> > +                 inode->fuse_ino);
> > +
> > +        fd = lo_inode_open(lo, inode, O_RDONLY);
> > +        if (fd < 0) {
> > +            err = -fd;
> > +            break;
> > +        }
> > +
> > +        if (syncfs(fd) < 0) {
> 
> I don't have a good feeling about calling syncfs() with lo->mutex held.
> This seems to be that global mutex which is held at so many places
> and will serialize everything else. I think we agreed that syncfs()
> can take 10s of seconds if fs is busy. And that means we will stall
> other filesystem operations too.
> 
> So will be good if we can call syncfs() outside the lock. May be prepare
> a list of inodes which are there, take a reference and drop the lock.
> call syncfs and then drop the reference on inode.
> 

You're right. I'll do that.

> Vivek
> 
> > +            err = errno;
> > +            close(fd);
> > +            break;
> > +        }
> > +
> > +        close(fd);
> > +    }
> > +
> > +    pthread_mutex_unlock(&lo->mutex);
> > +
> > +    fuse_reply_err(req, err);
> > +}
> > +
> >  static void lo_destroy(void *userdata)
> >  {
> >      struct lo_data *lo = (struct lo_data *)userdata;
> > @@ -3214,6 +3251,7 @@ static struct fuse_lowlevel_ops lo_oper = {
> >      .copy_file_range = lo_copy_file_range,
> >  #endif
> >      .lseek = lo_lseek,
> > +    .syncfs = lo_syncfs,
> >      .destroy = lo_destroy,
> >  };
> >  
> > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
> > index 62441cfcdb95..343188447901 100644
> > --- a/tools/virtiofsd/passthrough_seccomp.c
> > +++ b/tools/virtiofsd/passthrough_seccomp.c
> > @@ -107,6 +107,7 @@ static const int syscall_allowlist[] = {
> >      SCMP_SYS(set_robust_list),
> >      SCMP_SYS(setxattr),
> >      SCMP_SYS(symlinkat),
> > +    SCMP_SYS(syncfs),
> >      SCMP_SYS(time), /* Rarely needed, except on static builds */
> >      SCMP_SYS(tgkill),
> >      SCMP_SYS(unlinkat),
> > -- 
> > 2.26.3
> > 
> 

Re: [Virtio-fs] [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Miklos Szeredi]

On Mon, May 10, 2021 at 5:55 PM Greg Kurz <groug@kaod.org> wrote:
>
> Honor the expected behavior of syncfs() to synchronously flush all data
> and metadata on linux systems. Simply loop on all known submounts and
> call syncfs() on them.

Why not pass the submount's root to the server, so it can do just one
targeted syncfs?

E.g. somehting like this in fuse_sync_fs():

args.nodeid = get_node_id(sb->s_root->d_inode);

Thanks,
Miklos

Re: [Virtio-fs] [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Vivek Goyal]

On Tue, May 11, 2021 at 02:31:14PM +0200, Miklos Szeredi wrote:
> On Mon, May 10, 2021 at 5:55 PM Greg Kurz <groug@kaod.org> wrote:
> >
> > Honor the expected behavior of syncfs() to synchronously flush all data
> > and metadata on linux systems. Simply loop on all known submounts and
> > call syncfs() on them.
> 
> Why not pass the submount's root to the server, so it can do just one
> targeted syncfs?
> 
> E.g. somehting like this in fuse_sync_fs():
> 
> args.nodeid = get_node_id(sb->s_root->d_inode);

Hi Miklos,

I think current proposal was due to lack of full understanding on my part.
I was assuming we have one super block in client and that's not the case
looks like. For every submount, we will have another superblock known
to vfs, IIUC. That means when sync() happens, we will receive ->syncfs()
for each of those super blocks. And that means file server does not
have to keep track of submounts explicitly and it will either receive
a single targeted SYNCFS (for the case of syncfs(fd)) or receive
multile SYNCFS calls (one for each submount when sync() is called).

If that's the case, it makes sense to send nodeid of the root dentry
of superblock and file server can just call syncfs(inode->fd).

Thanks
Vivek

Re: [Virtio-fs] [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Vivek Goyal]

On Tue, May 11, 2021 at 08:54:09AM -0400, Vivek Goyal wrote:
> On Tue, May 11, 2021 at 02:31:14PM +0200, Miklos Szeredi wrote:
> > On Mon, May 10, 2021 at 5:55 PM Greg Kurz <groug@kaod.org> wrote:
> > >
> > > Honor the expected behavior of syncfs() to synchronously flush all data
> > > and metadata on linux systems. Simply loop on all known submounts and
> > > call syncfs() on them.
> > 
> > Why not pass the submount's root to the server, so it can do just one
> > targeted syncfs?
> > 
> > E.g. somehting like this in fuse_sync_fs():
> > 
> > args.nodeid = get_node_id(sb->s_root->d_inode);
> 
> Hi Miklos,
> 
> I think current proposal was due to lack of full understanding on my part.
> I was assuming we have one super block in client and that's not the case
> looks like. For every submount, we will have another superblock known
> to vfs, IIUC. That means when sync() happens, we will receive ->syncfs()
> for each of those super blocks. And that means file server does not
> have to keep track of submounts explicitly and it will either receive
> a single targeted SYNCFS (for the case of syncfs(fd)) or receive
> multile SYNCFS calls (one for each submount when sync() is called).

Tried sync() with submounts enabled and we are seeing a SYNCFS call
only for top level super block and not for submounts.

Greg noticed that it probably is due to the fact that iterate_super()
skips super blocks which don't have SB_BORN flag set. 

Only vfs_get_tree() seems to set SB_BORN and for our submounts we
are not calling vfs_get_tree(), hence SB_BORN is not set. NFS seems
to call vfs_get_tree() and hence SB_BORN must be set for submounts.

Maybe we need to modify virtio_fs_get_tree() so that it can deal with
mount as well as submounts and then fuse_dentry_automount() should
probably call vfs_get_tree() and that should set SB_BORN and hopefully
sync() will work with it. Greg is planning to give it a try.

Does it sound reasonable.

Thanks
Vivek

Re: [Virtio-fs] [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Greg Kurz]

On Tue, 11 May 2021 14:31:14 +0200
Miklos Szeredi <mszeredi@redhat.com> wrote:

> On Mon, May 10, 2021 at 5:55 PM Greg Kurz <groug@kaod.org> wrote:
> >
> > Honor the expected behavior of syncfs() to synchronously flush all data
> > and metadata on linux systems. Simply loop on all known submounts and
> > call syncfs() on them.
> 
> Why not pass the submount's root to the server, so it can do just one
> targeted syncfs?
> 
> E.g. somehting like this in fuse_sync_fs():
> 
> args.nodeid = get_node_id(sb->s_root->d_inode);
> 
> Thanks,
> Miklos
> 

As Vivek already pointed out, there was some misunderstanding on
how submounts were supposed to work. Things got clearer since then :)

So, basically, we have two cases:

1) virtiofsd announces submounts : the d_automount implementation
   creates a new super block and mounts the submount

2) virtiofsd doesn't announce submounts: the client only knows
   about the top-level super block

You suggestion is for case 1) while this series was made with
case 2) in mind, hence the tracking of the super blocks in
the server.

Vivek and I discussed and agreed to address 2) later and
to just focus on 1) for now.

Your suggestion doesn't work with the current code base
because ->sync_fs() is never called on our submounts'
super blocks. This is because they don't have SB_BORN
set, which looks incorrect. A call to vfs_get_tree() would
fix it, but some code refactoring is needed in
fuse_dentry_automount() and virtio_fs_get_tree() for that.

Cheers,

--
Greg

Re: [Virtio-fs] [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Miklos Szeredi]

On Tue, May 11, 2021 at 4:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, May 11, 2021 at 08:54:09AM -0400, Vivek Goyal wrote:
> > On Tue, May 11, 2021 at 02:31:14PM +0200, Miklos Szeredi wrote:
> > > On Mon, May 10, 2021 at 5:55 PM Greg Kurz <groug@kaod.org> wrote:
> > > >
> > > > Honor the expected behavior of syncfs() to synchronously flush all data
> > > > and metadata on linux systems. Simply loop on all known submounts and
> > > > call syncfs() on them.
> > >
> > > Why not pass the submount's root to the server, so it can do just one
> > > targeted syncfs?
> > >
> > > E.g. somehting like this in fuse_sync_fs():
> > >
> > > args.nodeid = get_node_id(sb->s_root->d_inode);
> >
> > Hi Miklos,
> >
> > I think current proposal was due to lack of full understanding on my part.
> > I was assuming we have one super block in client and that's not the case
> > looks like. For every submount, we will have another superblock known
> > to vfs, IIUC. That means when sync() happens, we will receive ->syncfs()
> > for each of those super blocks. And that means file server does not
> > have to keep track of submounts explicitly and it will either receive
> > a single targeted SYNCFS (for the case of syncfs(fd)) or receive
> > multile SYNCFS calls (one for each submount when sync() is called).
>
> Tried sync() with submounts enabled and we are seeing a SYNCFS call
> only for top level super block and not for submounts.
>
> Greg noticed that it probably is due to the fact that iterate_super()
> skips super blocks which don't have SB_BORN flag set.
>
> Only vfs_get_tree() seems to set SB_BORN and for our submounts we
> are not calling vfs_get_tree(), hence SB_BORN is not set. NFS seems
> to call vfs_get_tree() and hence SB_BORN must be set for submounts.
>
> Maybe we need to modify virtio_fs_get_tree() so that it can deal with
> mount as well as submounts and then fuse_dentry_automount() should
> probably call vfs_get_tree() and that should set SB_BORN and hopefully
> sync() will work with it. Greg is planning to give it a try.
>
> Does it sound reasonable.

Just setting SB_BORN sounds much simpler.  What's the disadvantage?

Thanks,
Miklos

Re: [Virtio-fs] [for-6.1 v3 3/3] virtiofsd: Add support for FUSE_SYNCFS request

[Vivek Goyal]

On Tue, May 11, 2021 at 05:08:42PM +0200, Miklos Szeredi wrote:
> On Tue, May 11, 2021 at 4:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, May 11, 2021 at 08:54:09AM -0400, Vivek Goyal wrote:
> > > On Tue, May 11, 2021 at 02:31:14PM +0200, Miklos Szeredi wrote:
> > > > On Mon, May 10, 2021 at 5:55 PM Greg Kurz <groug@kaod.org> wrote:
> > > > >
> > > > > Honor the expected behavior of syncfs() to synchronously flush all data
> > > > > and metadata on linux systems. Simply loop on all known submounts and
> > > > > call syncfs() on them.
> > > >
> > > > Why not pass the submount's root to the server, so it can do just one
> > > > targeted syncfs?
> > > >
> > > > E.g. somehting like this in fuse_sync_fs():
> > > >
> > > > args.nodeid = get_node_id(sb->s_root->d_inode);
> > >
> > > Hi Miklos,
> > >
> > > I think current proposal was due to lack of full understanding on my part.
> > > I was assuming we have one super block in client and that's not the case
> > > looks like. For every submount, we will have another superblock known
> > > to vfs, IIUC. That means when sync() happens, we will receive ->syncfs()
> > > for each of those super blocks. And that means file server does not
> > > have to keep track of submounts explicitly and it will either receive
> > > a single targeted SYNCFS (for the case of syncfs(fd)) or receive
> > > multile SYNCFS calls (one for each submount when sync() is called).
> >
> > Tried sync() with submounts enabled and we are seeing a SYNCFS call
> > only for top level super block and not for submounts.
> >
> > Greg noticed that it probably is due to the fact that iterate_super()
> > skips super blocks which don't have SB_BORN flag set.
> >
> > Only vfs_get_tree() seems to set SB_BORN and for our submounts we
> > are not calling vfs_get_tree(), hence SB_BORN is not set. NFS seems
> > to call vfs_get_tree() and hence SB_BORN must be set for submounts.
> >
> > Maybe we need to modify virtio_fs_get_tree() so that it can deal with
> > mount as well as submounts and then fuse_dentry_automount() should
> > probably call vfs_get_tree() and that should set SB_BORN and hopefully
> > sync() will work with it. Greg is planning to give it a try.
> >
> > Does it sound reasonable.
> 
> Just setting SB_BORN sounds much simpler.  What's the disadvantage?

I was little hesitant to set it directly because no other filesystem
seems to be doing it. Hence I assumed that VFS expects filesystems to
not set SB_BORN.

But I do agree that setting SB_BORN in automount code is much simpler
solution.

Thanks
Vivek

Re: [for-6.1 v3 0/3] virtiofsd: Add support for FUSE_SYNCFS request

[Vivek Goyal]

Hi Greg,

I don't see FUSE_SYNCFS support in virtiofsd. I see that kernel 
patches got merged. Did you post another version of patches?

Will be nice to add syncfs support in virtiofsd/virtiofsd_rs as well. 

Thanks
Vivek

On Mon, May 10, 2021 at 05:55:36PM +0200, Greg Kurz wrote:
> FUSE_SYNCFS allows the client to flush the host page cache.
> This isn't available in upstream linux yet, but the following
> tree can be used to test:
> 
> https://gitlab.com/gkurz/linux/-/tree/virtio-fs-sync
> 
> v3: - track submounts and do per-submount syncfs() (Vivek)
>     - based on new version of FUSE_SYNCFS (still not upstream)
>       https://listman.redhat.com/archives/virtio-fs/2021-May/msg00025.html
> 
> v2: - based on new version of FUSE_SYNCFS
>       https://listman.redhat.com/archives/virtio-fs/2021-April/msg00166.html
>     - propagate syncfs() errors to client (Vivek)
> 
> Greg Kurz (3):
>   Update linux headers to 5.13-rc1 + FUSE_SYNCFS
>   virtiofsd: Track mounts
>   virtiofsd: Add support for FUSE_SYNCFS request
> 
>  .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.h   |  35 -
>  include/standard-headers/drm/drm_fourcc.h     |  23 +-
>  include/standard-headers/linux/ethtool.h      | 109 ++-
>  include/standard-headers/linux/fuse.h         |  27 +-
>  include/standard-headers/linux/input.h        |   2 +-
>  include/standard-headers/linux/virtio_ids.h   |   2 +
>  .../standard-headers/rdma/vmw_pvrdma-abi.h    |   7 +
>  linux-headers/asm-generic/unistd.h            |  13 +-
>  linux-headers/asm-mips/unistd_n32.h           | 752 +++++++--------
>  linux-headers/asm-mips/unistd_n64.h           | 704 +++++++-------
>  linux-headers/asm-mips/unistd_o32.h           | 844 ++++++++---------
>  linux-headers/asm-powerpc/kvm.h               |   2 +
>  linux-headers/asm-powerpc/unistd_32.h         | 857 +++++++++---------
>  linux-headers/asm-powerpc/unistd_64.h         | 801 ++++++++--------
>  linux-headers/asm-s390/unistd_32.h            |   5 +
>  linux-headers/asm-s390/unistd_64.h            |   5 +
>  linux-headers/asm-x86/kvm.h                   |   1 +
>  linux-headers/asm-x86/unistd_32.h             |   5 +
>  linux-headers/asm-x86/unistd_64.h             |   5 +
>  linux-headers/asm-x86/unistd_x32.h            |   5 +
>  linux-headers/linux/kvm.h                     | 134 +++
>  linux-headers/linux/userfaultfd.h             |  36 +-
>  linux-headers/linux/vfio.h                    |  35 +
>  tools/virtiofsd/fuse_lowlevel.c               |  11 +
>  tools/virtiofsd/fuse_lowlevel.h               |  12 +
>  tools/virtiofsd/passthrough_ll.c              |  80 +-
>  tools/virtiofsd/passthrough_seccomp.c         |   1 +
>  27 files changed, 2465 insertions(+), 2048 deletions(-)
> 
> -- 
> 2.26.3
> 
> 

Re: [for-6.1 v3 0/3] virtiofsd: Add support for FUSE_SYNCFS request

[Greg Kurz]

On Wed, 10 Nov 2021 14:48:40 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:

> Hi Greg,
> 
> I don't see FUSE_SYNCFS support in virtiofsd. I see that kernel 
> patches got merged. Did you post another version of patches?
> 

Hi Vivek,

Unfortunately no and I'm really not sure I can get time to do
so. Sorry for that... :-\

Please feel free to take over.

Cheers,

--
Greg

> Will be nice to add syncfs support in virtiofsd/virtiofsd_rs as well. 
> 
> Thanks
> Vivek
> 
> On Mon, May 10, 2021 at 05:55:36PM +0200, Greg Kurz wrote:
> > FUSE_SYNCFS allows the client to flush the host page cache.
> > This isn't available in upstream linux yet, but the following
> > tree can be used to test:
> > 
> > https://gitlab.com/gkurz/linux/-/tree/virtio-fs-sync
> > 
> > v3: - track submounts and do per-submount syncfs() (Vivek)
> >     - based on new version of FUSE_SYNCFS (still not upstream)
> >       https://listman.redhat.com/archives/virtio-fs/2021-May/msg00025.html
> > 
> > v2: - based on new version of FUSE_SYNCFS
> >       https://listman.redhat.com/archives/virtio-fs/2021-April/msg00166.html
> >     - propagate syncfs() errors to client (Vivek)
> > 
> > Greg Kurz (3):
> >   Update linux headers to 5.13-rc1 + FUSE_SYNCFS
> >   virtiofsd: Track mounts
> >   virtiofsd: Add support for FUSE_SYNCFS request
> > 
> >  .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.h   |  35 -
> >  include/standard-headers/drm/drm_fourcc.h     |  23 +-
> >  include/standard-headers/linux/ethtool.h      | 109 ++-
> >  include/standard-headers/linux/fuse.h         |  27 +-
> >  include/standard-headers/linux/input.h        |   2 +-
> >  include/standard-headers/linux/virtio_ids.h   |   2 +
> >  .../standard-headers/rdma/vmw_pvrdma-abi.h    |   7 +
> >  linux-headers/asm-generic/unistd.h            |  13 +-
> >  linux-headers/asm-mips/unistd_n32.h           | 752 +++++++--------
> >  linux-headers/asm-mips/unistd_n64.h           | 704 +++++++-------
> >  linux-headers/asm-mips/unistd_o32.h           | 844 ++++++++---------
> >  linux-headers/asm-powerpc/kvm.h               |   2 +
> >  linux-headers/asm-powerpc/unistd_32.h         | 857 +++++++++---------
> >  linux-headers/asm-powerpc/unistd_64.h         | 801 ++++++++--------
> >  linux-headers/asm-s390/unistd_32.h            |   5 +
> >  linux-headers/asm-s390/unistd_64.h            |   5 +
> >  linux-headers/asm-x86/kvm.h                   |   1 +
> >  linux-headers/asm-x86/unistd_32.h             |   5 +
> >  linux-headers/asm-x86/unistd_64.h             |   5 +
> >  linux-headers/asm-x86/unistd_x32.h            |   5 +
> >  linux-headers/linux/kvm.h                     | 134 +++
> >  linux-headers/linux/userfaultfd.h             |  36 +-
> >  linux-headers/linux/vfio.h                    |  35 +
> >  tools/virtiofsd/fuse_lowlevel.c               |  11 +
> >  tools/virtiofsd/fuse_lowlevel.h               |  12 +
> >  tools/virtiofsd/passthrough_ll.c              |  80 +-
> >  tools/virtiofsd/passthrough_seccomp.c         |   1 +
> >  27 files changed, 2465 insertions(+), 2048 deletions(-)
> > 
> > -- 
> > 2.26.3
> > 
> > 
>