Hi Aleksa,
On 2026/7/13 12:52, Aleksa Sarai wrote:
On 2026-07-11, Giuseppe Scrivano <[email protected]> wrote:
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 86fa5c6a0c70..3040d4cf9b85 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -386,6 +386,7 @@ static void erofs_default_options(struct erofs_sb_info *sbi)
enum {
Opt_user_xattr, Opt_acl, Opt_cache_strategy, Opt_dax, Opt_dax_enum,
Opt_device, Opt_domain_id, Opt_directio, Opt_fsoffset, Opt_inode_share,
+ Opt_source_fd,
};
static const struct constant_table erofs_param_cache_strategy[] = {
@@ -413,6 +414,7 @@ static const struct fs_parameter_spec erofs_fs_parameters[]
= {
fsparam_flag_no("directio", Opt_directio),
fsparam_u64("fsoffset", Opt_fsoffset),
fsparam_flag("inode_share", Opt_inode_share),
+ fsparam_fd("source", Opt_source_fd),
{}
};
@@ -524,6 +526,11 @@ static int erofs_fc_parse_param(struct fs_context *fc,
else
set_opt(&sbi->opt, INODE_SHARE);
break;
+ case Opt_source_fd:
+ if (sbi->dif0.file)
+ return -EINVAL;
+ sbi->dif0.file = get_file(param->file);
+ break;
I don't think this handling is right for a few reasons:
1. AFAICS this shadows the default "source" handling logic (because
-ENOPARAM is not returned for the non-fd case), which means that
this regresses existing erofs users -- everyone already uses
"source" today. I must really be missing something if this worked
when you tested it.
Additionally, fsparam_fd unfortunately permits strings (where the
string is the numerical value of the fd number), meaning that this
will call get_file(<garbage>) if someone uses FSCONFIG_SET_STRING.
You will need to check param->type at least to avoid that.
I meant to send a patch for this earlier this year, but a nicer
solution would be to have a custom helper similar to fs_lookup_param
except that it permits FSCONFIG_SET_FD, FSCONFIG_SET_PATH,
FSCONFIG_SET_PATH_EMPTY, and FSCONFIG_SET_STRING. This is sorely
missing and people keep accidentally creating unusable interfaces as
a result. I mentioned this in an LPC talk last year[1].
proc_parse_pidns_param was my minimal version that only accepts
FSCONFIG_SET_FD and FSCONFIG_SET_STRING, and if you don't want to
add dirfd support yet then you should use something more like that.
Yes, I tried this patch just now, it seems it regresses the default
"source" as you said. I have to withdraw my rvb.
Just check the codebase, it seems at least the minimal change is that
it needs a way for the default "source" to fall back to
vfs_parse_fs_param_source().
My initial rough thought for source_fd support for a filesystem was
not simple as this too, but I never tried to seek time to implement
myself according to my priority list. As you said, I think in order
to better parse both source or source_fd cases, it should be better
to have better vfs helpers to parse both cases in a clean way;
I think then that should also enable bdev-backed source fds [1].
2. On a slightly less critical note, fc->source has special handling in
the VFS in a few places and AFAICS this is the first example of
someone adding an implementation of "source" that does not set
fc->source to a proper value, which deserves some additional review.
(At at quick glance it seems this just means that some stuff in
procfs will show as "none" rather than fc->source debugging, but
again it probably needs a closer look.)
Yes, "none" is useless for source fd cases, it'd be better to have
a path instead.
[1] https://github.com/composefs/composefs-rs/issues/346#issuecomment-4903974902
Thanks,
Gao Xiang
[1]: https://youtu.be/NX5IzF6JXp0?t=72
