new block group bitmaps not being initialized after resize!?

new block group bitmaps not being initialized after resize!?

[Carlos Maiolino]

Hi guys,

I'm working on a Fedora bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=852833)
together with Eric and we found that the problem described on the bugzilla
happens when the commit 93f9052643 is not applied, which is the case of the
Fedora 16 kernel being discussed there.

Although we already found the solution to the problem in the commit above,
looking through the commit have raised some questions regarding to the bitmap of
the newer block groups added to the FS after it is extended.

The newer block groups do not have flags ITABLE_ZEROED and INODE_UNINIT set,
even when 'lazy_itable_init' is enabled.

Without this commit, inode stale data might be exposed and also makes fsck
complain about all inodes of the newer block groups.

The question is, are these flags expected to not be found on these newer block
groups or they should be set?

The lack of these flags on newer block groups is an expected behaviour or is
something that should be fixed?

FWIW, in the old ext4_group_add(), we added EXT4_BG_INODE_ZEROED flag to the
bg_flags, also, I did some tests here and the lack of these flags look to not be
affecting filesystem integrity, i.e. new inodes can be properly allocated, which
sounds that these uninitialized inodes/bitmaps are set to be initialized as soon
as a first inode is allocated on these new BGs.


Anything else I'm missing Eric?

Cheers,
-- 
Carlos

Re: new block group bitmaps not being initialized after resize!?

[Eric Sandeen]

On 1/10/13 6:07 PM, Carlos Maiolino wrote:
> Hi guys,
> 
> I'm working on a Fedora bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=852833)
> together with Eric and we found that the problem described on the bugzilla
> happens when the commit 93f9052643 is not applied, which is the case of the
> Fedora 16 kernel being discussed there.

Also, to be clear, this is with older e2fsprogs which was using the old resize
interface.  Not sure what the behavior is w/ newer e2fsprogs, but we don't
see the corruption.

Note, we see the corruption on these older kernels even when resizing from
say 100G to 120G.  It appears fixed upstream, but so much has changed,
we need to be sure the older interface doesn't have bugs lurking, I think.

> Although we already found the solution to the problem in the commit above,
> looking through the commit have raised some questions regarding to the bitmap of
> the newer block groups added to the FS after it is extended.
> 
> The newer block groups do not have flags ITABLE_ZEROED and INODE_UNINIT set,
> even when 'lazy_itable_init' is enabled.

In particular, we see things like this in the last pre-resize group:

Group 799: (Blocks 26181632-26214399) [INODE_UNINIT, ITABLE_ZEROED]
  Checksum 0xafe7, unused inodes 1936
  Block bitmap at 25165855 (bg #768 + 31), Inode bitmap at 25166111 (bg #768 + 287)
  Inode table at 25170087-25170207 (bg #768 + 4263)
  32768 free blocks, 1936 free inodes, 0 directories, 1936 unused inodes
  Free blocks: 26181632-26214399
  Free inodes: 1546865-1548800

and this in the newly-added groups:

Group 800: (Blocks 26214400-26247167)
  Checksum 0xddc4, unused inodes 0
  Block bitmap at 26236224 (+21824), Inode bitmap at 26236225 (+21825)
  Inode table at 26214400-26214520
  32645 free blocks, 1936 free inodes, 0 directories
  Free blocks: 26214521-26236223, 26236226-26247167
  Free inodes: 1548801-1550736

so it says 0 unused inodes, but also 1936 free inodes (?), and
no UNINIT or ZEROED flags set.  e2fsck finds stale data in the inode table,
and goes nuts.

> Without this commit, inode stale data might be exposed and also makes fsck
> complain about all inodes of the newer block groups.

*nod* :)
 
so 93f9052643 seems to have accidentally fixed this, by setting
the unused counter to EXT4_INODES_PER_GROUP(), but it feels like
we've missed properly setting up this block group.

To be honest, though, sometimes I get lost in the sea of flags.

> The question is, are these flags expected to not be found on these newer block
> groups or they should be set?

*nod* :)

-Eric

