Re: [PATCH v2] erofs: fix lockdep false positives on initializing erofs_pseudo_mnt
On 2024/3/7 22:18, Gao Xiang wrote: On 2024/3/7 18:10, Baokun Li wrote: Lockdep reported the following issue when mounting erofs with a domain_id: WARNING: possible recursive locking detected 6.8.0-rc7-xfstests #521 Not tainted mount/396 is trying to acquire lock: 907a80e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 but task is already holding lock: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 lock(>s_umount_key#50/1); lock(>s_umount_key#50/1); *** DEADLOCK *** May be due to missing lock nesting notation 2 locks held by mount/396: #0: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 #1: c00e6f28 (erofs_domain_list_lock){+.+.}-{3:3}, at: erofs_fscache_register_fs+0x3d/0x270 [erofs] stack backtrace: CPU: 1 PID: 396 Comm: mount Not tainted 6.8.0-rc7-xfstests #521 Call Trace: dump_stack_lvl+0x64/0xb0 validate_chain+0x5c4/0xa00 __lock_acquire+0x6a9/0xd50 lock_acquire+0xcd/0x2b0 down_write_nested+0x45/0xd0 alloc_super+0xe3/0x3d0 sget_fc+0x62/0x2f0 vfs_get_super+0x21/0x90 vfs_get_tree+0x2c/0xf0 fc_mount+0x12/0x40 vfs_kern_mount.part.0+0x75/0x90 kern_mount+0x24/0x40 erofs_fscache_register_fs+0x1ef/0x270 [erofs] erofs_fc_fill_super+0x213/0x380 [erofs] This is because the file_system_type of both erofs and the pseudo-mount point of domain_id is erofs_fs_type, so two successive calls to alloc_super() are considered to be using the same lock and trigger the warning above. Therefore add a nodev file_system_type called erofs_anon_fs_type in fscache.c to silence this complaint. Because kern_mount() takes a pointer to struct file_system_type, not its (string) name. So we don't need to call register_filesystem(). In addition, call init_pseudo() in erofs_anon_init_fs_context() as suggested by Al Viro, so that we can remove erofs_fc_fill_pseudo_super(), erofs_fc_anon_get_tree(), and erofs_anon_context_ops. Signed-off-by: Baokun Li I will add Suggested-by: Al Viro when applying.. Okay, thanks for adding it. Also since it's a false positive and too close to the final so let's keep this patch in -next for a while and then aim for v6.9-rc1 instead. Thanks, Gao Xiang Fine! Thanks! -- With Best Regards, Baokun Li .
Re: [PATCH v2] erofs: fix lockdep false positives on initializing erofs_pseudo_mnt
On 2024/3/7 18:10, Baokun Li wrote: Lockdep reported the following issue when mounting erofs with a domain_id: WARNING: possible recursive locking detected 6.8.0-rc7-xfstests #521 Not tainted mount/396 is trying to acquire lock: 907a80e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 but task is already holding lock: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 lock(>s_umount_key#50/1); lock(>s_umount_key#50/1); *** DEADLOCK *** May be due to missing lock nesting notation 2 locks held by mount/396: #0: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 #1: c00e6f28 (erofs_domain_list_lock){+.+.}-{3:3}, at: erofs_fscache_register_fs+0x3d/0x270 [erofs] stack backtrace: CPU: 1 PID: 396 Comm: mount Not tainted 6.8.0-rc7-xfstests #521 Call Trace: dump_stack_lvl+0x64/0xb0 validate_chain+0x5c4/0xa00 __lock_acquire+0x6a9/0xd50 lock_acquire+0xcd/0x2b0 down_write_nested+0x45/0xd0 alloc_super+0xe3/0x3d0 sget_fc+0x62/0x2f0 vfs_get_super+0x21/0x90 vfs_get_tree+0x2c/0xf0 fc_mount+0x12/0x40 vfs_kern_mount.part.0+0x75/0x90 kern_mount+0x24/0x40 erofs_fscache_register_fs+0x1ef/0x270 [erofs] erofs_fc_fill_super+0x213/0x380 [erofs] This is because the file_system_type of both erofs and the pseudo-mount point of domain_id is erofs_fs_type, so two successive calls to alloc_super() are considered to be using the same lock and trigger the warning above. Therefore add a nodev file_system_type called erofs_anon_fs_type in fscache.c to silence this complaint. Because kern_mount() takes a pointer to struct file_system_type, not its (string) name. So we don't need to call register_filesystem(). In addition, call init_pseudo() in erofs_anon_init_fs_context() as suggested by Al Viro, so that we can remove erofs_fc_fill_pseudo_super(), erofs_fc_anon_get_tree(), and erofs_anon_context_ops. Signed-off-by: Baokun Li I will add Suggested-by: Al Viro when applying.. Also since it's a false positive and too close to the final so let's keep this patch in -next for a while and then aim for v6.9-rc1 instead. Thanks, Gao Xiang
Re: [PATCH v2] erofs: fix lockdep false positives on initializing erofs_pseudo_mnt
On 3/7/24 6:10 PM, Baokun Li wrote: > Lockdep reported the following issue when mounting erofs with a domain_id: > > > WARNING: possible recursive locking detected > 6.8.0-rc7-xfstests #521 Not tainted > > mount/396 is trying to acquire lock: > 907a80e0 (>s_umount_key#50/1){+.+.}-{3:3}, > at: alloc_super+0xe3/0x3d0 > > but task is already holding lock: > 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, > at: alloc_super+0xe3/0x3d0 > > other info that might help us debug this: > Possible unsafe locking scenario: > >CPU0 > > lock(>s_umount_key#50/1); > lock(>s_umount_key#50/1); > > *** DEADLOCK *** > > May be due to missing lock nesting notation > > 2 locks held by mount/396: > #0: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, > at: alloc_super+0xe3/0x3d0 > #1: c00e6f28 (erofs_domain_list_lock){+.+.}-{3:3}, > at: erofs_fscache_register_fs+0x3d/0x270 [erofs] > > stack backtrace: > CPU: 1 PID: 396 Comm: mount Not tainted 6.8.0-rc7-xfstests #521 > Call Trace: > > dump_stack_lvl+0x64/0xb0 > validate_chain+0x5c4/0xa00 > __lock_acquire+0x6a9/0xd50 > lock_acquire+0xcd/0x2b0 > down_write_nested+0x45/0xd0 > alloc_super+0xe3/0x3d0 > sget_fc+0x62/0x2f0 > vfs_get_super+0x21/0x90 > vfs_get_tree+0x2c/0xf0 > fc_mount+0x12/0x40 > vfs_kern_mount.part.0+0x75/0x90 > kern_mount+0x24/0x40 > erofs_fscache_register_fs+0x1ef/0x270 [erofs] > erofs_fc_fill_super+0x213/0x380 [erofs] > > This is because the file_system_type of both erofs and the pseudo-mount > point of domain_id is erofs_fs_type, so two successive calls to > alloc_super() are considered to be using the same lock and trigger the > warning above. > > Therefore add a nodev file_system_type called erofs_anon_fs_type in > fscache.c to silence this complaint. Because kern_mount() takes a > pointer to struct file_system_type, not its (string) name. So we don't > need to call register_filesystem(). In addition, call init_pseudo() in > erofs_anon_init_fs_context() as suggested by Al Viro, so that we can > remove erofs_fc_fill_pseudo_super(), erofs_fc_anon_get_tree(), and > erofs_anon_context_ops. > > Signed-off-by: Baokun Li LGTM. Reviewed-and-tested-by: Jingbo Xu > --- > V1->V2: > Modified as suggested by Al Viro to simplify the code. > > fs/erofs/fscache.c | 15 ++- > fs/erofs/internal.h | 1 - > fs/erofs/super.c| 30 +- > 3 files changed, 15 insertions(+), 31 deletions(-) > > diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c > index 89a7c2453aae..122a4753ecea 100644 > --- a/fs/erofs/fscache.c > +++ b/fs/erofs/fscache.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2022, Alibaba Cloud > * Copyright (C) 2022, Bytedance Inc. All rights reserved. > */ > +#include > #include > #include "internal.h" > > @@ -12,6 +13,18 @@ static LIST_HEAD(erofs_domain_list); > static LIST_HEAD(erofs_domain_cookies_list); > static struct vfsmount *erofs_pseudo_mnt; > > +static int erofs_anon_init_fs_context(struct fs_context *fc) > +{ > + return init_pseudo(fc, EROFS_SUPER_MAGIC) ? 0 : -ENOMEM; > +} > + > +static struct file_system_type erofs_anon_fs_type = { > + .owner = THIS_MODULE, > + .name = "pseudo_erofs", > + .init_fs_context = erofs_anon_init_fs_context, > + .kill_sb= kill_anon_super, > +}; > + > struct erofs_fscache_request { > struct erofs_fscache_request *primary; > struct netfs_cache_resources cache_resources; > @@ -381,7 +394,7 @@ static int erofs_fscache_init_domain(struct super_block > *sb) > goto out; > > if (!erofs_pseudo_mnt) { > - struct vfsmount *mnt = kern_mount(_fs_type); > + struct vfsmount *mnt = kern_mount(_anon_fs_type); > if (IS_ERR(mnt)) { > err = PTR_ERR(mnt); > goto out; > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h > index 0f0706325b7b..701d4eec693a 100644 > --- a/fs/erofs/internal.h > +++ b/fs/erofs/internal.h > @@ -385,7 +385,6 @@ struct erofs_map_dev { > unsigned int m_deviceid; > }; > > -extern struct file_system_type erofs_fs_type; > extern const struct super_operations erofs_sops; > > extern const struct address_space_operations erofs_raw_access_aops; > diff --git a/fs/erofs/super.c b/fs/erofs/super.c > index 9b4b66dcdd4f..6fbb1fba2d31 100644 > --- a/fs/erofs/super.c > +++ b/fs/erofs/super.c > @@ -579,13 +579,6 @@ static const struct export_operations erofs_export_ops = > { > .get_parent = erofs_get_parent, > }; > > -static int erofs_fc_fill_pseudo_super(struct super_block *sb, struct > fs_context *fc) > -{ > - static const struct tree_descr empty_descr = {""}; > - > - return
Re: [PATCH v2] erofs: fix lockdep false positives on initializing erofs_pseudo_mnt
LGTM Reviewed-by: Yang Erkun 在 2024/3/7 18:10, Baokun Li 写道: Lockdep reported the following issue when mounting erofs with a domain_id: WARNING: possible recursive locking detected 6.8.0-rc7-xfstests #521 Not tainted mount/396 is trying to acquire lock: 907a80e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 but task is already holding lock: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 lock(>s_umount_key#50/1); lock(>s_umount_key#50/1); *** DEADLOCK *** May be due to missing lock nesting notation 2 locks held by mount/396: #0: 907a8aaa90e0 (>s_umount_key#50/1){+.+.}-{3:3}, at: alloc_super+0xe3/0x3d0 #1: c00e6f28 (erofs_domain_list_lock){+.+.}-{3:3}, at: erofs_fscache_register_fs+0x3d/0x270 [erofs] stack backtrace: CPU: 1 PID: 396 Comm: mount Not tainted 6.8.0-rc7-xfstests #521 Call Trace: dump_stack_lvl+0x64/0xb0 validate_chain+0x5c4/0xa00 __lock_acquire+0x6a9/0xd50 lock_acquire+0xcd/0x2b0 down_write_nested+0x45/0xd0 alloc_super+0xe3/0x3d0 sget_fc+0x62/0x2f0 vfs_get_super+0x21/0x90 vfs_get_tree+0x2c/0xf0 fc_mount+0x12/0x40 vfs_kern_mount.part.0+0x75/0x90 kern_mount+0x24/0x40 erofs_fscache_register_fs+0x1ef/0x270 [erofs] erofs_fc_fill_super+0x213/0x380 [erofs] This is because the file_system_type of both erofs and the pseudo-mount point of domain_id is erofs_fs_type, so two successive calls to alloc_super() are considered to be using the same lock and trigger the warning above. Therefore add a nodev file_system_type called erofs_anon_fs_type in fscache.c to silence this complaint. Because kern_mount() takes a pointer to struct file_system_type, not its (string) name. So we don't need to call register_filesystem(). In addition, call init_pseudo() in erofs_anon_init_fs_context() as suggested by Al Viro, so that we can remove erofs_fc_fill_pseudo_super(), erofs_fc_anon_get_tree(), and erofs_anon_context_ops. Signed-off-by: Baokun Li --- V1->V2: Modified as suggested by Al Viro to simplify the code. fs/erofs/fscache.c | 15 ++- fs/erofs/internal.h | 1 - fs/erofs/super.c| 30 +- 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c index 89a7c2453aae..122a4753ecea 100644 --- a/fs/erofs/fscache.c +++ b/fs/erofs/fscache.c @@ -3,6 +3,7 @@ * Copyright (C) 2022, Alibaba Cloud * Copyright (C) 2022, Bytedance Inc. All rights reserved. */ +#include #include #include "internal.h" @@ -12,6 +13,18 @@ static LIST_HEAD(erofs_domain_list); static LIST_HEAD(erofs_domain_cookies_list); static struct vfsmount *erofs_pseudo_mnt; +static int erofs_anon_init_fs_context(struct fs_context *fc) +{ + return init_pseudo(fc, EROFS_SUPER_MAGIC) ? 0 : -ENOMEM; +} + +static struct file_system_type erofs_anon_fs_type = { + .owner = THIS_MODULE, + .name = "pseudo_erofs", + .init_fs_context = erofs_anon_init_fs_context, + .kill_sb= kill_anon_super, +}; + struct erofs_fscache_request { struct erofs_fscache_request *primary; struct netfs_cache_resources cache_resources; @@ -381,7 +394,7 @@ static int erofs_fscache_init_domain(struct super_block *sb) goto out; if (!erofs_pseudo_mnt) { - struct vfsmount *mnt = kern_mount(_fs_type); + struct vfsmount *mnt = kern_mount(_anon_fs_type); if (IS_ERR(mnt)) { err = PTR_ERR(mnt); goto out; diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 0f0706325b7b..701d4eec693a 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -385,7 +385,6 @@ struct erofs_map_dev { unsigned int m_deviceid; }; -extern struct file_system_type erofs_fs_type; extern const struct super_operations erofs_sops; extern const struct address_space_operations erofs_raw_access_aops; diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 9b4b66dcdd4f..6fbb1fba2d31 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -579,13 +579,6 @@ static const struct export_operations erofs_export_ops = { .get_parent = erofs_get_parent, }; -static int erofs_fc_fill_pseudo_super(struct super_block *sb, struct fs_context *fc) -{ - static const struct tree_descr empty_descr = {""}; - - return simple_fill_super(sb, EROFS_SUPER_MAGIC, _descr); -} - static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) { struct inode *inode; @@ -712,11 +705,6 @@ static int