Patchwork [RFC,1/2] ext4 resize: Mark the added group with EXT4_BG_INODE_ZEROED flag

login
register
mail settings
Submitter Solofo.Ramangalahy@bull.net
Date Nov. 21, 2008, 10:23 a.m.
Message ID <20081121102309.507553245@bull.net>
Download mbox | patch
Permalink /patch/9981/
State RFC
Headers show

Comments

Solofo.Ramangalahy@bull.net - Nov. 21, 2008, 10:23 a.m.
The inode table has been zeroed in setup_new_group_blocks().
Mark it as such in ext4_group_add().

As a side note, online resize and inode zeroing are "dual".

In order to obtain a filesystem with faster formating times one can
do:
. either format a smaller fs and then resize it,
. or format the fs with lazy_itable_init

---
 fs/ext4/resize.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
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
Andreas Dilger - Nov. 24, 2008, 11:25 p.m.
On Nov 21, 2008  11:23 +0100, Solofo.Ramangalahy@bull.net wrote:
> The inode table has been zeroed in setup_new_group_blocks().
> Mark it as such in ext4_group_add().
> 
> As a side note, online resize and inode zeroing are "dual".
> 
> In order to obtain a filesystem with faster formating times one can
> do:
> . either format a smaller fs and then resize it,
> . or format the fs with lazy_itable_init

As discussed on the concall, it probably makes more sense to have the
resize code work by marking the inode tables UNINIT (if GDT_CSUM feature
is enabled) and then start the "itable zeroing" thread, if it isn't
already running, to do the zeroing of the itable.

If GDT_CSUM isn't set then the below fix is the right solution.

> Index: linux-2.6.28-rc4-itable_init/fs/ext4/resize.c
> ===================================================================
> --- linux-2.6.28-rc4-itable_init.orig/fs/ext4/resize.c
> +++ linux-2.6.28-rc4-itable_init/fs/ext4/resize.c
> @@ -865,7 +865,7 @@ int ext4_group_add(struct super_block *s
>  	gdp->bg_free_blocks_count = cpu_to_le16(input->free_blocks_count);
>  	gdp->bg_free_inodes_count = cpu_to_le16(EXT4_INODES_PER_GROUP(sb));
>  	gdp->bg_checksum = ext4_group_desc_csum(sbi, input->group, gdp);
> -
> +	gdp->bg_flags |= cpu_to_le16(EXT4_BG_INODE_ZEROED);
>  	/*
>  	 * We can allocate memory for mb_alloc based on the new group
>  	 * descriptor

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
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
Solofo.Ramangalahy@bull.net - Nov. 25, 2008, 11:27 a.m.
Andreas Dilger writes:
 > > As a side note, online resize and inode zeroing are "dual".
 > > 
 > > In order to obtain a filesystem with faster formating times one can
 > > do:
 > > . either format a smaller fs and then resize it,
 > > . or format the fs with lazy_itable_init
 > 
 > As discussed on the concall, it probably makes more sense to have the
 > resize code work by marking the inode tables UNINIT (if GDT_CSUM feature
 > is enabled)

If I understand correctly, this is already the case:
#define EXT4_BG_INODE_UNINIT    0x0001 /* Inode table/bitmap not in use */
#define EXT4_BG_BLOCK_UNINIT    0x0002 /* Block bitmap not in use */
#define EXT4_BG_INODE_ZEROED    0x0004 /* On-disk itable initialized to zero */

As the EXT4_BG_INODE_ZEROED is not present, the inode table is UNINIT.

By the way, is there any reason the #defines are like this, instead of:
#define EXT4_BG_INODE_UNINIT    0x0001 /* Inode table/bitmap not in use */
#define EXT4_BG_BLOCK_UNINIT    0x0002 /* Block bitmap not in use */
#define EXT4_BG_ITABLE_UNINIT   0x0004 /* On-disk itable not initialized */
?

 > and then start the "itable zeroing" thread, if it isn't
 > already running, to do the zeroing of the itable.

Yes.

Is there other resize changes you could think of?
While working on this, I noted this checkpatch error 
"ERROR: do not use assignment in if condition"
(but I am not sure of the exact justification).

Thanks,
Andreas Dilger - Nov. 25, 2008, 9:18 p.m.
On Nov 25, 2008  12:27 +0100, Solofo.Ramangalahy@bull.net wrote:
> Andreas Dilger writes:
>  > As discussed on the concall, it probably makes more sense to have the
>  > resize code work by marking the inode tables UNINIT (if GDT_CSUM feature
>  > is enabled)
> 
> If I understand correctly, this is already the case:
> #define EXT4_BG_INODE_UNINIT    0x0001 /* Inode table/bitmap not in use */
> #define EXT4_BG_BLOCK_UNINIT    0x0002 /* Block bitmap not in use */
> #define EXT4_BG_INODE_ZEROED    0x0004 /* On-disk itable initialized to zero */

> As the EXT4_BG_INODE_ZEROED is not present, the inode table is UNINIT.

Ah, I suppose you are correct.

> By the way, is there any reason the #defines are like this, instead of:
> #define EXT4_BG_INODE_UNINIT    0x0001 /* Inode table/bitmap not in use */
> #define EXT4_BG_BLOCK_UNINIT    0x0002 /* Block bitmap not in use */
> #define EXT4_BG_ITABLE_UNINIT   0x0004 /* On-disk itable not initialized */
> ?

No particular reason, that is just how the implementation was done.  In
hindsight that probably would make more sense...

> While working on this, I noted this checkpatch error 
> "ERROR: do not use assignment in if condition"
> (but I am not sure of the exact justification).

I'm not sure what you are asking.  The reason not to use "assignment in if"
is because of possible coding error like:

	if (x = 6) {
		/* do something if x is 6 */
	}

when coder actually meant to write:

	if (x == 6) {
		/* do something if x is 6 */
	}

The first one will now generate a warning in GCC unless written as:

	if ((x = 6)) {
		/* do something if x is 6 */
	}

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
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

Patch

Index: linux-2.6.28-rc4-itable_init/fs/ext4/resize.c
===================================================================
--- linux-2.6.28-rc4-itable_init.orig/fs/ext4/resize.c
+++ linux-2.6.28-rc4-itable_init/fs/ext4/resize.c
@@ -865,7 +865,7 @@  int ext4_group_add(struct super_block *s
 	gdp->bg_free_blocks_count = cpu_to_le16(input->free_blocks_count);
 	gdp->bg_free_inodes_count = cpu_to_le16(EXT4_INODES_PER_GROUP(sb));
 	gdp->bg_checksum = ext4_group_desc_csum(sbi, input->group, gdp);
-
+	gdp->bg_flags |= cpu_to_le16(EXT4_BG_INODE_ZEROED);
 	/*
 	 * We can allocate memory for mb_alloc based on the new group
 	 * descriptor