fs/erofs/super.c | 51 ++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 23 deletions(-)
When erofs_kill_sb() is called in block dev based mode, s_bdev may not have
been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will
be mistaken for fscache mode, and then attempt to free an anon_dev that has
never been allocated, triggering the following warning:
============================================
ida_free called for id=0 which is not allocated.
WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140
Modules linked in:
CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630
RIP: 0010:ida_free+0x134/0x140
Call Trace:
<TASK>
erofs_kill_sb+0x81/0x90
deactivate_locked_super+0x35/0x80
get_tree_bdev+0x136/0x1e0
vfs_get_tree+0x2c/0xf0
do_new_mount+0x190/0x2f0
[...]
============================================
Instead of allocating the erofs_sb_info in fill_super() allocate it
during erofs_get_tree() and ensure that erofs can always have the info
available during erofs_kill_sb().
Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
Changes since v1:
Allocate and initialise fc->s_fs_info in erofs_fc_get_tree() instead of
modifying fc->sb_flags.
V1: https://lore.kernel.org/r/20240415121746.1207242-1-libaokun1@huawei.com/
fs/erofs/super.c | 51 ++++++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 23 deletions(-)
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index b21bd8f78dc1..4104280be2ea 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -581,8 +581,7 @@ static const struct export_operations erofs_export_ops = {
static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
{
struct inode *inode;
- struct erofs_sb_info *sbi;
- struct erofs_fs_context *ctx = fc->fs_private;
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
int err;
sb->s_magic = EROFS_SUPER_MAGIC;
@@ -590,19 +589,6 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
sb->s_maxbytes = MAX_LFS_FILESIZE;
sb->s_op = &erofs_sops;
- sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
- if (!sbi)
- return -ENOMEM;
-
- sb->s_fs_info = sbi;
- sbi->opt = ctx->opt;
- sbi->devs = ctx->devs;
- ctx->devs = NULL;
- sbi->fsid = ctx->fsid;
- ctx->fsid = NULL;
- sbi->domain_id = ctx->domain_id;
- ctx->domain_id = NULL;
-
sbi->blkszbits = PAGE_SHIFT;
if (erofs_is_fscache_mode(sb)) {
sb->s_blocksize = PAGE_SIZE;
@@ -704,11 +690,32 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
return 0;
}
-static int erofs_fc_get_tree(struct fs_context *fc)
+static void erofs_ctx_to_info(struct fs_context *fc)
{
struct erofs_fs_context *ctx = fc->fs_private;
+ struct erofs_sb_info *sbi = fc->s_fs_info;
+
+ sbi->opt = ctx->opt;
+ sbi->devs = ctx->devs;
+ ctx->devs = NULL;
+ sbi->fsid = ctx->fsid;
+ ctx->fsid = NULL;
+ sbi->domain_id = ctx->domain_id;
+ ctx->domain_id = NULL;
+}
- if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
+static int erofs_fc_get_tree(struct fs_context *fc)
+{
+ struct erofs_sb_info *sbi;
+
+ sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
+ if (!sbi)
+ return -ENOMEM;
+
+ fc->s_fs_info = sbi;
+ erofs_ctx_to_info(fc);
+
+ if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
return get_tree_nodev(fc, erofs_fc_fill_super);
return get_tree_bdev(fc, erofs_fc_fill_super);
@@ -767,6 +774,7 @@ static void erofs_fc_free(struct fs_context *fc)
kfree(ctx->fsid);
kfree(ctx->domain_id);
kfree(ctx);
+ kfree(fc->s_fs_info);
}
static const struct fs_context_operations erofs_context_ops = {
@@ -783,6 +791,7 @@ static int erofs_init_fs_context(struct fs_context *fc)
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
+
ctx->devs = kzalloc(sizeof(struct erofs_dev_context), GFP_KERNEL);
if (!ctx->devs) {
kfree(ctx);
@@ -799,17 +808,13 @@ static int erofs_init_fs_context(struct fs_context *fc)
static void erofs_kill_sb(struct super_block *sb)
{
- struct erofs_sb_info *sbi;
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
- if (erofs_is_fscache_mode(sb))
+ if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid)
kill_anon_super(sb);
else
kill_block_super(sb);
- sbi = EROFS_SB(sb);
- if (!sbi)
- return;
-
erofs_free_dev_context(sbi->devs);
fs_put_dax(sbi->dax_dev, NULL);
erofs_fscache_unregister_fs(sb);
--
2.31.1
Hi Baokun, Thanks for catching this and move forward fixing this! On 4/17/24 2:55 PM, Baokun Li wrote: > When erofs_kill_sb() is called in block dev based mode, s_bdev may not have > been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will > be mistaken for fscache mode, and then attempt to free an anon_dev that has > never been allocated, triggering the following warning: > > ============================================ > ida_free called for id=0 which is not allocated. > WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140 > Modules linked in: > CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630 > RIP: 0010:ida_free+0x134/0x140 > Call Trace: > <TASK> > erofs_kill_sb+0x81/0x90 > deactivate_locked_super+0x35/0x80 > get_tree_bdev+0x136/0x1e0 > vfs_get_tree+0x2c/0xf0 > do_new_mount+0x190/0x2f0 > [...] > ============================================ > > Instead of allocating the erofs_sb_info in fill_super() allocate it > during erofs_get_tree() and ensure that erofs can always have the info > available during erofs_kill_sb(). I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will be better, as I see some filesystems (e.g. autofs) do this way. Maybe another potential advantage of doing this way is that erofs_fs_context is not needed anymore and we can use sbi directly. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > Signed-off-by: Baokun Li <libaokun1@huawei.com> > --- > Changes since v1: > Allocate and initialise fc->s_fs_info in erofs_fc_get_tree() instead of > modifying fc->sb_flags. > > V1: https://lore.kernel.org/r/20240415121746.1207242-1-libaokun1@huawei.com/ > > fs/erofs/super.c | 51 ++++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 23 deletions(-) > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c > index b21bd8f78dc1..4104280be2ea 100644 > --- a/fs/erofs/super.c > +++ b/fs/erofs/super.c > @@ -581,8 +581,7 @@ static const struct export_operations erofs_export_ops = { > static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) > { > struct inode *inode; > - struct erofs_sb_info *sbi; > - struct erofs_fs_context *ctx = fc->fs_private; > + struct erofs_sb_info *sbi = EROFS_SB(sb); > int err; > > sb->s_magic = EROFS_SUPER_MAGIC; > @@ -590,19 +589,6 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) > sb->s_maxbytes = MAX_LFS_FILESIZE; > sb->s_op = &erofs_sops; > > - sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); > - if (!sbi) > - return -ENOMEM; > - > - sb->s_fs_info = sbi; > - sbi->opt = ctx->opt; > - sbi->devs = ctx->devs; > - ctx->devs = NULL; > - sbi->fsid = ctx->fsid; > - ctx->fsid = NULL; > - sbi->domain_id = ctx->domain_id; > - ctx->domain_id = NULL; > - > sbi->blkszbits = PAGE_SHIFT; > if (erofs_is_fscache_mode(sb)) { > sb->s_blocksize = PAGE_SIZE; > @@ -704,11 +690,32 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) > return 0; > } > > -static int erofs_fc_get_tree(struct fs_context *fc) > +static void erofs_ctx_to_info(struct fs_context *fc) > { > struct erofs_fs_context *ctx = fc->fs_private; > + struct erofs_sb_info *sbi = fc->s_fs_info; > + > + sbi->opt = ctx->opt; > + sbi->devs = ctx->devs; > + ctx->devs = NULL; > + sbi->fsid = ctx->fsid; > + ctx->fsid = NULL; > + sbi->domain_id = ctx->domain_id; > + ctx->domain_id = NULL; > +} I'm not sure if abstracting this logic into a seperate helper really helps understanding the code as the logic itself is quite simple and easy to be understood. Usually it's a hint of over-abstraction when a simple helper has only one caller. > > - if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid) > +static int erofs_fc_get_tree(struct fs_context *fc) > +{ > + struct erofs_sb_info *sbi; > + > + sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); > + if (!sbi) > + return -ENOMEM; > + > + fc->s_fs_info = sbi; > + erofs_ctx_to_info(fc); > + > + if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid) > return get_tree_nodev(fc, erofs_fc_fill_super); > > return get_tree_bdev(fc, erofs_fc_fill_super); > @@ -767,6 +774,7 @@ static void erofs_fc_free(struct fs_context *fc) > kfree(ctx->fsid); > kfree(ctx->domain_id); > kfree(ctx); > + kfree(fc->s_fs_info); > } > > static const struct fs_context_operations erofs_context_ops = { > @@ -783,6 +791,7 @@ static int erofs_init_fs_context(struct fs_context *fc) > ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > if (!ctx) > return -ENOMEM; > + > ctx->devs = kzalloc(sizeof(struct erofs_dev_context), GFP_KERNEL); > if (!ctx->devs) { > kfree(ctx); > @@ -799,17 +808,13 @@ static int erofs_init_fs_context(struct fs_context *fc) > > static void erofs_kill_sb(struct super_block *sb) > { > - struct erofs_sb_info *sbi; > + struct erofs_sb_info *sbi = EROFS_SB(sb); > > - if (erofs_is_fscache_mode(sb)) > + if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid) > kill_anon_super(sb); > else > kill_block_super(sb); > > - sbi = EROFS_SB(sb); > - if (!sbi) > - return; > - > erofs_free_dev_context(sbi->devs); > fs_put_dax(sbi->dax_dev, NULL); > erofs_fscache_unregister_fs(sb); -- Thanks, Jingbo
On 2024/4/18 10:16, Jingbo Xu wrote: > Hi Baokun, > > Thanks for catching this and move forward fixing this! Hi Jingbo, Thanks for your review! > > On 4/17/24 2:55 PM, Baokun Li wrote: >> When erofs_kill_sb() is called in block dev based mode, s_bdev may not have >> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it will >> be mistaken for fscache mode, and then attempt to free an anon_dev that has >> never been allocated, triggering the following warning: >> >> ============================================ >> ida_free called for id=0 which is not allocated. >> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140 >> Modules linked in: >> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630 >> RIP: 0010:ida_free+0x134/0x140 >> Call Trace: >> <TASK> >> erofs_kill_sb+0x81/0x90 >> deactivate_locked_super+0x35/0x80 >> get_tree_bdev+0x136/0x1e0 >> vfs_get_tree+0x2c/0xf0 >> do_new_mount+0x190/0x2f0 >> [...] >> ============================================ >> >> Instead of allocating the erofs_sb_info in fill_super() allocate it >> during erofs_get_tree() and ensure that erofs can always have the info >> available during erofs_kill_sb(). > > I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will > be better, as I see some filesystems (e.g. autofs) do this way. Maybe > another potential advantage of doing this way is that erofs_fs_context > is not needed anymore and we can use sbi directly. Yes, except for some extra memory usage when remounting, this idea sounds great. Let me send a version of v3 to get rid of erofs_fs_context. > >> Signed-off-by: Christian Brauner <brauner@kernel.org> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> --- >> Changes since v1: >> Allocate and initialise fc->s_fs_info in erofs_fc_get_tree() instead of >> modifying fc->sb_flags. >> >> V1: https://lore.kernel.org/r/20240415121746.1207242-1-libaokun1@huawei.com/ >> >> fs/erofs/super.c | 51 ++++++++++++++++++++++++++---------------------- >> 1 file changed, 28 insertions(+), 23 deletions(-) >> >> diff --git a/fs/erofs/super.c b/fs/erofs/super.c >> index b21bd8f78dc1..4104280be2ea 100644 >> --- a/fs/erofs/super.c >> +++ b/fs/erofs/super.c >> @@ -581,8 +581,7 @@ static const struct export_operations erofs_export_ops = { >> static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) >> { >> struct inode *inode; >> - struct erofs_sb_info *sbi; >> - struct erofs_fs_context *ctx = fc->fs_private; >> + struct erofs_sb_info *sbi = EROFS_SB(sb); >> int err; >> >> sb->s_magic = EROFS_SUPER_MAGIC; >> @@ -590,19 +589,6 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) >> sb->s_maxbytes = MAX_LFS_FILESIZE; >> sb->s_op = &erofs_sops; >> >> - sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); >> - if (!sbi) >> - return -ENOMEM; >> - >> - sb->s_fs_info = sbi; >> - sbi->opt = ctx->opt; >> - sbi->devs = ctx->devs; >> - ctx->devs = NULL; >> - sbi->fsid = ctx->fsid; >> - ctx->fsid = NULL; >> - sbi->domain_id = ctx->domain_id; >> - ctx->domain_id = NULL; >> - >> sbi->blkszbits = PAGE_SHIFT; >> if (erofs_is_fscache_mode(sb)) { >> sb->s_blocksize = PAGE_SIZE; >> @@ -704,11 +690,32 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc) >> return 0; >> } >> >> -static int erofs_fc_get_tree(struct fs_context *fc) >> +static void erofs_ctx_to_info(struct fs_context *fc) >> { >> struct erofs_fs_context *ctx = fc->fs_private; >> + struct erofs_sb_info *sbi = fc->s_fs_info; >> + >> + sbi->opt = ctx->opt; >> + sbi->devs = ctx->devs; >> + ctx->devs = NULL; >> + sbi->fsid = ctx->fsid; >> + ctx->fsid = NULL; >> + sbi->domain_id = ctx->domain_id; >> + ctx->domain_id = NULL; >> +} > I'm not sure if abstracting this logic into a seperate helper really > helps understanding the code as the logic itself is quite simple and > easy to be understood. Usually it's a hint of over-abstraction when a > simple helper has only one caller. > Static functions that have only one caller are compiled inline, so we don't have to worry about how that affects the code. The reason these codes are encapsulated in a separate function is so that the code reader understands that these codes are integrated as a whole, and that we shouldn't have to move one or two of these lines individually. But after we get rid of erofs_fs_context, those won't be needed anymore. >> >> - if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid) >> +static int erofs_fc_get_tree(struct fs_context *fc) >> +{ >> + struct erofs_sb_info *sbi; >> + >> + sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); >> + if (!sbi) >> + return -ENOMEM; >> + >> + fc->s_fs_info = sbi; >> + erofs_ctx_to_info(fc); >> + >> + if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid) >> return get_tree_nodev(fc, erofs_fc_fill_super); >> >> return get_tree_bdev(fc, erofs_fc_fill_super); >> @@ -767,6 +774,7 @@ static void erofs_fc_free(struct fs_context *fc) >> kfree(ctx->fsid); >> kfree(ctx->domain_id); >> kfree(ctx); >> + kfree(fc->s_fs_info); >> } >> >> static const struct fs_context_operations erofs_context_ops = { >> @@ -783,6 +791,7 @@ static int erofs_init_fs_context(struct fs_context *fc) >> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); >> if (!ctx) >> return -ENOMEM; >> + >> ctx->devs = kzalloc(sizeof(struct erofs_dev_context), GFP_KERNEL); >> if (!ctx->devs) { >> kfree(ctx); >> @@ -799,17 +808,13 @@ static int erofs_init_fs_context(struct fs_context *fc) >> >> static void erofs_kill_sb(struct super_block *sb) >> { >> - struct erofs_sb_info *sbi; >> + struct erofs_sb_info *sbi = EROFS_SB(sb); >> >> - if (erofs_is_fscache_mode(sb)) >> + if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && sbi->fsid) >> kill_anon_super(sb); >> else >> kill_block_super(sb); >> >> - sbi = EROFS_SB(sb); >> - if (!sbi) >> - return; >> - >> erofs_free_dev_context(sbi->devs); >> fs_put_dax(sbi->dax_dev, NULL); >> erofs_fscache_unregister_fs(sb); -- With Best Regards, Baokun Li
On 4/18/24 11:36 AM, Baokun Li wrote: > On 2024/4/18 10:16, Jingbo Xu wrote: >> Hi Baokun, >> >> Thanks for catching this and move forward fixing this! > > Hi Jingbo, > > Thanks for your review! > >> >> On 4/17/24 2:55 PM, Baokun Li wrote: >>> When erofs_kill_sb() is called in block dev based mode, s_bdev may >>> not have >>> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it >>> will >>> be mistaken for fscache mode, and then attempt to free an anon_dev >>> that has >>> never been allocated, triggering the following warning: >>> >>> ============================================ >>> ida_free called for id=0 which is not allocated. >>> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140 >>> Modules linked in: >>> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630 >>> RIP: 0010:ida_free+0x134/0x140 >>> Call Trace: >>> <TASK> >>> erofs_kill_sb+0x81/0x90 >>> deactivate_locked_super+0x35/0x80 >>> get_tree_bdev+0x136/0x1e0 >>> vfs_get_tree+0x2c/0xf0 >>> do_new_mount+0x190/0x2f0 >>> [...] >>> ============================================ >>> >>> Instead of allocating the erofs_sb_info in fill_super() allocate it >>> during erofs_get_tree() and ensure that erofs can always have the info >>> available during erofs_kill_sb(). >> >> I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will >> be better, as I see some filesystems (e.g. autofs) do this way. Maybe >> another potential advantage of doing this way is that erofs_fs_context >> is not needed anymore and we can use sbi directly. > Yes, except for some extra memory usage when remounting, > this idea sounds great. Let me send a version of v3 to get rid > of erofs_fs_context. I'm not sure if Gao Xaing also prefers this. I think it would be better to wait and listen for his thoughts before we sending v3. >> >>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>> Signed-off-by: Baokun Li <libaokun1@huawei.com> >>> --- >>> Changes since v1: >>> Allocate and initialise fc->s_fs_info in erofs_fc_get_tree() >>> instead of >>> modifying fc->sb_flags. >>> >>> V1: >>> https://lore.kernel.org/r/20240415121746.1207242-1-libaokun1@huawei.com/ >>> >>> fs/erofs/super.c | 51 ++++++++++++++++++++++++++---------------------- >>> 1 file changed, 28 insertions(+), 23 deletions(-) >>> >>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c >>> index b21bd8f78dc1..4104280be2ea 100644 >>> --- a/fs/erofs/super.c >>> +++ b/fs/erofs/super.c >>> @@ -581,8 +581,7 @@ static const struct export_operations >>> erofs_export_ops = { >>> static int erofs_fc_fill_super(struct super_block *sb, struct >>> fs_context *fc) >>> { >>> struct inode *inode; >>> - struct erofs_sb_info *sbi; >>> - struct erofs_fs_context *ctx = fc->fs_private; >>> + struct erofs_sb_info *sbi = EROFS_SB(sb); >>> int err; >>> sb->s_magic = EROFS_SUPER_MAGIC; >>> @@ -590,19 +589,6 @@ static int erofs_fc_fill_super(struct >>> super_block *sb, struct fs_context *fc) >>> sb->s_maxbytes = MAX_LFS_FILESIZE; >>> sb->s_op = &erofs_sops; >>> - sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); >>> - if (!sbi) >>> - return -ENOMEM; >>> - >>> - sb->s_fs_info = sbi; >>> - sbi->opt = ctx->opt; >>> - sbi->devs = ctx->devs; >>> - ctx->devs = NULL; >>> - sbi->fsid = ctx->fsid; >>> - ctx->fsid = NULL; >>> - sbi->domain_id = ctx->domain_id; >>> - ctx->domain_id = NULL; >>> - >>> sbi->blkszbits = PAGE_SHIFT; >>> if (erofs_is_fscache_mode(sb)) { >>> sb->s_blocksize = PAGE_SIZE; >>> @@ -704,11 +690,32 @@ static int erofs_fc_fill_super(struct >>> super_block *sb, struct fs_context *fc) >>> return 0; >>> } >>> -static int erofs_fc_get_tree(struct fs_context *fc) >>> +static void erofs_ctx_to_info(struct fs_context *fc) >>> { >>> struct erofs_fs_context *ctx = fc->fs_private; >>> + struct erofs_sb_info *sbi = fc->s_fs_info; >>> + >>> + sbi->opt = ctx->opt; >>> + sbi->devs = ctx->devs; >>> + ctx->devs = NULL; >>> + sbi->fsid = ctx->fsid; >>> + ctx->fsid = NULL; >>> + sbi->domain_id = ctx->domain_id; >>> + ctx->domain_id = NULL; >>> +} >> I'm not sure if abstracting this logic into a seperate helper really >> helps understanding the code as the logic itself is quite simple and >> easy to be understood. Usually it's a hint of over-abstraction when a >> simple helper has only one caller. >> > Static functions that have only one caller are compiled inline, so we > don't have to worry about how that affects the code. > > The reason these codes are encapsulated in a separate function is so > that the code reader understands that these codes are integrated > as a whole, and that we shouldn't have to move one or two of these > lines individually. > > But after we get rid of erofs_fs_context, those won't be needed > anymore. Yeah, I understand. It's only coding style concerns. -- Thanks, Jingbo
On 2024/4/18 13:50, Jingbo Xu wrote: > > On 4/18/24 11:36 AM, Baokun Li wrote: >> On 2024/4/18 10:16, Jingbo Xu wrote: >>> Hi Baokun, >>> >>> Thanks for catching this and move forward fixing this! >> Hi Jingbo, >> >> Thanks for your review! >> >>> On 4/17/24 2:55 PM, Baokun Li wrote: >>>> When erofs_kill_sb() is called in block dev based mode, s_bdev may >>>> not have >>>> been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it >>>> will >>>> be mistaken for fscache mode, and then attempt to free an anon_dev >>>> that has >>>> never been allocated, triggering the following warning: >>>> >>>> ============================================ >>>> ida_free called for id=0 which is not allocated. >>>> WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140 >>>> Modules linked in: >>>> CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630 >>>> RIP: 0010:ida_free+0x134/0x140 >>>> Call Trace: >>>> <TASK> >>>> erofs_kill_sb+0x81/0x90 >>>> deactivate_locked_super+0x35/0x80 >>>> get_tree_bdev+0x136/0x1e0 >>>> vfs_get_tree+0x2c/0xf0 >>>> do_new_mount+0x190/0x2f0 >>>> [...] >>>> ============================================ >>>> >>>> Instead of allocating the erofs_sb_info in fill_super() allocate it >>>> during erofs_get_tree() and ensure that erofs can always have the info >>>> available during erofs_kill_sb(). >>> I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will >>> be better, as I see some filesystems (e.g. autofs) do this way. Maybe >>> another potential advantage of doing this way is that erofs_fs_context >>> is not needed anymore and we can use sbi directly. >> Yes, except for some extra memory usage when remounting, >> this idea sounds great. Let me send a version of v3 to get rid >> of erofs_fs_context. > I'm not sure if Gao Xaing also prefers this. I think it would be better > to wait and listen for his thoughts before we sending v3. Okay, there's no rush on this. >>>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>>> Signed-off-by: Baokun Li <libaokun1@huawei.com> >>>> --- >>>> Changes since v1: >>>> Allocate and initialise fc->s_fs_info in erofs_fc_get_tree() >>>> instead of >>>> modifying fc->sb_flags. >>>> >>>> V1: >>>> https://lore.kernel.org/r/20240415121746.1207242-1-libaokun1@huawei.com/ >>>> >>>> fs/erofs/super.c | 51 ++++++++++++++++++++++++++---------------------- >>>> 1 file changed, 28 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c >>>> index b21bd8f78dc1..4104280be2ea 100644 >>>> --- a/fs/erofs/super.c >>>> +++ b/fs/erofs/super.c >>>> @@ -581,8 +581,7 @@ static const struct export_operations >>>> erofs_export_ops = { >>>> static int erofs_fc_fill_super(struct super_block *sb, struct >>>> fs_context *fc) >>>> { >>>> struct inode *inode; >>>> - struct erofs_sb_info *sbi; >>>> - struct erofs_fs_context *ctx = fc->fs_private; >>>> + struct erofs_sb_info *sbi = EROFS_SB(sb); >>>> int err; >>>> sb->s_magic = EROFS_SUPER_MAGIC; >>>> @@ -590,19 +589,6 @@ static int erofs_fc_fill_super(struct >>>> super_block *sb, struct fs_context *fc) >>>> sb->s_maxbytes = MAX_LFS_FILESIZE; >>>> sb->s_op = &erofs_sops; >>>> - sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); >>>> - if (!sbi) >>>> - return -ENOMEM; >>>> - >>>> - sb->s_fs_info = sbi; >>>> - sbi->opt = ctx->opt; >>>> - sbi->devs = ctx->devs; >>>> - ctx->devs = NULL; >>>> - sbi->fsid = ctx->fsid; >>>> - ctx->fsid = NULL; >>>> - sbi->domain_id = ctx->domain_id; >>>> - ctx->domain_id = NULL; >>>> - >>>> sbi->blkszbits = PAGE_SHIFT; >>>> if (erofs_is_fscache_mode(sb)) { >>>> sb->s_blocksize = PAGE_SIZE; >>>> @@ -704,11 +690,32 @@ static int erofs_fc_fill_super(struct >>>> super_block *sb, struct fs_context *fc) >>>> return 0; >>>> } >>>> -static int erofs_fc_get_tree(struct fs_context *fc) >>>> +static void erofs_ctx_to_info(struct fs_context *fc) >>>> { >>>> struct erofs_fs_context *ctx = fc->fs_private; >>>> + struct erofs_sb_info *sbi = fc->s_fs_info; >>>> + >>>> + sbi->opt = ctx->opt; >>>> + sbi->devs = ctx->devs; >>>> + ctx->devs = NULL; >>>> + sbi->fsid = ctx->fsid; >>>> + ctx->fsid = NULL; >>>> + sbi->domain_id = ctx->domain_id; >>>> + ctx->domain_id = NULL; >>>> +} >>> I'm not sure if abstracting this logic into a seperate helper really >>> helps understanding the code as the logic itself is quite simple and >>> easy to be understood. Usually it's a hint of over-abstraction when a >>> simple helper has only one caller. >>> >> Static functions that have only one caller are compiled inline, so we >> don't have to worry about how that affects the code. >> >> The reason these codes are encapsulated in a separate function is so >> that the code reader understands that these codes are integrated >> as a whole, and that we shouldn't have to move one or two of these >> lines individually. >> >> But after we get rid of erofs_fs_context, those won't be needed >> anymore. > Yeah, I understand. It's only coding style concerns. > > > Okay, thanks! -- With Best Regards, Baokun Li
Hi, On Thu, Apr 18, 2024 at 02:12:39PM +0800, Baokun Li wrote: > On 2024/4/18 13:50, Jingbo Xu wrote: > > > > On 4/18/24 11:36 AM, Baokun Li wrote: > > > On 2024/4/18 10:16, Jingbo Xu wrote: > > > > Hi Baokun, > > > > > > > > Thanks for catching this and move forward fixing this! > > > Hi Jingbo, > > > > > > Thanks for your review! > > > > > > > On 4/17/24 2:55 PM, Baokun Li wrote: > > > > > When erofs_kill_sb() is called in block dev based mode, s_bdev may > > > > > not have > > > > > been initialised yet, and if CONFIG_EROFS_FS_ONDEMAND is enabled, it > > > > > will > > > > > be mistaken for fscache mode, and then attempt to free an anon_dev > > > > > that has > > > > > never been allocated, triggering the following warning: > > > > > > > > > > ============================================ > > > > > ida_free called for id=0 which is not allocated. > > > > > WARNING: CPU: 14 PID: 926 at lib/idr.c:525 ida_free+0x134/0x140 > > > > > Modules linked in: > > > > > CPU: 14 PID: 926 Comm: mount Not tainted 6.9.0-rc3-dirty #630 > > > > > RIP: 0010:ida_free+0x134/0x140 > > > > > Call Trace: > > > > > <TASK> > > > > > erofs_kill_sb+0x81/0x90 > > > > > deactivate_locked_super+0x35/0x80 > > > > > get_tree_bdev+0x136/0x1e0 > > > > > vfs_get_tree+0x2c/0xf0 > > > > > do_new_mount+0x190/0x2f0 > > > > > [...] > > > > > ============================================ > > > > > > > > > > Instead of allocating the erofs_sb_info in fill_super() allocate it > > > > > during erofs_get_tree() and ensure that erofs can always have the info > > > > > available during erofs_kill_sb(). > > > > I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will > > > > be better, as I see some filesystems (e.g. autofs) do this way. Maybe > > > > another potential advantage of doing this way is that erofs_fs_context > > > > is not needed anymore and we can use sbi directly. > > > Yes, except for some extra memory usage when remounting, > > > this idea sounds great. Let me send a version of v3 to get rid > > > of erofs_fs_context. > > I'm not sure if Gao Xaing also prefers this. I think it would be better > > to wait and listen for his thoughts before we sending v3. > Okay, there's no rush on this. I checked days ago, for example, XFS is also worked in this way. And .reconfigure() needs to be carefully handled too. > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > > > Signed-off-by: Baokun Li <libaokun1@huawei.com> > > > > > --- > > > > > Changes since v1: > > > > > Allocate and initialise fc->s_fs_info in erofs_fc_get_tree() > > > > > instead of > > > > > modifying fc->sb_flags. > > > > > > > > > > V1: > > > > > https://lore.kernel.org/r/20240415121746.1207242-1-libaokun1@huawei.com/ > > > > > > > > > > fs/erofs/super.c | 51 ++++++++++++++++++++++++++---------------------- > > > > > 1 file changed, 28 insertions(+), 23 deletions(-) > > > > > > > > > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c > > > > > index b21bd8f78dc1..4104280be2ea 100644 > > > > > --- a/fs/erofs/super.c > > > > > +++ b/fs/erofs/super.c > > > > > @@ -581,8 +581,7 @@ static const struct export_operations > > > > > erofs_export_ops = { > > > > > static int erofs_fc_fill_super(struct super_block *sb, struct > > > > > fs_context *fc) > > > > > { > > > > > struct inode *inode; > > > > > - struct erofs_sb_info *sbi; > > > > > - struct erofs_fs_context *ctx = fc->fs_private; > > > > > + struct erofs_sb_info *sbi = EROFS_SB(sb); > > > > > int err; > > > > > sb->s_magic = EROFS_SUPER_MAGIC; > > > > > @@ -590,19 +589,6 @@ static int erofs_fc_fill_super(struct > > > > > super_block *sb, struct fs_context *fc) > > > > > sb->s_maxbytes = MAX_LFS_FILESIZE; > > > > > sb->s_op = &erofs_sops; > > > > > - sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); > > > > > - if (!sbi) > > > > > - return -ENOMEM; > > > > > - > > > > > - sb->s_fs_info = sbi; > > > > > - sbi->opt = ctx->opt; > > > > > - sbi->devs = ctx->devs; > > > > > - ctx->devs = NULL; > > > > > - sbi->fsid = ctx->fsid; > > > > > - ctx->fsid = NULL; > > > > > - sbi->domain_id = ctx->domain_id; > > > > > - ctx->domain_id = NULL; > > > > > - > > > > > sbi->blkszbits = PAGE_SHIFT; > > > > > if (erofs_is_fscache_mode(sb)) { > > > > > sb->s_blocksize = PAGE_SIZE; > > > > > @@ -704,11 +690,32 @@ static int erofs_fc_fill_super(struct > > > > > super_block *sb, struct fs_context *fc) > > > > > return 0; > > > > > } > > > > > -static int erofs_fc_get_tree(struct fs_context *fc) > > > > > +static void erofs_ctx_to_info(struct fs_context *fc) > > > > > { > > > > > struct erofs_fs_context *ctx = fc->fs_private; > > > > > + struct erofs_sb_info *sbi = fc->s_fs_info; > > > > > + > > > > > + sbi->opt = ctx->opt; > > > > > + sbi->devs = ctx->devs; > > > > > + ctx->devs = NULL; > > > > > + sbi->fsid = ctx->fsid; > > > > > + ctx->fsid = NULL; > > > > > + sbi->domain_id = ctx->domain_id; > > > > > + ctx->domain_id = NULL; > > > > > +} > > > > I'm not sure if abstracting this logic into a seperate helper really > > > > helps understanding the code as the logic itself is quite simple and > > > > easy to be understood. Usually it's a hint of over-abstraction when a > > > > simple helper has only one caller. > > > > > > > Static functions that have only one caller are compiled inline, so we > > > don't have to worry about how that affects the code. > > > > > > The reason these codes are encapsulated in a separate function is so > > > that the code reader understands that these codes are integrated > > > as a whole, and that we shouldn't have to move one or two of these > > > lines individually. > > > > > > But after we get rid of erofs_fs_context, those won't be needed > > > anymore. > > Yeah, I understand. It's only coding style concerns. > > > > > > > Okay, thanks! I'm fine to get rid of those (erofs_fs_context) as long as the codebase is more clearer and simple. BTW, for the current codebase, I also think it's unneeded to have a separate helper called once without extra actual meaning... Thanks, Gao Xiang > > -- > With Best Regards, > Baokun Li
On 2024/4/18 15:49, Gao Xiang wrote: > Hi, > > On Thu, Apr 18, 2024 at 02:12:39PM +0800, Baokun Li wrote: >> On 2024/4/18 13:50, Jingbo Xu wrote: >>> On 4/18/24 11:36 AM, Baokun Li wrote: >>>> On 2024/4/18 10:16, Jingbo Xu wrote: >>>>> Hi Baokun, >>>>> >>>>> Thanks for catching this and move forward fixing this! >>>> Hi Jingbo, >>>> >>>> Thanks for your review! >>>> >>>>> On 4/17/24 2:55 PM, Baokun Li wrote: SNIP >>>>> >>>>> Instead of allocating the erofs_sb_info in fill_super() allocate it >>>>> during erofs_get_tree() and ensure that erofs can always have the info >>>>> available during erofs_kill_sb(). >>>>> I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will >>>>> be better, as I see some filesystems (e.g. autofs) do this way. Maybe >>>>> another potential advantage of doing this way is that erofs_fs_context >>>>> is not needed anymore and we can use sbi directly. >>>> Yes, except for some extra memory usage when remounting, >>>> this idea sounds great. Let me send a version of v3 to get rid >>>> of erofs_fs_context. >>> I'm not sure if Gao Xaing also prefers this. I think it would be better >>> to wait and listen for his thoughts before we sending v3. >> Okay, there's no rush on this. > I checked days ago, for example, XFS is also worked in this way. > And .reconfigure() needs to be carefully handled too. Ok, I'll implement it in the next iteration. >>>>>> +static void erofs_ctx_to_info(struct fs_context *fc) >>>>>> { >>>>>> struct erofs_fs_context *ctx = fc->fs_private; >>>>>> + struct erofs_sb_info *sbi = fc->s_fs_info; >>>>>> + >>>>>> + sbi->opt = ctx->opt; >>>>>> + sbi->devs = ctx->devs; >>>>>> + ctx->devs = NULL; >>>>>> + sbi->fsid = ctx->fsid; >>>>>> + ctx->fsid = NULL; >>>>>> + sbi->domain_id = ctx->domain_id; >>>>>> + ctx->domain_id = NULL; >>>>>> +} >>>>> I'm not sure if abstracting this logic into a seperate helper really >>>>> helps understanding the code as the logic itself is quite simple and >>>>> easy to be understood. Usually it's a hint of over-abstraction when a >>>>> simple helper has only one caller. >>>>> >>>> Static functions that have only one caller are compiled inline, so we >>>> don't have to worry about how that affects the code. >>>> >>>> The reason these codes are encapsulated in a separate function is so >>>> that the code reader understands that these codes are integrated >>>> as a whole, and that we shouldn't have to move one or two of these >>>> lines individually. >>>> >>>> But after we get rid of erofs_fs_context, those won't be needed >>>> anymore. >>> Yeah, I understand. It's only coding style concerns. >>> >>> >>> >> Okay, thanks! > I'm fine to get rid of those (erofs_fs_context) as long as the codebase > is more clearer and simple. BTW, for the current codebase, I also think > it's unneeded to have a separate helper called once without extra actual > meaning... > > Thanks, > Gao Xiang > Ok, this helper function will be gone in the next iteration. Thanks for the review! -- With Best Regards, Baokun Li
© 2016 - 2024 Red Hat, Inc.