Hi Tao,
We've noticed this warning during development. It seems not a bug, so
I ignored that.
Many thanks for cacthing and clearing up the annoying warning.
在 2022/11/25 15:40, Hou Tao 写道:
> From: Hou Tao <[email protected]>
>
> When shared domain is enabled, doing mount twice with the same fsid and
> domain_id will trigger sysfs warning as shown below:
>
> sysfs: cannot create duplicate filename '/fs/erofs/d0,meta.bin'
> CPU: 15 PID: 1051 Comm: mount Not tainted 6.1.0-rc6+ #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> Call Trace:
> <TASK>
> dump_stack_lvl+0x38/0x49
> dump_stack+0x10/0x12
> sysfs_warn_dup.cold+0x17/0x27
> sysfs_create_dir_ns+0xb8/0xd0
> kobject_add_internal+0xb1/0x240
> kobject_init_and_add+0x71/0xa0
> erofs_register_sysfs+0x89/0x110
> erofs_fc_fill_super+0x98c/0xaf0
> vfs_get_super+0x7d/0x100
> get_tree_nodev+0x16/0x20
> erofs_fc_get_tree+0x20/0x30
> vfs_get_tree+0x24/0xb0
> path_mount+0x2fa/0xa90
> do_mount+0x7c/0xa0
> __x64_sys_mount+0x8b/0xe0
> do_syscall_64+0x30/0x60
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> The reason is erofs_fscache_register_cookie() doesn't guarantee the primary
> data blob (aka fsid) is unique in the shared domain and
> erofs_register_sysfs() invoked by the second mount will fail due to the
> duplicated fsid in the shared domain and report warning.
>
> It would be better to check the uniqueness of fsid before doing
> erofs_register_sysfs(), so adding a new flags parameter for
> erofs_fscache_register_cookie() and doing the uniqueness check if
> EROFS_REG_COOKIE_NEED_NOEXIST is enabled.
>
> After the patch, the error in dmesg for the duplicated mount would be:
>
> erofs: ...: erofs_domain_register_cookie: XX already exists in domain YY
>
> Signed-off-by: Hou Tao <[email protected]>
> ---
> fs/erofs/fscache.c | 47 +++++++++++++++++++++++++++++++++------------
> fs/erofs/internal.h | 10 ++++++++--
> fs/erofs/super.c | 2 +-
> 3 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> index af5ed6b9c54d..6a792a513d6b 100644
> --- a/fs/erofs/fscache.c
> +++ b/fs/erofs/fscache.c
> @@ -494,7 +494,8 @@ static int erofs_fscache_register_domain(struct super_block *sb)
>
> static
> struct erofs_fscache *erofs_fscache_acquire_cookie(struct super_block *sb,
> - char *name, bool need_inode)
> + char *name,
> + unsigned int flags)
> {
> struct fscache_volume *volume = EROFS_SB(sb)->volume;
> struct erofs_fscache *ctx;
> @@ -516,7 +517,7 @@ struct erofs_fscache *erofs_fscache_acquire_cookie(struct super_block *sb,
> fscache_use_cookie(cookie, false);
> ctx->cookie = cookie;
>
> - if (need_inode) {
> + if (flags & EROFS_REG_COOKIE_NEED_INODE) {
> struct inode *const inode = new_inode(sb);
>
> if (!inode) {
> @@ -554,14 +555,15 @@ static void erofs_fscache_relinquish_cookie(struct erofs_fscache *ctx)
>
> static
> struct erofs_fscache *erofs_fscache_domain_init_cookie(struct super_block *sb,
> - char *name, bool need_inode)
> + char *name,
> + unsigned int flags)
> {
> int err;
> struct inode *inode;
> struct erofs_fscache *ctx;
> struct erofs_domain *domain = EROFS_SB(sb)->domain;
>
> - ctx = erofs_fscache_acquire_cookie(sb, name, need_inode);
> + ctx = erofs_fscache_acquire_cookie(sb, name, flags);
> if (IS_ERR(ctx))
> return ctx;
>
> @@ -589,7 +591,8 @@ struct erofs_fscache *erofs_fscache_domain_init_cookie(struct super_block *sb,
>
> static
> struct erofs_fscache *erofs_domain_register_cookie(struct super_block *sb,
> - char *name, bool need_inode)
> + char *name,
> + unsigned int flags)
> {
> struct inode *inode;
> struct erofs_fscache *ctx;
> @@ -602,23 +605,30 @@ struct erofs_fscache *erofs_domain_register_cookie(struct super_block *sb,
> ctx = inode->i_private;
> if (!ctx || ctx->domain != domain || strcmp(ctx->name, name))
> continue;
> - igrab(inode);
> + if (!(flags & EROFS_REG_COOKIE_NEED_NOEXIST)) {
> + igrab(inode);
> + } else {
> + erofs_err(sb, "%s already exists in domain %s", name,
> + domain->domain_id);
> + ctx = ERR_PTR(-EEXIST);
> + }
> spin_unlock(&psb->s_inode_list_lock);
> mutex_unlock(&erofs_domain_cookies_lock);
> return ctx;
> }
> spin_unlock(&psb->s_inode_list_lock);
> - ctx = erofs_fscache_domain_init_cookie(sb, name, need_inode);
> + ctx = erofs_fscache_domain_init_cookie(sb, name, flags);
> mutex_unlock(&erofs_domain_cookies_lock);
> return ctx;
> }
>
> struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
> - char *name, bool need_inode)
> + char *name,
> + unsigned int flags)
> {
> if (EROFS_SB(sb)->domain_id)
> - return erofs_domain_register_cookie(sb, name, need_inode);
> - return erofs_fscache_acquire_cookie(sb, name, need_inode);
> + return erofs_domain_register_cookie(sb, name, flags);
> + return erofs_fscache_acquire_cookie(sb, name, flags);
> }
>
> void erofs_fscache_unregister_cookie(struct erofs_fscache *ctx)
> @@ -647,6 +657,7 @@ int erofs_fscache_register_fs(struct super_block *sb)
> int ret;
> struct erofs_sb_info *sbi = EROFS_SB(sb);
> struct erofs_fscache *fscache;
> + unsigned int flags;
>
> if (sbi->domain_id)
> ret = erofs_fscache_register_domain(sb);
> @@ -655,8 +666,20 @@ int erofs_fscache_register_fs(struct super_block *sb)
> if (ret)
> return ret;
>
> - /* acquired domain/volume will be relinquished in kill_sb() on error */
> - fscache = erofs_fscache_register_cookie(sb, sbi->fsid, true);
> + /*
> + * When shared domain is enabled, using NEED_NOEXIST to guarantee
> + * the primary data blob (aka fsid) is unique in the shared domain.
> + *
> + * For non-shared-domain case, fscache_acquire_volume() invoked by
> + * erofs_fscache_register_volume() has already guaranteed
> + * the uniqueness of primary data blob.
> + *
> + * Acquired domain/volume will be relinquished in kill_sb() on error.
> + */
> + flags = EROFS_REG_COOKIE_NEED_INODE;
> + if (sbi->domain_id)
> + flags |= EROFS_REG_COOKIE_NEED_NOEXIST;
> + fscache = erofs_fscache_register_cookie(sb, sbi->fsid, flags);
> if (IS_ERR(fscache))
> return PTR_ERR(fscache);
>
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 05dc68627722..a5f8c8fcdd17 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -604,13 +604,18 @@ static inline int z_erofs_load_lzma_config(struct super_block *sb,
> }
> #endif /* !CONFIG_EROFS_FS_ZIP */
>
> +/* flags for erofs_fscache_register_cookie() */
> +#define EROFS_REG_COOKIE_NEED_INODE 1
> +#define EROFS_REG_COOKIE_NEED_NOEXIST 2
How about using TAB to align the columns?
Apart from this, LGTM.
Reviewed-by: Jia Zhu <[email protected]>
Thanks.
> +
> /* fscache.c */
> #ifdef CONFIG_EROFS_FS_ONDEMAND
> int erofs_fscache_register_fs(struct super_block *sb);
> void erofs_fscache_unregister_fs(struct super_block *sb);
>
> struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
> - char *name, bool need_inode);
> + char *name,
> + unsigned int flags);
> void erofs_fscache_unregister_cookie(struct erofs_fscache *fscache);
>
> extern const struct address_space_operations erofs_fscache_access_aops;
> @@ -623,7 +628,8 @@ static inline void erofs_fscache_unregister_fs(struct super_block *sb) {}
>
> static inline
> struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
> - char *name, bool need_inode)
> + char *name,
> + unsigned int flags)
> {
> return ERR_PTR(-EOPNOTSUPP);
> }
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 1c7dcca702b3..481788c24a68 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -245,7 +245,7 @@ static int erofs_init_device(struct erofs_buf *buf, struct super_block *sb,
> }
>
> if (erofs_is_fscache_mode(sb)) {
> - fscache = erofs_fscache_register_cookie(sb, dif->path, false);
> + fscache = erofs_fscache_register_cookie(sb, dif->path, 0);
> if (IS_ERR(fscache))
> return PTR_ERR(fscache);
> dif->fscache = fscache;
Hi,
On 11/25/2022 4:25 PM, Jia Zhu wrote:
> Hi Tao,
> We've noticed this warning during development. It seems not a bug, so
> I ignored that.
> Many thanks for cacthing and clearing up the annoying warning.
Yes, it is not a bug. The duplicated mount will fail with -EEXIST even without
the patch. But it is a bit scary to see such warning in dmesg, so just fix it.
>
> 在 2022/11/25 15:40, Hou Tao 写道:
>> From: Hou Tao <[email protected]>
>>
>> When shared domain is enabled, doing mount twice with the same fsid and
>> domain_id will trigger sysfs warning as shown below:
>>
>> sysfs: cannot create duplicate filename '/fs/erofs/d0,meta.bin'
>> CPU: 15 PID: 1051 Comm: mount Not tainted 6.1.0-rc6+ #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>> Call Trace:
>> <TASK>
>> dump_stack_lvl+0x38/0x49
>> dump_stack+0x10/0x12
>> sysfs_warn_dup.cold+0x17/0x27
>> sysfs_create_dir_ns+0xb8/0xd0
>> kobject_add_internal+0xb1/0x240
>> kobject_init_and_add+0x71/0xa0
>> erofs_register_sysfs+0x89/0x110
>> erofs_fc_fill_super+0x98c/0xaf0
>> vfs_get_super+0x7d/0x100
>> get_tree_nodev+0x16/0x20
>> erofs_fc_get_tree+0x20/0x30
>> vfs_get_tree+0x24/0xb0
>> path_mount+0x2fa/0xa90
>> do_mount+0x7c/0xa0
>> __x64_sys_mount+0x8b/0xe0
>> do_syscall_64+0x30/0x60
>> entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>
>> The reason is erofs_fscache_register_cookie() doesn't guarantee the primary
>> data blob (aka fsid) is unique in the shared domain and
>> erofs_register_sysfs() invoked by the second mount will fail due to the
>> duplicated fsid in the shared domain and report warning.
>>
>> It would be better to check the uniqueness of fsid before doing
>> erofs_register_sysfs(), so adding a new flags parameter for
>> erofs_fscache_register_cookie() and doing the uniqueness check if
>> EROFS_REG_COOKIE_NEED_NOEXIST is enabled.
>>
>> After the patch, the error in dmesg for the duplicated mount would be:
>>
>> erofs: ...: erofs_domain_register_cookie: XX already exists in domain YY
>>
>> Signed-off-by: Hou Tao <[email protected]>
SNIP
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index 05dc68627722..a5f8c8fcdd17 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -604,13 +604,18 @@ static inline int z_erofs_load_lzma_config(struct
>> super_block *sb,
>> }
>> #endif /* !CONFIG_EROFS_FS_ZIP */
>> +/* flags for erofs_fscache_register_cookie() */
>> +#define EROFS_REG_COOKIE_NEED_INODE 1
>> +#define EROFS_REG_COOKIE_NEED_NOEXIST 2
> How about using TAB to align the columns?
> Apart from this, LGTM.
> Reviewed-by: Jia Zhu <[email protected]>
> Thanks.
Thanks for the review. Will do the alignment in v2.
>> +
>> /* fscache.c */
>> #ifdef CONFIG_EROFS_FS_ONDEMAND
>> int erofs_fscache_register_fs(struct super_block *sb);
>> void erofs_fscache_unregister_fs(struct super_block *sb);
>> struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
>> - char *name, bool need_inode);
>> + char *name,
>> + unsigned int flags);
>> void erofs_fscache_unregister_cookie(struct erofs_fscache *fscache);
>> extern const struct address_space_operations erofs_fscache_access_aops;
>> @@ -623,7 +628,8 @@ static inline void erofs_fscache_unregister_fs(struct
>> super_block *sb) {}
>> static inline
>> struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
>> - char *name, bool need_inode)
>> + char *name,
>> + unsigned int flags)
>> {
>> return ERR_PTR(-EOPNOTSUPP);
>> }
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 1c7dcca702b3..481788c24a68 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -245,7 +245,7 @@ static int erofs_init_device(struct erofs_buf *buf,
>> struct super_block *sb,
>> }
>> if (erofs_is_fscache_mode(sb)) {
>> - fscache = erofs_fscache_register_cookie(sb, dif->path, false);
>> + fscache = erofs_fscache_register_cookie(sb, dif->path, 0);
>> if (IS_ERR(fscache))
>> return PTR_ERR(fscache);
>> dif->fscache = fscache;
> .