Christian Brauner <[email protected]> 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.");
>>
>>
>>
>>