> The lack of these flags on newer block groups is an expected behaviour or is
> something that should be fixed?
> 
> FWIW, in the old ext4_group_add(), we added EXT4_BG_INODE_ZEROED flag to the
> bg_flags, also, I did some tests here and the lack of these flags look to not be
> affecting filesystem integrity, i.e. new inodes can be properly allocated, which
> sounds that these uninitialized inodes/bitmaps are set to be initialized as soon
> as a first inode is allocated on these new BGs.
> 
> 
> Anything else I'm missing Eric?
> 
> Cheers,
> 

Re: new block group bitmaps not being initialized after resize!?

[Andreas Dilger]

On 2013-01-10, at 6:07 PM, Eric Sandeen wrote:
> On 1/10/13 6:07 PM, Carlos Maiolino wrote:
>> I'm working on a Fedora bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=852833)
>> together with Eric and we found that the problem described on the
>> bugzilla happens when the commit 93f9052643 is not applied, which
>> is the case of the Fedora 16 kernel being discussed there.
> 
> Also, to be clear, this is with older e2fsprogs which was using the
> old resize interface.  Not sure what the behavior is w/ newer
> e2fsprogs, but we don't see the corruption.
> 
> Note, we see the corruption on these older kernels even when resizing from say 100G to 120G.  It appears fixed upstream, but so much has
> changed, we need to be sure the older interface doesn't have bugs
> lurking, I think.

The original resize code didn't ever know about uninit_bg, so it
would always zero out the inode table, so I suspect that this was
added at some later point. 

>> Although we already found the solution to the problem in the commit
>> above, looking through the commit have raised some questions
>> regarding to the bitmap of the newer block groups added to the FS
>> after it is extended.
>> 
>> The newer block groups do not have flags ITABLE_ZEROED and
>> INODE_UNINIT set, even when 'lazy_itable_init' is enabled.
> 
> In particular, we see things like this in the last pre-resize group:
> 
> Group 799: (Blocks 26181632-26214399) [INODE_UNINIT, ITABLE_ZEROED]
>  Checksum 0xafe7, unused inodes 1936
>  Block bitmap at 25165855 (bg #768 + 31), Inode bitmap at 25166111 (bg #768 + 287)
>  Inode table at 25170087-25170207 (bg #768 + 4263)
>  32768 free blocks, 1936 free inodes, 0 directories, 1936 unused inodes
>  Free blocks: 26181632-26214399
>  Free inodes: 1546865-1548800
> 
> and this in the newly-added groups:
> 
> Group 800: (Blocks 26214400-26247167)
>  Checksum 0xddc4, unused inodes 0
>  Block bitmap at 26236224 (+21824), Inode bitmap at 26236225 (+21825)
>  Inode table at 26214400-26214520
>  32645 free blocks, 1936 free inodes, 0 directories
>  Free blocks: 26214521-26236223, 26236226-26247167
>  Free inodes: 1548801-1550736
> 
> so it says 0 unused inodes, but also 1936 free inodes (?), and
> no UNINIT or ZEROED flags set.  e2fsck finds stale data in the inode table, and goes nuts.

Zeroing the inode table but not setting the INODE_ZEROED flag
would not be harmful, but this seems to not be the case.

When the filesystem is remounted, does the kernel lazyinit
thread zero out the new groups in the inode table?


>> Without this commit, inode stale data might be exposed and also makes fsck complain about all inodes of the newer block groups.
> 
> *nod* :)

That's why 

> so 93f9052643 seems to have accidentally fixed this, by setting
> the unused counter to EXT4_INODES_PER_GROUP(), but it feels like
> we've missed properly setting up this block group.

Actually, just setting the unused counter is not enough to properly
fix this problem.  The lazyinit thread should be started to do
background zeroing of the inode table, otherwise if the group
descriptor is corrupted and the bg_itable_unused value is wrong,
then the uninitialized inodes would be accessed.

> To be honest, though, sometimes I get lost in the sea of flags.
> 
>> The question is, are these flags expected to not be found on
>> these newer block groups or they should be set?
> 
> *nod* :)

