[PATCH] mke2fs: fix flexbg check to allow small filesystems

[PATCH] mke2fs: fix flexbg check to allow small filesystems

[Andreas Dilger]

Fix the flexbg check added in d988201ef9c so that it allows common
small filesystem configurations to succeed instead of reporting
a cryptic error message and failing due to a bad flex_bg layout:

    mke2fs -O flex_bg -b 4096 -I 1024 -F /tmp/tt 79106
    mke2fs 1.42.11 (09-Jul-2014)
    /tmp/tt: Invalid argument passed to ext2 library while setting
             up superblock

This check in ext2fs_initialize() was added to prevent the metadata
from being allocated beyond the end of the filesystem, but it is
also causing a wide range of failures for small filesystems.

Fix this by shrinking the flex_bg factor by half repeatedly until it
would be expected to fit into the current filesystem size, rather
than return an error as previously done.  Ideally it would only try
a smaller flex_bg factor if the default value was being used, and had
not been specified on the command line, but it isn't possible to see
if this is the case in ext2fs_initialize() or not.

Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
---
 lib/ext2fs/initialize.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
index de358fd..d58a664 100644
--- a/lib/ext2fs/initialize.c
+++ b/lib/ext2fs/initialize.c
@@ -91,7 +91,7 @@ errcode_t ext2fs_initialize(const char *name, int flags,
 	unsigned int	rem;
 	unsigned int	overhead = 0;
 	unsigned int	ipg;
-	unsigned int	flexbg_size;
+	unsigned int	flexbg_factor;
 	dgrp_t		i;
 	blk64_t		free_blocks;
 	blk64_t		flexbg_overhead;
@@ -428,18 +428,19 @@ ipg_retry:
 	 * This is a simple check, so that the backup superblock and
 	 * other feature related blocks are not considered.
 	 */
-	flexbg_size = 1 << fs->super->s_log_groups_per_flex;
+flex_retry:
+	flexbg_factor = 1 << super->s_log_groups_per_flex;
 	flexbg_overhead = super->s_first_data_block + 1 +
 		fs->desc_blocks + super->s_reserved_gdt_blocks +
-		(__u64)flexbg_size * (2 + fs->inode_blocks_per_group);
+		(__u64)flexbg_factor * (2 + fs->inode_blocks_per_group);
 
 	/*
 	 * Disallow creating ext4 which breaks flex_bg metadata layout
 	 * obviously.
 	 */
-	if (flexbg_overhead > ext2fs_blocks_count(fs->super)) {
-		retval = EXT2_ET_INVALID_ARGUMENT;
-		goto cleanup;
+	if (flexbg_overhead > ext2fs_blocks_count(super)) {
+		super->s_log_groups_per_flex -= 1;
+		goto flex_retry;
 	}
 
 	/*
-- 
1.7.3.4

RE: [PATCH] mke2fs: fix flexbg check to allow small filesystems

[Akira Fujita]

Hi Andreas,

>From: linux-ext4-owner@vger.kernel.org [mailto:linux-ext4-owner@vger.kernel.org] On Behalf Of
>Andreas Dilger
>Sent: Wednesday, July 30, 2014 5:30 PM
>To: tytso@mit.edu
>Cc: linux-ext4@vger.kernel.org; Andreas Dilger; Andreas Dilger
>Subject: [PATCH] mke2fs: fix flexbg check to allow small filesystems
>
>Fix the flexbg check added in d988201ef9c so that it allows common
>small filesystem configurations to succeed instead of reporting
>a cryptic error message and failing due to a bad flex_bg layout:
>
>    mke2fs -O flex_bg -b 4096 -I 1024 -F /tmp/tt 79106
>    mke2fs 1.42.11 (09-Jul-2014)
>    /tmp/tt: Invalid argument passed to ext2 library while setting
>             up superblock
>
>This check in ext2fs_initialize() was added to prevent the metadata
>from being allocated beyond the end of the filesystem, but it is
>also causing a wide range of failures for small filesystems.
>
>Fix this by shrinking the flex_bg factor by half repeatedly until it
>would be expected to fit into the current filesystem size, rather
>than return an error as previously done.  Ideally it would only try
>a smaller flex_bg factor if the default value was being used, and had
>not been specified on the command line, but it isn't possible to see
>if this is the case in ext2fs_initialize() or not.
>
>Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
>---
> lib/ext2fs/initialize.c |   13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
>index de358fd..d58a664 100644
>--- a/lib/ext2fs/initialize.c
>+++ b/lib/ext2fs/initialize.c
>@@ -91,7 +91,7 @@ errcode_t ext2fs_initialize(const char *name, int flags,
> 	unsigned int	rem;
> 	unsigned int	overhead = 0;
> 	unsigned int	ipg;
>-	unsigned int	flexbg_size;
>+	unsigned int	flexbg_factor;
> 	dgrp_t		i;
> 	blk64_t		free_blocks;
> 	blk64_t		flexbg_overhead;
>@@ -428,18 +428,19 @@ ipg_retry:
> 	 * This is a simple check, so that the backup superblock and
> 	 * other feature related blocks are not considered.
> 	 */
>-	flexbg_size = 1 << fs->super->s_log_groups_per_flex;
>+flex_retry:
>+	flexbg_factor = 1 << super->s_log_groups_per_flex;
> 	flexbg_overhead = super->s_first_data_block + 1 +
> 		fs->desc_blocks + super->s_reserved_gdt_blocks +
>-		(__u64)flexbg_size * (2 + fs->inode_blocks_per_group);
>+		(__u64)flexbg_factor * (2 + fs->inode_blocks_per_group);
>
> 	/*
> 	 * Disallow creating ext4 which breaks flex_bg metadata layout
> 	 * obviously.
> 	 */

This comment is unneeded any more,
But patch looks good to me, thanks.

Reviewed-by: Akira Fujita <a-fujita@rs.jp.nec.co.jp>


>-	if (flexbg_overhead > ext2fs_blocks_count(fs->super)) {
>-		retval = EXT2_ET_INVALID_ARGUMENT;
>-		goto cleanup;
>+	if (flexbg_overhead > ext2fs_blocks_count(super)) {
>+		super->s_log_groups_per_flex -= 1;
>+		goto flex_retry;
> 	}
>
> 	/*
>--
>1.7.3.4
>