[PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

[PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

[Yury V. Zaytsev]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

Hi!

I have recently stumbled upon a RHEL6.1 installation problem on an SSD
drive that was working fine with older kernels, where created file
systems could not be mounted and the installation was aborting.

It turned out that the drive had a firmware bug and was exposing a
minimum_io_size of 8912, whereas my x86_64 system had a page size of
4096 bytes.

mkfs in turn was silently creating file systems with 8912 bytes block
size, because it was auto-detected as opposed to specified via the -b
option from the command line.

This made the problem hard to debug, hence I attach a tentative patch to
fix this issue. I'd like to acknowledge Mike Snitzer for directing me to
this list.

I'm not subscribed, so please CC: me upon replies and be mindful that
it's my first contribution of this kind, so even though I did my best to
do it the right way, I might have screwed something up.

Thanks!

-- 
Sincerely yours,
Yury V. Zaytsev


[-- Attachment #2: 0001-mke2fs-check-that-auto-detected-blocksize-sys_page_s.patch --]
[-- Type: text/x-patch, Size: 2446 bytes --]

>From 5dcc3266d46ee3846bd8707c7b99bd312fe2276b Mon Sep 17 00:00:00 2001
From: "Yury V. Zaytsev" <yury@shurup.com>
Date: Wed, 8 Jun 2011 11:12:54 +0200
Subject: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

Block size can be specified manually via the -b option or deduced
automatically. Unfortunately, the check that it is still smaller than
the system page size is only performed right after the command line
options were parsed.

Therefore, if buggy or inappropriately installed/configured hardware
hints that larger block sizes have to be used, mkfs will silently create
a file system which can not be mounted on the system in question.

By moving the check beyond the last assignment to blocksize it is now
ensured, that mkfs will issue a warning even if inappropriate blocksize
was auto-detected.

The new behavior can be easily tested, by exporting the following
variables before running mkfs:

    export MKE2FS_DEVICE_SECTSIZE=8912
    export MKE2FS_DEVICE_PHYS_SECTSIZE=8912

Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
---
 misc/mke2fs.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index e28828e..b600617 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1526,17 +1526,6 @@ static void PRS(int argc, char *argv[])
 		ext2fs_close(jfs);
 	}
 
-	if (blocksize > sys_page_size) {
-		if (!force) {
-			com_err(program_name, 0,
-				_("%d-byte blocks too big for system (max %d)"),
-				blocksize, sys_page_size);
-			proceed_question();
-		}
-		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
-				  "(max %d), forced to continue\n"),
-			blocksize, sys_page_size);
-	}
 	if (optind < argc) {
 		fs_param.s_blocks_count = parse_num_blocks(argv[optind++],
 				fs_param.s_log_block_size);
@@ -1811,6 +1800,19 @@ got_size:
 
 	blocksize = EXT2_BLOCK_SIZE(&fs_param);
 
+	/* This check should happen beyond the last assignment to blocksize */
+	if (blocksize > sys_page_size) {
+		if (!force) {
+			com_err(program_name, 0,
+				_("%d-byte blocks too big for system (max %d)"),
+				blocksize, sys_page_size);
+			proceed_question();
+		}
+		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
+				  "(max %d), forced to continue\n"),
+			blocksize, sys_page_size);
+	}
+
 	lazy_itable_init = 0;
 	if (access("/sys/fs/ext4/features/lazy_itable_init", R_OK) == 0)
 		lazy_itable_init = 1;
-- 
1.7.5.4

Re: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

[Amir Goldstein]

On Wed, Jun 8, 2011 at 12:23 PM, Yury V. Zaytsev <yury@shurup.com> wrote:
> Hi!
>
> I have recently stumbled upon a RHEL6.1 installation problem on an SSD
> drive that was working fine with older kernels, where created file
> systems could not be mounted and the installation was aborting.
>
> It turned out that the drive had a firmware bug and was exposing a
> minimum_io_size of 8912, whereas my x86_64 system had a page size of
> 4096 bytes.
>
> mkfs in turn was silently creating file systems with 8912 bytes block
> size, because it was auto-detected as opposed to specified via the -b
> option from the command line.
>
> This made the problem hard to debug, hence I attach a tentative patch to
> fix this issue. I'd like to acknowledge Mike Snitzer for directing me to
> this list.
>
> I'm not subscribed, so please CC: me upon replies and be mindful that
> it's my first contribution of this kind, so even though I did my best to
> do it the right way, I might have screwed something up.

The only thing you screwed up is sending the patch as an attachment.
People here expect it to be as plain text in the email, so that they can
reply and comment inline.
Other than that, the fix looks good (and well explained) to me.

Cheers,
Amir.

>
> Thanks!
>
> --
> Sincerely yours,
> Yury V. Zaytsev
>
>

Re: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

[Yury V. Zaytsev]