Depends on how it is implemented. :-/  The flag should definitely
not be set unless the itable is actually overwritten with zeroes.
It makes sense that the lazyinit thread would do this in the
background while the filesystem is mounted instead of waiting for
the next time that the filesystem is mounted.

Looking at the code, it appears that this is not happening at the
end of the resize, since ext4_register_li_request() is marked
static for the superblock.  It looks like it would be relatively
straight forward to add a call to ext4_register_li_request() to
ext4_resize_end() with the first group added to the resize.

It looks like ext4_run_li_request() will skip groups that are
already marked as INODE_ZERO, so it is fine to always call it even
if the kernel is already setting this itself in some cases (not
that I see this happening).

>> The lack of these flags on newer block groups is an expected
>> behaviour or is something that should be fixed?
>> 
>> FWIW, in the old ext4_group_add(), we added EXT4_BG_INODE_ZEROED
>> flag to the bg_flags, also, I did some tests here and the lack
>> of these flags look to not be affecting filesystem integrity,
>> i.e. new inodes can be properly allocated, which sounds that
>> these uninitialized inodes/bitmaps are set to be initialized
>> as soon as a first inode is allocated on these new BGs.

Well, the old code always zeroed the inode table, and it made sense
to mark it as such.

Cheers, Andreas

Re: new block group bitmaps not being initialized after resize!?

[Eric Sandeen]

On 1/11/13 12:44 AM, Andreas Dilger wrote:
> On 2013-01-10, at 6:07 PM, Eric Sandeen wrote:
>> On 1/10/13 6:07 PM, Carlos Maiolino wrote:
>>> I'm working on a Fedora bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=852833)
>>> together with Eric and we found that the problem described on the
>>> bugzilla happens when the commit 93f9052643 is not applied, which
>>> is the case of the Fedora 16 kernel being discussed there.
>>
>> Also, to be clear, this is with older e2fsprogs which was using the
>> old resize interface.  Not sure what the behavior is w/ newer
>> e2fsprogs, but we don't see the corruption.
>>
>> Note, we see the corruption on these older kernels even when resizing from say 100G to 120G.  It appears fixed upstream, but so much has
>> changed, we need to be sure the older interface doesn't have bugs
>> lurking, I think.
> 
> The original resize code didn't ever know about uninit_bg, so it
> would always zero out the inode table, so I suspect that this was
> added at some later point. 

Ok, so that must have gotten dropped when everything was reworked
to the new interface.

We probably should have a (hidden?) switch in resize2fs to exercise
the old interface, so we can test this.

