Re: [PATCH] virtio-fs: Query rootmode during mount

[Hanna Czenczek]

On 07.11.24 13:18, Miklos Szeredi wrote:
> On Thu, 7 Nov 2024 at 11:00, Hanna Czenczek <hreitz@xxxxxxxxxx> wrote:
> > It isn’t much, but I believe it’s most of fuse_fill_super_common()
> > (without restructuring the code so flags returned by INIT are put into a
> > separate structure and then re-joined into sb and fc later).
> Probably not worth it.
>
> > fuse_send_init() reads sb->s_bdi->ra_pages, process_init_reply() writes
> > it and sb->s_time_gran, ->s_flags, ->s_stack_depth, ->s_export_op, and
> > ->s_iflags.  In addition, process_init_reply() depends on several flags
> > and objects in fc being set up (among those are fc->dax and
> > fc->default_permissions), which is done by fuse_fill_super_common().
> Okay, got it.
>
> > So I think what we need from fuse_fill_super_common() is:
> > - fuse_sb_defaults() (so these values can then be overwritten by
> > process_init_reply()),
> > - fuse_dax_conn_alloc(),
> > - fuse_bdi_init(),
> > - fc->default_permissions at least, but I’d just take the fc->[flag]
> > setting block as a whole then.
> >
> > I assume we’ll also want the SB_MANDLOCK check then, and
> > rcu_assign_pointer().  Then we might as well also set the block sizes
> > and the subtype.
> >
> > The problem is that I don’t know the order things in
> > fuse_fill_super_common() need to be in, and fuse_dev_alloc_install() is
> > called before fuse_bdi_init(), so I didn’t want to move that.
> >
> > So what I understand is that calling fuse_dev_alloc_install() there
> > isn’t necessary?  I’m happy to move that to part 2, as you suggest, but
> Hmm, fuse_dev_install() chains the fud onto fc->devices.  This is used
> by fuse_resend() and fuse_abort_conn().  Resending isn't really
> interesting at this point, but aborting should work from the start, so
> this should not be moved after sending requests.
>
> > I’m not sure we can really omit much from part 1 without changing how
> > process_init_reply() operates.  We could in theory delay
> > process_init_reply() until after GETATTR (and thus after setting
> > s_root), but that seems kind of wrong, and would still require setting
> > up BDI and DAX for fuse_send_init().
> Agree, let's keep the split as is, but store the fud temporarily in
> fuse_fs_context and leave setting *ctx->fudptr to part2.

Sure!

> > > > +       if (sb->s_root || (fm->fc && fm->fc->initialized && !fm->submount)) {
> > > How could fm->submount be set if sb->s_root isn't?
> > fuse_get_tree_submount(), specifically fuse_fill_super_submount() whose
> > error path leads to deactivate_locked_super(), can fail before
> > sb->s_root is set.
> Right.
>
> > Still, the idea was rather to make it clear that this condition (INIT
> > sent but s_root not set) is unique to non-submounts, so as not to mess
> > with the submount code unintentionally.
> >
> > > Or sb->s_root set
> > > and fc->initialized isn't?
> > That would be the non-virtio-fs non-submount case (fuse_fill_super()),
> > where s_root is set first and INIT sent after.
> But this is virtiofs specific code.

Ah, right…  I must have forgotten that at some point.

> Regardless, something smells here: fuse_mount_remove() is only called
> if sb->s_root is set (both plain fuse and virtiofs).  The top level
> fuse_mount is added to fc->mounts in fuse_conn_init(), way before
> sb->s_root is set...
>
> Will look into this.

Thanks!

Hanna

> Thanks,
> Miklos