On Tue, 13 May 2014, Andrey Tsyvarev wrote:
> Date: Tue, 13 May 2014 14:17:25 +0400
> From: Andrey Tsyvarev <tsyva...@ispras.ru>
> To: Lukáš Czerner <lczer...@redhat.com>
> Cc: Theodore Ts'o <ty...@mit.edu>, Andreas Dilger <adilger.ker...@dilger.ca>,
> linux-e...@vger.kernel.org, linux-kernel@vger.kernel.org,
> Alexey Khoroshilov <khoroshi...@ispras.ru>
> 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<tsyva...@ispras.ru>
> > > To: Theodore Ts'o<ty...@mit.edu>
> > > Cc: Andrey Tsyvarev<tsyva...@ispras.ru>,
> > > Andreas Dilger<adilger.ker...@dilger.ca>,linux-e...@vger.kernel.org,
> > > linux-kernel@vger.kernel.org, Alexey
> > > Khoroshilov<khoroshi...@ispras.ru>
> > > 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<lczer...@redhat.com>
> >
> > > Signed-off-by: Andrey Tsyvarev<tsyva...@ispras.ru>
> > > ---
> > > 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
>
>
