Re: [PATCH] ext4: Do not destroy ext4_groupinfo_caches if ext4_mb_init() fails

[Lukáš Czerner]

On Tue, 13 May 2014, Andrey Tsyvarev wrote:

> Date: Tue, 13 May 2014 14:17:25 +0400
> From: Andrey Tsyvarev <tsyvarev@xxxxxxxxx>
> To: LukÃÅ Czerner <lczerner@xxxxxxxxxx>
> Cc: Theodore Ts'o <tytso@xxxxxxx>, Andreas Dilger <adilger.kernel@xxxxxxxxx>,
>     linux-ext4@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx,
>     Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> 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<tsyvarev@xxxxxxxxx>
> > > To: Theodore Ts'o<tytso@xxxxxxx>
> > > Cc: Andrey Tsyvarev<tsyvarev@xxxxxxxxx>,
> > >      Andreas Dilger<adilger.kernel@xxxxxxxxx>,linux-ext4@xxxxxxxxxxxxxxx,
> > >      linux-kernel@xxxxxxxxxxxxxxx, Alexey
> > > Khoroshilov<khoroshilov@xxxxxxxxx>
> > > 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<lczerner@xxxxxxxxxx>
> > 
> > > Signed-off-by: Andrey Tsyvarev<tsyvarev@xxxxxxxxx>
> > > ---
> > >   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
> 
>