Re: [PATCH] erofs: reuse superblock for file-backed mounts

[Giuseppe Scrivano <gscrivan@redhat.com>]

Christian Brauner <brauner@kernel.org> writes:

>> >>> The time difference shows that sharing the superblock also benefits
>> >>> the page cache and inode cache, as subsequent mounts of the same image
>> >>> avoid re-reading the backing file.  This is particularly useful for
>> >>> container hosts running multiple containers from the same base image.
>> >>
>> >> Doesn't this approach break-down as soon as you get to change SB flags (through
>> >> e.g mount -o remount)? It will change superblock flags for all of them, right?
>> >>
>> >> (I don't think you can switch off the superblock transparently on a
>> >> reconfigure?)
>> >
>> > This is the same preexisting behavior for block device mounts.  That
>
> Yeah, and it sucks ass. :)
> Only an incompatible vfs-level RO/RW property causes a silent reuse to
> fail. Otherwise it not possible to detect that a superblock has been
> created and requested mount options silently ignore.
>
> In other words, doing this unconditionally is incompatible with current
> userspace.
>
> Btw, I added FSCONFIG_CMD_CREATE_EXCL for this reason which refuses to
> reuse an existing superblock for filesystems that would otherwise end up
> sharing a superblock. This way userspace is guaranteed to not silently
> get filesystem options ignored.
>
>> > said, I realize this can feel confusing since EROFS is not a block
>> > device and allowed this so far.
>> >
>> > One way to solve this could be an explicit mount option "share_sb" that
>> > is opt-in and, once set, blocks any remount operations.  Would that
>> > work?
>> 
>> Would something like the following fixup on top of the previous patch be
>> acceptable (suggestions for better names are welcome)?
>
> The patch as written still means that a task A creates a new erofs mount
> with for file F and marks it as shared. Another task B creates new erofs
> mount expecting to get a new superblock and will end up sharing it with
> task A instead.
>
> That is still quite the behavior change and could be used to subvert
> workloads expectations and be used in (odd) attacks.

no, my suggestion is that task B gets the same superblock created by A
only if it also specifies share_sb.

If the option is not specified, as is the case for existing workloads,
then it gets a new superblock even if A is already mounted with
share_sb.

The tasks A and B will get the same superblock only if they both specify
share_sb.  It is fully opt-in, it doesn't affect any other user that
doesn't want this behavior.

Regards,
Giuseppe



>
> So if you want sb-sharing you need it to be an admin-level setting or
> you need to make it a domain the same way page cache sharing for erofs
> is a domain. IOW, only task that mount with the same domain end up
> sharing superblocks. Or it's a global setting.
>
> But doing it this way seems problematic to me.
>
>> Thanks,
>> Giuseppe
>> 
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index 580f8d9f14e7..ac26daf4f78a 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -156,6 +156,7 @@ struct erofs_sb_info {
>>  #define EROFS_MOUNT_DAX_NEVER		0x00000080
>>  #define EROFS_MOUNT_DIRECT_IO		0x00000100
>>  #define EROFS_MOUNT_INODE_SHARE		0x00000200
>> +#define EROFS_MOUNT_SHARE_SB		0x00000400
>>  
>>  #define clear_opt(opt, option)	((opt)->mount_opt &= ~EROFS_MOUNT_##option)
>>  #define set_opt(opt, option)	((opt)->mount_opt |= EROFS_MOUNT_##option)
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index cbd76727f0d3..dd2a351811c8 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -386,7 +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,
>> +	Opt_source, Opt_share_sb,
>>  };
>>  
>>  static const struct constant_table erofs_param_cache_strategy[] = {
>> @@ -414,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_flag("share_sb",		Opt_share_sb),
>>  	fsparam_file_or_string("source",	Opt_source),
>>  	{}
>>  };
>> @@ -560,6 +561,12 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>>  		else
>>  			set_opt(&sbi->opt, INODE_SHARE);
>>  		break;
>> +	case Opt_share_sb:
>> +		if (!IS_ENABLED(CONFIG_EROFS_FS_BACKED_BY_FILE))
>> +			errorfc(fc, "%s option not supported", erofs_fs_parameters[opt].name);
>> +		else
>> +			set_opt(&sbi->opt, SHARE_SB);
>> +		break;
>>  	case Opt_source:
>>  		return erofs_fc_parse_source(fc, param);
>>  	}
>> @@ -798,6 +805,8 @@ static int erofs_fc_test_file_super(struct super_block *sb,
>>  		return 0;
>>  	if (!sbi->dif0.file || !new_sbi->dif0.file)
>>  		return 0;
>> +	if (!test_opt(&new_sbi->opt, SHARE_SB))
>> +		return 0;
>>  	return file_inode(sbi->dif0.file) == file_inode(new_sbi->dif0.file) &&
>>  	       sbi->dif0.fsoff == new_sbi->dif0.fsoff &&
>>  	       sbi->opt.mount_opt == new_sbi->opt.mount_opt &&
>> @@ -874,6 +883,9 @@ static int erofs_fc_reconfigure(struct fs_context *fc)
>>  
>>  	DBG_BUGON(!sb_rdonly(sb));
>>  
>> +	if (test_opt(&sbi->opt, SHARE_SB))
>> +		return -EBUSY;
>> +
>>  	if (new_sbi->domain_id)
>>  		erofs_info(sb, "ignoring reconfiguration for domain_id.");
>>  
>> 
>> 
>>