>>> Although we already found the solution to the problem in the commit
>>> above, looking through the commit have raised some questions
>>> regarding to the bitmap of the newer block groups added to the FS
>>> after it is extended.
>>>
>>> The newer block groups do not have flags ITABLE_ZEROED and
>>> INODE_UNINIT set, even when 'lazy_itable_init' is enabled.
>>
>> In particular, we see things like this in the last pre-resize group:
>>
>> Group 799: (Blocks 26181632-26214399) [INODE_UNINIT, ITABLE_ZEROED]
>>  Checksum 0xafe7, unused inodes 1936
>>  Block bitmap at 25165855 (bg #768 + 31), Inode bitmap at 25166111 (bg #768 + 287)
>>  Inode table at 25170087-25170207 (bg #768 + 4263)
>>  32768 free blocks, 1936 free inodes, 0 directories, 1936 unused inodes
>>  Free blocks: 26181632-26214399
>>  Free inodes: 1546865-1548800
>>
>> and this in the newly-added groups:
>>
>> Group 800: (Blocks 26214400-26247167)
>>  Checksum 0xddc4, unused inodes 0
>>  Block bitmap at 26236224 (+21824), Inode bitmap at 26236225 (+21825)
>>  Inode table at 26214400-26214520
>>  32645 free blocks, 1936 free inodes, 0 directories
>>  Free blocks: 26214521-26236223, 26236226-26247167
>>  Free inodes: 1548801-1550736
>>
>> so it says 0 unused inodes, but also 1936 free inodes (?), and
>> no UNINIT or ZEROED flags set.  e2fsck finds stale data in the inode table, and goes nuts.
> 
> Zeroing the inode table but not setting the INODE_ZEROED flag
> would not be harmful, but this seems to not be the case.

we appear to be not zeroing the table, and not setting INODE_ZEROED.
But we should have set INODE_UNINIT, or zeroed it, right?

> When the filesystem is remounted, does the kernel lazyinit
> thread zero out the new groups in the inode table?

Don't think so.

> 
>>> Without this commit, inode stale data might be exposed and also makes fsck complain about all inodes of the newer block groups.
>>
>> *nod* :)
> 
> That's why 
> 
>> so 93f9052643 seems to have accidentally fixed this, by setting
>> the unused counter to EXT4_INODES_PER_GROUP(), but it feels like
>> we've missed properly setting up this block group.
> 
> Actually, just setting the unused counter is not enough to properly
> fix this problem.  The lazyinit thread should be started to do
> background zeroing of the inode table, otherwise if the group
> descriptor is corrupted and the bg_itable_unused value is wrong,
> then the uninitialized inodes would be accessed.
> 
>> To be honest, though, sometimes I get lost in the sea of flags.
>>
>>> The question is, are these flags expected to not be found on
>>> these newer block groups or they should be set?
>>
>> *nod* :)
> 
> Depends on how it is implemented. :-/  The flag should definitely
> not be set unless the itable is actually overwritten with zeroes.
> It makes sense that the lazyinit thread would do this in the
> background while the filesystem is mounted instead of waiting for
> the next time that the filesystem is mounted.
> 
> Looking at the code, it appears that this is not happening at the
> end of the resize, since ext4_register_li_request() is marked
> static for the superblock.  It looks like it would be relatively
> straight forward to add a call to ext4_register_li_request() to
> ext4_resize_end() with the first group added to the resize.
> 
> It looks like ext4_run_li_request() will skip groups that are
> already marked as INODE_ZERO, so it is fine to always call it even
> if the kernel is already setting this itself in some cases (not
> that I see this happening).

Hum, but lazyinit will take some time to complete; in this case
we resized, unmounted, ran fsck and everything was a mess.  Even if
we'd started lazyinit I don't think that'ts enough, because we never
flagged the group as uninit.

>>> The lack of these flags on newer block groups is an expected
>>> behaviour or is something that should be fixed?
>>>
>>> FWIW, in the old ext4_group_add(), we added EXT4_BG_INODE_ZEROED
>>> flag to the bg_flags, also, I did some tests here and the lack
>>> of these flags look to not be affecting filesystem integrity,
>>> i.e. new inodes can be properly allocated, which sounds that
>>> these uninitialized inodes/bitmaps are set to be initialized
>>> as soon as a first inode is allocated on these new BGs.
> 
> Well, the old code always zeroed the inode table, and it made sense
> to mark it as such.

so I think we need to either:

1) mark it as UNINIT and start the background thread, or
2) just zero the darned thing out on resize.

and

3) make this testable, again.  :/

-Eric

> Cheers, Andreas
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: new block group bitmaps not being initialized after resize!?

