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

[Vivek Goyal <vgoyal@redhat.com>]

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