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
[...]
============================================
To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
parsing the fsid, and then the superblock inherits this flag when it is
allocated, so that the sb_flags can be used to distinguish whether it is
in block dev based mode when calling erofs_kill_sb().
Signed-off-by: Baokun Li <[email protected]>
---
fs/erofs/super.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index b21bd8f78dc1..7539ce7d64bc 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
ctx->fsid = kstrdup(param->string, GFP_KERNEL);
if (!ctx->fsid)
return -ENOMEM;
+ fc->sb_flags |= SB_NODEV;
break;
case Opt_domain_id:
kfree(ctx->domain_id);
@@ -706,9 +707,7 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
static int erofs_fc_get_tree(struct fs_context *fc)
{
- struct erofs_fs_context *ctx = fc->fs_private;
-
- if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
+ if (fc->sb_flags & SB_NODEV)
return get_tree_nodev(fc, erofs_fc_fill_super);
return get_tree_bdev(fc, erofs_fc_fill_super);
@@ -801,7 +800,7 @@ static void erofs_kill_sb(struct super_block *sb)
{
struct erofs_sb_info *sbi;
- if (erofs_is_fscache_mode(sb))
+ if (sb->s_flags & SB_NODEV)
kill_anon_super(sb);
else
kill_block_super(sb);
--
2.31.1
On Mon, Apr 15, 2024 at 08:17:46PM +0800, 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
> [...]
> ============================================
>
> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
> parsing the fsid, and then the superblock inherits this flag when it is
> allocated, so that the sb_flags can be used to distinguish whether it is
> in block dev based mode when calling erofs_kill_sb().
>
> Signed-off-by: Baokun Li <[email protected]>
> ---
> fs/erofs/super.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index b21bd8f78dc1..7539ce7d64bc 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
> ctx->fsid = kstrdup(param->string, GFP_KERNEL);
> if (!ctx->fsid)
> return -ENOMEM;
> + fc->sb_flags |= SB_NODEV;
Hm, I wouldn't do it this way. That's an abuse of that flag imho.
Record the information in the erofs_fs_context if you need to.
On 2024/4/15 21:38, Christian Brauner wrote:
> On Mon, Apr 15, 2024 at 08:17:46PM +0800, 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
>> [...]
>> ============================================
>>
>> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
>> parsing the fsid, and then the superblock inherits this flag when it is
>> allocated, so that the sb_flags can be used to distinguish whether it is
>> in block dev based mode when calling erofs_kill_sb().
>>
>> Signed-off-by: Baokun Li <[email protected]>
>> ---
>> fs/erofs/super.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index b21bd8f78dc1..7539ce7d64bc 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>> ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>> if (!ctx->fsid)
>> return -ENOMEM;
>> + fc->sb_flags |= SB_NODEV;
> Hm, I wouldn't do it this way. That's an abuse of that flag imho.
> Record the information in the erofs_fs_context if you need to.
Hi Christian!
The problem here is that when mounting erofs, if we have an fsid
then it is not block device based, if we don't have an fsid it is block
device based. So only after we confirmed whether we have an fsid
or not, we can confirm whether we need SB_NODEV or not.
--
With Best Regards,
Baokun Li
.
On 2024/4/15 21:38, Christian Brauner wrote:
> On Mon, Apr 15, 2024 at 08:17:46PM +0800, 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
>> [...]
>> ============================================
>>
>> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
>> parsing the fsid, and then the superblock inherits this flag when it is
>> allocated, so that the sb_flags can be used to distinguish whether it is
>> in block dev based mode when calling erofs_kill_sb().
>>
>> Signed-off-by: Baokun Li <[email protected]>
>> ---
>> fs/erofs/super.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index b21bd8f78dc1..7539ce7d64bc 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>> ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>> if (!ctx->fsid)
>> return -ENOMEM;
>> + fc->sb_flags |= SB_NODEV;
> Hm, I wouldn't do it this way. That's an abuse of that flag imho.
> Record the information in the erofs_fs_context if you need to.
The stack diagram that triggers the problem is as follows, the call to
erofs_kill_sb() fails before fill_super() has been executed, and we can
only use super_block to determine whether it is currently in nodev
fscahe mode or block device based mode. So if it is recorded in
erofs_fs_context (aka fc->fs_private), we can't access the recorded data
unless we pass fc into erofs_kill_sb() as well.
do_new_mount
fs_context_for_mount
alloc_fs_context
erofs_init_fs_context
// Mount options have not been parsed yet,
// it is not clear if there is an fsid
parse_monolithic_mount_data
generic_parse_monolithic
erofs_fc_parse_param
case Opt_fsid:
// fc->sb_flags |= SB_NODEV;
vfs_get_tree
erofs_fc_get_tree
get_tree_bdev
s = sget_dev(fc, dev)
s = alloc_super()
s->s_flags = fc->sb_flags;
setup_bdev_super
// return error
sb->s_bdev = bdev // s_bdev is NULL
deactivate_locked_super
erofs_kill_sb
// Block device based mode is considered fscahe mode for nodev
erofs_is_fscache_mode(sb)
IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && !sb->s_bdev
kill_anon_super(sb)
free_anon_bdev(dev)
ida_free(&unnamed_dev_ida, MINOR(dev))
((int)id < 0)
WARN(1, "ida_free called for id=%d which is not
allocated.\n", id);
Thanks!
--
With Best Regards,
Baokun Li
Hi Christian, Baokun,
On Mon, Apr 15, 2024 at 11:23:58PM +0800, Baokun Li wrote:
> On 2024/4/15 21:38, Christian Brauner wrote:
> > On Mon, Apr 15, 2024 at 08:17:46PM +0800, 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
> > > [...]
> > > ============================================
> > >
> > > To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
> > > parsing the fsid, and then the superblock inherits this flag when it is
> > > allocated, so that the sb_flags can be used to distinguish whether it is
> > > in block dev based mode when calling erofs_kill_sb().
> > >
> > > Signed-off-by: Baokun Li <[email protected]>
> > > ---
> > > fs/erofs/super.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > > index b21bd8f78dc1..7539ce7d64bc 100644
> > > --- a/fs/erofs/super.c
> > > +++ b/fs/erofs/super.c
> > > @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
> > > ctx->fsid = kstrdup(param->string, GFP_KERNEL);
> > > if (!ctx->fsid)
> > > return -ENOMEM;
> > > + fc->sb_flags |= SB_NODEV;
> > Hm, I wouldn't do it this way. That's an abuse of that flag imho.
> > Record the information in the erofs_fs_context if you need to.
> The stack diagram that triggers the problem is as follows, the call to
> erofs_kill_sb() fails before fill_super() has been executed, and we can
> only use super_block to determine whether it is currently in nodev
> fscahe mode or block device based mode. So if it is recorded in
> erofs_fs_context (aka fc->fs_private), we can't access the recorded data
> unless we pass fc into erofs_kill_sb() as well.
>
If I understand correctly, from the discussion above, I think
there exists a gap between alloc_super() and sb->s_bdev is set.
But .kill_sb() can be called between them and fc is not passed
into .kill_sb().
I'm not sure how to resolve it in EROFS itself, anyway...
Thanks,
Gao Xiang
On 2024/4/16 8:57, Gao Xiang wrote:
> Hi Christian, Baokun,
>
> On Mon, Apr 15, 2024 at 11:23:58PM +0800, Baokun Li wrote:
>> On 2024/4/15 21:38, Christian Brauner wrote:
>>> On Mon, Apr 15, 2024 at 08:17:46PM +0800, 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
>>>> [...]
>>>> ============================================
>>>>
>>>> To avoid this problem, add SB_NODEV to fc->sb_flags after successfully
>>>> parsing the fsid, and then the superblock inherits this flag when it is
>>>> allocated, so that the sb_flags can be used to distinguish whether it is
>>>> in block dev based mode when calling erofs_kill_sb().
>>>>
>>>> Signed-off-by: Baokun Li <[email protected]>
>>>> ---
>>>> fs/erofs/super.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>> index b21bd8f78dc1..7539ce7d64bc 100644
>>>> --- a/fs/erofs/super.c
>>>> +++ b/fs/erofs/super.c
>>>> @@ -520,6 +520,7 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>>>> ctx->fsid = kstrdup(param->string, GFP_KERNEL);
>>>> if (!ctx->fsid)
>>>> return -ENOMEM;
>>>> + fc->sb_flags |= SB_NODEV;
>>> Hm, I wouldn't do it this way. That's an abuse of that flag imho.
>>> Record the information in the erofs_fs_context if you need to.
>> The stack diagram that triggers the problem is as follows, the call to
>> erofs_kill_sb() fails before fill_super() has been executed, and we can
>> only use super_block to determine whether it is currently in nodev
>> fscahe mode or block device based mode. So if it is recorded in
>> erofs_fs_context (aka fc->fs_private), we can't access the recorded data
>> unless we pass fc into erofs_kill_sb() as well.
>>
> If I understand correctly, from the discussion above, I think
> there exists a gap between alloc_super() and sb->s_bdev is set.
> But .kill_sb() can be called between them and fc is not passed
> into .kill_sb().
>
> I'm not sure how to resolve it in EROFS itself, anyway...
>
> Thanks,
> Gao Xiang
Yes, exactly!
There is no fill_super between alloc_super() and kill_sb(), so erofs has
no way to set a flag for the superblock directly. The only way I can
think of now is to modify fc->sb_flags in erofs_fc_parse_param()
to indirectly set a flag for the superblock.
Cheers,
Baokun
> > I'm not sure how to resolve it in EROFS itself, anyway...
Instead of allocating the erofs_sb_info in fill_super() allocate it
during erofs_get_tree() and then you can ensure that you always have the
info you need available during erofs_kill_sb(). See the appended
(untested) patch.
On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
> > > I'm not sure how to resolve it in EROFS itself, anyway...
>
> Instead of allocating the erofs_sb_info in fill_super() allocate it
> during erofs_get_tree() and then you can ensure that you always have the
> info you need available during erofs_kill_sb(). See the appended
> (untested) patch.
Hi Christian,
Yeah, that is a good way I think. Although sbi will be allocated
unconditionally instead but that is minor.
I'm on OSSNA this week, will test this patch more when returning.
Hi Baokun,
Could you also check this on your side?
Thanks,
Gao Xiang
> From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <[email protected]>
> Date: Mon, 15 Apr 2024 20:17:46 +0800
> Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
>
> 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: Baokun Li <[email protected]>
> Signed-off-by: Christian Brauner <[email protected]>
> ---
> fs/erofs/super.c | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index c0eb139adb07..4ed80154edf8 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -581,7 +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_sb_info *sbi = EROFS_SB(sb);
> struct erofs_fs_context *ctx = fc->fs_private;
> int err;
>
> @@ -590,15 +590,10 @@ 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;
> @@ -707,8 +702,15 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
> static int erofs_fc_get_tree(struct fs_context *fc)
> {
> struct erofs_fs_context *ctx = fc->fs_private;
> + struct erofs_sb_info *sbi;
> +
> + sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> + if (!sbi)
> + return -ENOMEM;
>
> - if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
> + fc->s_fs_info = sbi;
> + sbi->fsid = ctx->fsid;
> + 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);
> @@ -762,11 +764,15 @@ static void erofs_free_dev_context(struct erofs_dev_context *devs)
> static void erofs_fc_free(struct fs_context *fc)
> {
> struct erofs_fs_context *ctx = fc->fs_private;
> + struct erofs_sb_info *sbi = fc->s_fs_info;
>
> erofs_free_dev_context(ctx->devs);
> kfree(ctx->fsid);
> kfree(ctx->domain_id);
> kfree(ctx);
> +
> + if (sbi)
> + kfree(sbi);
> }
>
> static const struct fs_context_operations erofs_context_ops = {
> @@ -783,6 +789,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 +806,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.43.0
>
On 2024/4/16 22:49, Gao Xiang wrote:
> On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
>>>> I'm not sure how to resolve it in EROFS itself, anyway...
>> Instead of allocating the erofs_sb_info in fill_super() allocate it
>> during erofs_get_tree() and then you can ensure that you always have the
>> info you need available during erofs_kill_sb(). See the appended
>> (untested) patch.
> Hi Christian,
>
> Yeah, that is a good way I think. Although sbi will be allocated
> unconditionally instead but that is minor.
>
> I'm on OSSNA this week, will test this patch more when returning.
>
> Hi Baokun,
>
> Could you also check this on your side?
>
> Thanks,
> Gao Xiang
Hi Xiang,
This patch does fix the initial problem.
Hi Christian,
Thanks for the patch, this is a good idea. Just with nits below.
Otherwise feel free to add.
Reviewed-and-tested-by: Baokun Li <[email protected]>
>
>> From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
>> From: Christian Brauner <[email protected]>
>> Date: Mon, 15 Apr 2024 20:17:46 +0800
>> Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
>>
SNIP
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index c0eb139adb07..4ed80154edf8 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -581,7 +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_sb_info *sbi = EROFS_SB(sb);
>> struct erofs_fs_context *ctx = fc->fs_private;
>> int err;
>>
>> @@ -590,15 +590,10 @@ 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;
This line is no longer needed.
>> 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;
Since erofs_sb_info is now allocated in erofs_fc_get_tree(), why not
encapsulate the above lines as erofs_ctx_to_info() helper function
to be called in erofs_fc_get_tree()？Then erofs_fc_fill_super() wouldn't
have to use erofs_fs_context and would prevent the fsid from being
freed twice.
>> @@ -707,8 +702,15 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>> static int erofs_fc_get_tree(struct fs_context *fc)
>> {
>> struct erofs_fs_context *ctx = fc->fs_private;
>> + struct erofs_sb_info *sbi;
>> +
>> + sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>> + if (!sbi)
>> + return -ENOMEM;
>>
>> - if (IS_ENABLED(CONFIG_EROFS_FS_ONDEMAND) && ctx->fsid)
>> + fc->s_fs_info = sbi;
>> + sbi->fsid = ctx->fsid;
Here ctx->fsid is not set to null, so fsid may be freed twice.
>> + 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);
>> @@ -762,11 +764,15 @@ static void erofs_free_dev_context(struct erofs_dev_context *devs)
>> static void erofs_fc_free(struct fs_context *fc)
>> {
>> struct erofs_fs_context *ctx = fc->fs_private;
>> + struct erofs_sb_info *sbi = fc->s_fs_info;
>>
>> erofs_free_dev_context(ctx->devs);
>> kfree(ctx->fsid);
>> kfree(ctx->domain_id);
>> kfree(ctx);
>> +
>> + if (sbi)
>> + kfree(sbi);
There's no need to check sbi, kfree does.
>> }
>>
>> static const struct fs_context_operations erofs_context_ops = {
>> @@ -783,6 +789,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 +806,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.43.0
>>
On Wed, Apr 17, 2024 at 10:59:53AM +0800, Baokun Li wrote:
> On 2024/4/16 22:49, Gao Xiang wrote:
> > On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
> > > > > I'm not sure how to resolve it in EROFS itself, anyway...
> > > Instead of allocating the erofs_sb_info in fill_super() allocate it
> > > during erofs_get_tree() and then you can ensure that you always have the
> > > info you need available during erofs_kill_sb(). See the appended
> > > (untested) patch.
> > Hi Christian,
> >
> > Yeah, that is a good way I think. Although sbi will be allocated
> > unconditionally instead but that is minor.
> >
> > I'm on OSSNA this week, will test this patch more when returning.
> >
> > Hi Baokun,
> >
> > Could you also check this on your side?
> >
> > Thanks,
> > Gao Xiang
> Hi Xiang,
>
> This patch does fix the initial problem.
>
>
> Hi Christian,
>
> Thanks for the patch, this is a good idea. Just with nits below.
> Otherwise feel free to add.
>
> Reviewed-and-tested-by: Baokun Li <[email protected]>
> >
> > > From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
> > > From: Christian Brauner <[email protected]>
> > > Date: Mon, 15 Apr 2024 20:17:46 +0800
> > > Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
> > >
> SNIP
>
> > >
> > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > > index c0eb139adb07..4ed80154edf8 100644
> > > --- a/fs/erofs/super.c
> > > +++ b/fs/erofs/super.c
> > > @@ -581,7 +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_sb_info *sbi = EROFS_SB(sb);
> > > struct erofs_fs_context *ctx = fc->fs_private;
> > > int err;
> > > @@ -590,15 +590,10 @@ 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;
> This line is no longer needed.
> > > 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;
> Since erofs_sb_info is now allocated in erofs_fc_get_tree(), why not
> encapsulate the above lines as erofs_ctx_to_info() helper function
> to be called in erofs_fc_get_tree()？Then erofs_fc_fill_super() wouldn't
> have to use erofs_fs_context and would prevent the fsid from being
> freed twice.
Hi Baokun,
I'm not sure if Christian has enough time to polish the whole
codebase (I'm happy if do so). Basically, that is just a hint
to the issue, if you have more time, I guess you could also help
revive this patch together (also because you also have a real
EROFS test environment).
Let me also check this next week after OSSNA travelling.
Thanks,
Gao Xiang
On 2024/4/17 11:11, Gao Xiang wrote:
> On Wed, Apr 17, 2024 at 10:59:53AM +0800, Baokun Li wrote:
>> On 2024/4/16 22:49, Gao Xiang wrote:
>>> On Tue, Apr 16, 2024 at 02:35:08PM +0200, Christian Brauner wrote:
>>>>>> I'm not sure how to resolve it in EROFS itself, anyway...
>>>> Instead of allocating the erofs_sb_info in fill_super() allocate it
>>>> during erofs_get_tree() and then you can ensure that you always have the
>>>> info you need available during erofs_kill_sb(). See the appended
>>>> (untested) patch.
>>> Hi Christian,
>>>
>>> Yeah, that is a good way I think. Although sbi will be allocated
>>> unconditionally instead but that is minor.
>>>
>>> I'm on OSSNA this week, will test this patch more when returning.
>>>
>>> Hi Baokun,
>>>
>>> Could you also check this on your side?
>>>
>>> Thanks,
>>> Gao Xiang
>> Hi Xiang,
>>
>> This patch does fix the initial problem.
>>
>>
>> Hi Christian,
>>
>> Thanks for the patch, this is a good idea. Just with nits below.
>> Otherwise feel free to add.
>>
>> Reviewed-and-tested-by: Baokun Li <[email protected]>
>>>> From e4f586a41748b6edc05aca36d49b7b39e55def81 Mon Sep 17 00:00:00 2001
>>>> From: Christian Brauner <[email protected]>
>>>> Date: Mon, 15 Apr 2024 20:17:46 +0800
>>>> Subject: [PATCH] erofs: reliably distinguish block based and fscache mode
>>>>
>> SNIP
>>
>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>> index c0eb139adb07..4ed80154edf8 100644
>>>> --- a/fs/erofs/super.c
>>>> +++ b/fs/erofs/super.c
>>>> @@ -581,7 +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_sb_info *sbi = EROFS_SB(sb);
>>>> struct erofs_fs_context *ctx = fc->fs_private;
>>>> int err;
>>>> @@ -590,15 +590,10 @@ 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;
>> This line is no longer needed.
>>>> 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;
>> Since erofs_sb_info is now allocated in erofs_fc_get_tree(), why not
>> encapsulate the above lines as erofs_ctx_to_info() helper function
>> to be called in erofs_fc_get_tree()？Then erofs_fc_fill_super() wouldn't
>> have to use erofs_fs_context and would prevent the fsid from being
>> freed twice.
> Hi Baokun,
>
> I'm not sure if Christian has enough time to polish the whole
> codebase (I'm happy if do so). Basically, that is just a hint
> to the issue, if you have more time, I guess you could also help
> revive this patch together (also because you also have a real
> EROFS test environment).
>
> Let me also check this next week after OSSNA travelling.
>
> Thanks,
> Gao Xiang
Hi Xiang,
Ok, then I will polish the patch and send it out as a v2.
Thanks,
Baokun