[Theodore Ts'o]

On Fri, Jan 11, 2013 at 09:47:01AM -0600, Eric Sandeen wrote:
> > Zeroing the inode table but not setting the INODE_ZEROED flag
> > would not be harmful, but this seems to not be the case.
> 
> we appear to be not zeroing the table, and not setting INODE_ZEROED.
> But we should have set INODE_UNINIT, or zeroed it, right?

The flag names are confusing.  INODE_ZEROED means that the inode table
is not zero'ed.  This is correct.

INODE_UNINIT refers to the inode allocation bitmap not being
initialized, and what we are doing is correct as well.

What we should be doing is making sure that we kick off the lazyinit
thread.  So it's basically a missing call to
ext4_register_li_request(sb).

> Hum, but lazyinit will take some time to complete; in this case
> we resized, unmounted, ran fsck and everything was a mess.  Even if
> we'd started lazyinit I don't think that'ts enough, because we never
> flagged the group as uninit.

But as near as I can tell, at least with the latest upstream kernel,
the flags are being set correctly.  INODE_ZEROED is not being set, but
that's correct, because the inode table has not been zeroed.  

The real problem was the bug which was fixed by 93f905264; previous to
this commit, the unused inode count was incorrect.  My fault for not
understanding the consequences of this bug.  I was thinking in terms
of the e2fsck taking too long, but of course if there were previously
valid inodes in those blocks, e2fsck would in fact go crazy.  So that
commit really should have been marked cc: stable@vger.kernel.org.

Regards,

						- Ted

Re: new block group bitmaps not being initialized after resize!?

[Eric Sandeen]

On Jan 12, 2013, at 10:36 PM, "Theodore Ts'o" <tytso@mit.edu> wrote:

> On Fri, Jan 11, 2013 at 09:47:01AM -0600, Eric Sandeen wrote:
>>> Zeroing the inode table but not setting the INODE_ZEROED flag
>>> would not be harmful, but this seems to not be the case.
>> 
>> we appear to be not zeroing the table, and not setting INODE_ZEROED.
>> But we should have set INODE_UNINIT, or zeroed it, right?
> 
> The flag names are confusing.  INODE_ZEROED means that the inode table
> is not zero'ed.

Please tell me you said that backwards?

>  This is correct.
> 
> INODE_UNINIT refers to the inode allocation bitmap not being
> initialized, and what we are doing is correct as well.
> 
> What we should be doing is making sure that we kick off the lazyinit
> thread.  So it's basically a missing call to
> ext4_register_li_request(sb).
> 
>> Hum, but lazyinit will take some time to complete; in this case
>> we resized, unmounted, ran fsck and everything was a mess.  Even if
>> we'd started lazyinit I don't think that'ts enough, because we never
>> flagged the group as uninit.
> 
> But as near as I can tell, at least with the latest upstream kernel,
> the flags are being set correctly.  INODE_ZEROED is not being set, but
> that's correct, because the inode table has not been zeroed.  
> 
> The real problem was the bug which was fixed by 93f905264; previous to
> this commit, the unused inode count was incorrect.  My fault for not
> understanding the consequences of this bug.  I was thinking in terms
> of the e2fsck taking too long, but of course if there were previously
> valid inodes in those blocks, e2fsck would in fact go crazy.  So that
> commit really should have been marked cc: stable@vger.kernel.org.
> 
If setting the unused inode count is enough, then I guess it's resolved...
> Regards,
> 
>                        - Ted

Re: new block group bitmaps not being initialized after resize!?

[Theodore Ts'o]

On Sun, Jan 13, 2013 at 12:26:45AM -0500, Eric Sandeen wrote:
> > The flag names are confusing.  INODE_ZEROED means that the inode table
> > is not zero'ed.
> 
> Please tell me you said that backwards?

Yes, sorry...

> If setting the unused inode count is enough, then I guess it's resolved...

Well, modulo a patch to make sure the lazy init thread gets kicked
off, so that eventually we do end up zeroing the inode table (just in
case something goes wrong and we can't trust the block group
descriptor checksum).  I just finished that off last night, and it's
now ready for review (see next message).

						- Ted

[PATCH] ext4: trigger the lazy inode table initialization after resize

[Theodore Ts'o]

After we have finished extending the file system, we need to trigger a
the lazy inode table thread to zero out the inode tables.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h   |  2 ++
 fs/ext4/ioctl.c  |  9 +++++++++
 fs/ext4/resize.c |  8 +++++---
 fs/ext4/super.c  | 21 +++++++++++----------
 4 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8462eb3..8024623 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2227,6 +2227,8 @@ extern int ext4_group_desc_csum_verify(struct super_block *sb, __u32 group,
 				       struct ext4_group_desc *gdp);
 extern void ext4_group_desc_csum_set(struct super_block *sb, __u32 group,
 				     struct ext4_group_desc *gdp);
+extern int ext4_register_li_request(struct super_block *sb,
+				    ext4_group_t first_not_zeroed);
 
 static inline int ext4_has_group_desc_csum(struct super_block *sb)
 {
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 5747f52..4784ac2 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -313,6 +313,9 @@ mext_out:
 		if (err == 0)
 			err = err2;
 		mnt_drop_write_file(filp);
+		if (!err && ext4_has_group_desc_csum(sb) &&
+		    test_opt(sb, INIT_INODE_TABLE))
+			err = ext4_register_li_request(sb, input.group);
 group_add_out:
 		ext4_resize_end(sb);
 		return err;
@@ -358,6 +361,7 @@ group_add_out:
 		ext4_fsblk_t n_blocks_count;
 		struct super_block *sb = inode->i_sb;
 		int err = 0, err2 = 0;
+		ext4_group_t o_group = EXT4_SB(sb)->s_groups_count;
 
 		if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
 			       EXT4_FEATURE_RO_COMPAT_BIGALLOC)) {
@@ -388,6 +392,11 @@ group_add_out:
 		if (err == 0)
 			err = err2;
 		mnt_drop_write_file(filp);
+		if (!err && (o_group > EXT4_SB(sb)->s_groups_count) &&
+		    ext4_has_group_desc_csum(sb) &&
+		    test_opt(sb, INIT_INODE_TABLE))
+			err = ext4_register_li_request(sb, o_group);
+
 resizefs_out:
 		ext4_resize_end(sb);
 		return err;
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 05f8d45..8eefb63 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1506,10 +1506,12 @@ static int ext4_setup_next_flex_gd(struct super_block *sb,
 		group_data[i].blocks_count = blocks_per_group;
 		overhead = ext4_group_overhead_blocks(sb, group + i);
 		group_data[i].free_blocks_count = blocks_per_group - overhead;
-		if (ext4_has_group_desc_csum(sb))
+		if (ext4_has_group_desc_csum(sb)) {
 			flex_gd->bg_flags[i] = EXT4_BG_BLOCK_UNINIT |
 					       EXT4_BG_INODE_UNINIT;
-		else
+			if (!test_opt(sb, INIT_INODE_TABLE))
+				flex_gd->bg_flags[i] |= EXT4_BG_INODE_ZEROED;
+		} else
 			flex_gd->bg_flags[i] = EXT4_BG_INODE_ZEROED;
 	}
 
@@ -1594,7 +1596,7 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 
 	err = ext4_alloc_flex_bg_array(sb, input->group + 1);
 	if (err)
-		return err;
+		goto out;
 
 	err = ext4_mb_alloc_groupinfo(sb, input->group + 1);
 	if (err)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3d4fb81..c014edd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2776,7 +2776,7 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
 			break;
 	}
 
-	if (group == ngroups)
+	if (group >= ngroups)
 		ret = 1;
 
 	if (!ret) {
@@ -3016,33 +3016,34 @@ static struct ext4_li_request *ext4_li_request_new(struct super_block *sb,
 	return elr;
 }
 
-static int ext4_register_li_request(struct super_block *sb,
-				    ext4_group_t first_not_zeroed)
+int ext4_register_li_request(struct super_block *sb,
+			     ext4_group_t first_not_zeroed)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	struct ext4_li_request *elr;
+	struct ext4_li_request *elr = NULL;
 	ext4_group_t ngroups = EXT4_SB(sb)->s_groups_count;
 	int ret = 0;
 
+	mutex_lock(&ext4_li_mtx);
 	if (sbi->s_li_request != NULL) {
 		/*
 		 * Reset timeout so it can be computed again, because
 		 * s_li_wait_mult might have changed.
 		 */
 		sbi->s_li_request->lr_timeout = 0;
-		return 0;
+		goto out;
 	}
 
 	if (first_not_zeroed == ngroups ||
 	    (sb->s_flags & MS_RDONLY) ||
 	    !test_opt(sb, INIT_INODE_TABLE))
-		return 0;
+		goto out;
 
 	elr = ext4_li_request_new(sb, first_not_zeroed);
-	if (!elr)
-		return -ENOMEM;

Re: [PATCH] ext4: trigger the lazy inode table initialization after resize

[Carlos Maiolino]

Hi,

> @@ -358,6 +361,7 @@ group_add_out:
>  		ext4_fsblk_t n_blocks_count;
>  		struct super_block *sb = inode->i_sb;
>  		int err = 0, err2 = 0;
> +		ext4_group_t o_group = EXT4_SB(sb)->s_groups_count;
>  
>  		if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
>  			       EXT4_FEATURE_RO_COMPAT_BIGALLOC)) {
> @@ -388,6 +392,11 @@ group_add_out:
>  		if (err == 0)
>  			err = err2;
>  		mnt_drop_write_file(filp);
> +		if (!err && (o_group > EXT4_SB(sb)->s_groups_count) &&

Maybe a n00b question Ted, but can o_group here be bigger than ->s_groups_count
in any chance? 


The patch looks good,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

-- 
Carlos

Re: [PATCH] ext4: trigger the lazy inode table initialization after resize

[Theodore Ts'o]

On Mon, Jan 14, 2013 at 11:04:15AM -0200, Carlos Maiolino wrote:
> > @@ -388,6 +392,11 @@ group_add_out:
> >  		if (err == 0)
> >  			err = err2;
> >  		mnt_drop_write_file(filp);
> > +		if (!err && (o_group > EXT4_SB(sb)->s_groups_count) &&
> 
> Maybe a n00b question Ted, but can o_group here be bigger than ->s_groups_count
> in any chance? 

o_group can never be smaller than s_groups_count (since we don't
support online shrink).  o_group can be larger than s_groups_count if
ext4_resize_fs() has added one or more block groups to the file system
--- which is when we might need to kick off the lazy init thread.

Cheers,

						- Ted

Re: [PATCH] ext4: trigger the lazy inode table initialization after resize

[Carlos Maiolino]

On Mon, Jan 14, 2013 at 09:33:17AM -0500, Theodore Ts'o wrote:
> On Mon, Jan 14, 2013 at 11:04:15AM -0200, Carlos Maiolino wrote:
> > > @@ -388,6 +392,11 @@ group_add_out:
> > >  		if (err == 0)
> > >  			err = err2;
> > >  		mnt_drop_write_file(filp);
> > > +		if (!err && (o_group > EXT4_SB(sb)->s_groups_count) &&
> > 
> > Maybe a n00b question Ted, but can o_group here be bigger than ->s_groups_count
> > in any chance? 
> 
> o_group can never be smaller than s_groups_count (since we don't
> support online shrink).  o_group can be larger than s_groups_count if
> ext4_resize_fs() has added one or more block groups to the file system
> --- which is when we might need to kick off the lazy init thread.
> 
Thanks to catch the right question :)

> Cheers,
> 
> 						- Ted

-- 
Carlos

Re: [PATCH] ext4: trigger the lazy inode table initialization after resize

[Carlos Maiolino]

On Mon, Jan 14, 2013 at 09:33:17AM -0500, Theodore Ts'o wrote:
> On Mon, Jan 14, 2013 at 11:04:15AM -0200, Carlos Maiolino wrote:
> > > @@ -388,6 +392,11 @@ group_add_out:
> > >  		if (err == 0)
> > >  			err = err2;
> > >  		mnt_drop_write_file(filp);
> > > +		if (!err && (o_group > EXT4_SB(sb)->s_groups_count) &&
> > 
> > Maybe a n00b question Ted, but can o_group here be bigger than ->s_groups_count
> > in any chance? 
> 
> o_group can never be smaller than s_groups_count (since we don't
> support online shrink).  o_group can be larger than s_groups_count if
> ext4_resize_fs() has added one or more block groups to the file system
> --- which is when we might need to kick off the lazy init thread.
> 
> Cheers,
> 
s/bigger/smaller

Thanks to catch the right question.

-- 
Carlos