On Wed, 2011-06-08 at 15:50 +0300, Amir Goldstein wrote:

> The only thing you screwed up is sending the patch as an attachment.
> People here expect it to be as plain text in the email, so that they
> can reply and comment inline. Other than that, the fix looks good (and
> well explained) to me.

Hi Amir!

Thank you for reviewing my submission. I apologize for attaching the
patch the wrong way. Hopefully, this time I will be able to make my mail
program to include it inline.

I have refreshed patch against the latest "next" branch since it no
longer properly applied and hopefully it can now be included!

--Yury.

>From 1f8e5ad235694f2918fb442135619d4988366434 Mon Sep 17 00:00:00 2001
From: "Yury V. Zaytsev" <yury@shurup.com>
Date: Mon, 13 Jun 2011 00:35:05 +0200
Subject: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

Block size can be specified manually via the -b option or deduced
automatically. Unfortunately, the check that it is still smaller than
the system page size is only performed right after the command line
options are parsed.

Therefore, if buggy or inappropriately installed/configured hardware
hints that larger block sizes have to be used, mkfs will silently create
a file system which can not be mounted on the system in question.

By moving the check beyond the last assignment to blocksize it is now
ensured, that mkfs will issue a warning even if inappropriate blocksize
was auto-detected.

The new behavior can be easily tested, by exporting the following
variables before running mkfs:

    export MKE2FS_DEVICE_SECTSIZE=8912
    export MKE2FS_DEVICE_PHYS_SECTSIZE=8912

Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
---
 misc/mke2fs.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 5ff3f9f..d699b46 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1516,17 +1516,6 @@ profile_error:
 		ext2fs_close(jfs);
 	}
 
-	if (blocksize > sys_page_size) {
-		if (!force) {
-			com_err(program_name, 0,
-				_("%d-byte blocks too big for system (max %d)"),
-				blocksize, sys_page_size);
-			proceed_question();
-		}
-		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
-				  "(max %d), forced to continue\n"),
-			blocksize, sys_page_size);
-	}
 	if (optind < argc) {
 		fs_blocks_count = parse_num_blocks2(argv[optind++],
 						   fs_param.s_log_block_size);
@@ -1825,6 +1814,19 @@ profile_error:
 
 	blocksize = EXT2_BLOCK_SIZE(&fs_param);
 
+	/* This check should happen beyond the last assignment to blocksize */
+	if (blocksize > sys_page_size) {
+		if (!force) {
+			com_err(program_name, 0,
+				_("%d-byte blocks too big for system (max %d)"),
+				blocksize, sys_page_size);
+			proceed_question();
+		}
+		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
+				  "(max %d), forced to continue\n"),
+			blocksize, sys_page_size);
+	}
+
 	lazy_itable_init = 0;
 	if (access("/sys/fs/ext4/features/lazy_itable_init", R_OK) == 0)
 		lazy_itable_init = 1;
-- 
1.7.5.4

Re: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

[Yury V. Zaytsev]

Hi!

On Mon, 2011-06-13 at 00:46 +0200, Yury V. Zaytsev wrote:

> I have refreshed patch against the latest "next" branch since it no
> longer properly applied and hopefully it can now be included!

I would highly appreciate if the trivial patch that nonetheless fixes an
important issue to my mind posted last month could be looked at.

Is there anything else I could do to help with the inclusion? Sorry if I
am bumping the thread too fast, but I see other patches flowing in which
makes me think that I've done something wrong; if this is the case
please do let me know, I am willing to rectify it.

Thanks!

-- 
Sincerely yours,
Yury V. Zaytsev

> From 1f8e5ad235694f2918fb442135619d4988366434 Mon Sep 17 00:00:00 2001
> From: "Yury V. Zaytsev" <yury@shurup.com>
> Date: Mon, 13 Jun 2011 00:35:05 +0200
> Subject: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size
> 
> Block size can be specified manually via the -b option or deduced
> automatically. Unfortunately, the check that it is still smaller than
> the system page size is only performed right after the command line
> options are parsed.
> 
> Therefore, if buggy or inappropriately installed/configured hardware
> hints that larger block sizes have to be used, mkfs will silently create
> a file system which can not be mounted on the system in question.
> 
> By moving the check beyond the last assignment to blocksize it is now
> ensured, that mkfs will issue a warning even if inappropriate blocksize
> was auto-detected.
> 
> The new behavior can be easily tested, by exporting the following
> variables before running mkfs:
> 
>     export MKE2FS_DEVICE_SECTSIZE=8912
>     export MKE2FS_DEVICE_PHYS_SECTSIZE=8912
> 
> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
> ---
>  misc/mke2fs.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 5ff3f9f..d699b46 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1516,17 +1516,6 @@ profile_error:
>  		ext2fs_close(jfs);
>  	}
>  
> -	if (blocksize > sys_page_size) {
> -		if (!force) {
> -			com_err(program_name, 0,
> -				_("%d-byte blocks too big for system (max %d)"),
> -				blocksize, sys_page_size);
> -			proceed_question();
> -		}
> -		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
> -				  "(max %d), forced to continue\n"),
> -			blocksize, sys_page_size);
> -	}
>  	if (optind < argc) {
>  		fs_blocks_count = parse_num_blocks2(argv[optind++],
>  						   fs_param.s_log_block_size);
> @@ -1825,6 +1814,19 @@ profile_error:
>  
>  	blocksize = EXT2_BLOCK_SIZE(&fs_param);
>  
> +	/* This check should happen beyond the last assignment to blocksize */
> +	if (blocksize > sys_page_size) {
> +		if (!force) {
> +			com_err(program_name, 0,
> +				_("%d-byte blocks too big for system (max %d)"),
> +				blocksize, sys_page_size);
> +			proceed_question();
> +		}
> +		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
> +				  "(max %d), forced to continue\n"),
> +			blocksize, sys_page_size);
> +	}
> +
>  	lazy_itable_init = 0;
>  	if (access("/sys/fs/ext4/features/lazy_itable_init", R_OK) == 0)
>  		lazy_itable_init = 1;

