Caches from 'ext4_groupinfo_caches' may be in use by other mounts, which have already existed.
So, it is incorrect to destroy them when newly requested mount fails.
Found by Linux File System Verification project (linuxtesting.org).
Signed-off-by: Andrey Tsyvarev <[email protected]>
---
fs/ext4/mballoc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 04a5c75..becea1d 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2607,7 +2607,7 @@ int ext4_mb_init(struct super_block *sb)
sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
if (sbi->s_locality_groups == NULL) {
ret = -ENOMEM;
- goto out_free_groupinfo_slab;
+ goto out;
}
for_each_possible_cpu(i) {
struct ext4_locality_group *lg;
@@ -2632,8 +2632,6 @@ int ext4_mb_init(struct super_block *sb)
out_free_locality_groups:
free_percpu(sbi->s_locality_groups);
sbi->s_locality_groups = NULL;
-out_free_groupinfo_slab:
- ext4_groupinfo_destroy_slabs();
out:
kfree(sbi->s_mb_offsets);
sbi->s_mb_offsets = NULL;
--
1.8.3.1
On Mon, 12 May 2014, Andrey Tsyvarev wrote:
> Date: Mon, 12 May 2014 12:23:59 +0400
> From: Andrey Tsyvarev <[email protected]>
> To: Theodore Ts'o <[email protected]>
> Cc: Andrey Tsyvarev <[email protected]>,
> Andreas Dilger <[email protected]>, [email protected],
> [email protected], Alexey Khoroshilov <[email protected]>
> Subject: [PATCH] ext4: Do not destroy ext4_groupinfo_caches if ext4_mb_init()
> fails
>
> Caches from 'ext4_groupinfo_caches' may be in use by other mounts, which have already existed.
> So, it is incorrect to destroy them when newly requested mount fails.
>
> Found by Linux File System Verification project (linuxtesting.org).
Makes sense, thanks! Can you please share the test case which
triggered this ? It might be worth including in xfstests.
Reviewed-by: Lukas Czerner <[email protected]>
>
> Signed-off-by: Andrey Tsyvarev <[email protected]>
> ---
> fs/ext4/mballoc.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 04a5c75..becea1d 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2607,7 +2607,7 @@ int ext4_mb_init(struct super_block *sb)
> sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> if (sbi->s_locality_groups == NULL) {
> ret = -ENOMEM;
> - goto out_free_groupinfo_slab;
> + goto out;
> }
> for_each_possible_cpu(i) {
> struct ext4_locality_group *lg;
> @@ -2632,8 +2632,6 @@ int ext4_mb_init(struct super_block *sb)
> out_free_locality_groups:
> free_percpu(sbi->s_locality_groups);
> sbi->s_locality_groups = NULL;
> -out_free_groupinfo_slab:
> - ext4_groupinfo_destroy_slabs();
> out:
> kfree(sbi->s_mb_offsets);
> sbi->s_mb_offsets = NULL;
>
On Mon, May 12, 2014 at 05:08:41PM +0200, Lukáš Czerner wrote:
> >
> > Caches from 'ext4_groupinfo_caches' may be in use by other mounts, which have already existed.
> > So, it is incorrect to destroy them when newly requested mount fails.
> >
> > Found by Linux File System Verification project (linuxtesting.org).
>
> Makes sense, thanks! Can you please share the test case which
> triggered this ? It might be worth including in xfstests.
>
> Reviewed-by: Lukas Czerner <[email protected]>
Thanks, applied.
- Ted
12.05.2014 19:08, Lukáš Czerner пишет:
> On Mon, 12 May 2014, Andrey Tsyvarev wrote:
>
>> Date: Mon, 12 May 2014 12:23:59 +0400
>> From: Andrey Tsyvarev<[email protected]>
>> To: Theodore Ts'o<[email protected]>
>> Cc: Andrey Tsyvarev<[email protected]>,
>> Andreas Dilger<[email protected]>,[email protected],
>> [email protected], Alexey Khoroshilov<[email protected]>
>> Subject: [PATCH] ext4: Do not destroy ext4_groupinfo_caches if ext4_mb_init()
>> fails
>>
>> Caches from 'ext4_groupinfo_caches' may be in use by other mounts, which have already existed.
>> So, it is incorrect to destroy them when newly requested mount fails.
>>
>> Found by Linux File System Verification project (linuxtesting.org).
> Makes sense, thanks! Can you please share the test case which
> triggered this ? It might be worth including in xfstests.
Actually it was triggered by xfstests themselves but run with fault simulation.
The method of fault simulation is under development/evaluation now, we expect to publish a paper describing it in the near future.
BUG_ON() in get_groupinfo_cache() was firstly triggered by test generic/003, but actually it could be any other test, which uses a scratch device: xftests itself requires test device(TEST_DEV) mounted, so a fault simulated while mount scratch device causes the problem described.
> Reviewed-by: Lukas Czerner<[email protected]>
>
>> Signed-off-by: Andrey Tsyvarev<[email protected]>
>> ---
>> fs/ext4/mballoc.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 04a5c75..becea1d 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -2607,7 +2607,7 @@ int ext4_mb_init(struct super_block *sb)
>> sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
>> if (sbi->s_locality_groups == NULL) {
>> ret = -ENOMEM;
>> - goto out_free_groupinfo_slab;
>> + goto out;
>> }
>> for_each_possible_cpu(i) {
>> struct ext4_locality_group *lg;
>> @@ -2632,8 +2632,6 @@ int ext4_mb_init(struct super_block *sb)
>> out_free_locality_groups:
>> free_percpu(sbi->s_locality_groups);
>> sbi->s_locality_groups = NULL;
>> -out_free_groupinfo_slab:
>> - ext4_groupinfo_destroy_slabs();
>> out:
>> kfree(sbi->s_mb_offsets);
>> sbi->s_mb_offsets = NULL;
>>
--
Andrey Tsyvarev
Linux Verification Center, ISPRAS
On Tue, 13 May 2014, Andrey Tsyvarev wrote:
> Date: Tue, 13 May 2014 14:17:25 +0400
> From: Andrey Tsyvarev <[email protected]>
> To: Lukáš Czerner <[email protected]>
> Cc: Theodore Ts'o <[email protected]>, Andreas Dilger <[email protected]>,
> [email protected], [email protected],
> Alexey Khoroshilov <[email protected]>
> Subject: Re: [PATCH] ext4: Do not destroy ext4_groupinfo_caches if
> ext4_mb_init() fails
>
>
> 12.05.2014 19:08, Lukáš Czerner пишет:
> > On Mon, 12 May 2014, Andrey Tsyvarev wrote:
> >
> > > Date: Mon, 12 May 2014 12:23:59 +0400
> > > From: Andrey Tsyvarev<[email protected]>
> > > To: Theodore Ts'o<[email protected]>
> > > Cc: Andrey Tsyvarev<[email protected]>,
> > > Andreas Dilger<[email protected]>,[email protected],
> > > [email protected], Alexey
> > > Khoroshilov<[email protected]>
> > > Subject: [PATCH] ext4: Do not destroy ext4_groupinfo_caches if
> > > ext4_mb_init()
> > > fails
> > >
> > > Caches from 'ext4_groupinfo_caches' may be in use by other mounts, which
> > > have already existed.
> > > So, it is incorrect to destroy them when newly requested mount fails.
> > >
> > > Found by Linux File System Verification project (linuxtesting.org).
> > Makes sense, thanks! Can you please share the test case which
> > triggered this ? It might be worth including in xfstests.
>
> Actually it was triggered by xfstests themselves but run with fault
> simulation.
> The method of fault simulation is under development/evaluation now, we expect
> to publish a paper describing it in the near future.
>
> BUG_ON() in get_groupinfo_cache() was firstly triggered by test generic/003,
> but actually it could be any other test, which uses a scratch device: xftests
> itself requires test device(TEST_DEV) mounted, so a fault simulated while
> mount scratch device causes the problem described.
It sounds interesting. I hope that you'll send the information out
to the fsdevel list when your paper is finished, It looks like it
might be quite useful.
Thanks!
-Lukas
>
>
> > Reviewed-by: Lukas Czerner<[email protected]>
> >
> > > Signed-off-by: Andrey Tsyvarev<[email protected]>
> > > ---
> > > fs/ext4/mballoc.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > > index 04a5c75..becea1d 100644
> > > --- a/fs/ext4/mballoc.c
> > > +++ b/fs/ext4/mballoc.c
> > > @@ -2607,7 +2607,7 @@ int ext4_mb_init(struct super_block *sb)
> > > sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> > > if (sbi->s_locality_groups == NULL) {
> > > ret = -ENOMEM;
> > > - goto out_free_groupinfo_slab;
> > > + goto out;
> > > }
> > > for_each_possible_cpu(i) {
> > > struct ext4_locality_group *lg;
> > > @@ -2632,8 +2632,6 @@ int ext4_mb_init(struct super_block *sb)
> > > out_free_locality_groups:
> > > free_percpu(sbi->s_locality_groups);
> > > sbi->s_locality_groups = NULL;
> > > -out_free_groupinfo_slab:
> > > - ext4_groupinfo_destroy_slabs();
> > > out:
> > > kfree(sbi->s_mb_offsets);
> > > sbi->s_mb_offsets = NULL;
> > >
>
> --
> Andrey Tsyvarev
> Linux Verification Center, ISPRAS
>
>