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