Re: [PATCH] mke2fs: check that auto-detected blocksize <= sys_page_size

[Lukas Czerner]

On Mon, 13 Jun 2011, Yury V. Zaytsev wrote:

> On Wed, 2011-06-08 at 15:50 +0300, Amir Goldstein wrote:
> 
> > The only thing you screwed up is sending the patch as an attachment.
> > People here expect it to be as plain text in the email, so that they
> > can reply and comment inline. Other than that, the fix looks good (and
> > well explained) to me.
> 
> Hi Amir!
> 
> Thank you for reviewing my submission. I apologize for attaching the
> patch the wrong way. Hopefully, this time I will be able to make my mail
> program to include it inline.
> 
> I have refreshed patch against the latest "next" branch since it no
> longer properly applied and hopefully it can now be included!
> 
> --Yury.

Hi Yury,

the patch looks good. You can add my

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

but could you please resend the patch in proper format indicating second
version ? So Ted does not have to dig up the patch.

[PATCH v2] mke2fs: check that auto-detected blocksize <= sys_page_size

Thanks!
-Lukas

> 
> From 1f8e5ad235694f2918fb442135619d4988366434 Mon Sep 17 00:00:00 2001
> From: "Yury V. Zaytsev" <yury@shurup.com>
> Date: Mon, 13 Jun 2011 00:35:05 +0200
> Subject: 
> 
> Block size can be specified manually via the -b option or deduced
> automatically. Unfortunately, the check that it is still smaller than
> the system page size is only performed right after the command line
> options are parsed.
> 
> Therefore, if buggy or inappropriately installed/configured hardware
> hints that larger block sizes have to be used, mkfs will silently create
> a file system which can not be mounted on the system in question.
> 
> By moving the check beyond the last assignment to blocksize it is now
> ensured, that mkfs will issue a warning even if inappropriate blocksize
> was auto-detected.
> 
> The new behavior can be easily tested, by exporting the following
> variables before running mkfs:
> 
>     export MKE2FS_DEVICE_SECTSIZE=8912
>     export MKE2FS_DEVICE_PHYS_SECTSIZE=8912
> 
> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
> ---
>  misc/mke2fs.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 5ff3f9f..d699b46 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1516,17 +1516,6 @@ profile_error:
>  		ext2fs_close(jfs);
>  	}
>  
> -	if (blocksize > sys_page_size) {
> -		if (!force) {
> -			com_err(program_name, 0,
> -				_("%d-byte blocks too big for system (max %d)"),
> -				blocksize, sys_page_size);
> -			proceed_question();
> -		}
> -		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
> -				  "(max %d), forced to continue\n"),
> -			blocksize, sys_page_size);
> -	}
>  	if (optind < argc) {
>  		fs_blocks_count = parse_num_blocks2(argv[optind++],
>  						   fs_param.s_log_block_size);
> @@ -1825,6 +1814,19 @@ profile_error:
>  
>  	blocksize = EXT2_BLOCK_SIZE(&fs_param);
>  
> +	/* This check should happen beyond the last assignment to blocksize */
> +	if (blocksize > sys_page_size) {
> +		if (!force) {
> +			com_err(program_name, 0,
> +				_("%d-byte blocks too big for system (max %d)"),
> +				blocksize, sys_page_size);
> +			proceed_question();
> +		}
> +		fprintf(stderr, _("Warning: %d-byte blocks too big for system "
> +				  "(max %d), forced to continue\n"),
> +			blocksize, sys_page_size);
> +	}
> +
>  	lazy_itable_init = 0;
>  	if (access("/sys/fs/ext4/features/lazy_itable_init", R_OK) == 0)
>  		lazy_itable_init = 1;
> 

--