Re: [PATCH v2] erofs: accept source file descriptor via fsconfig

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
>