[PATCH] e2fsck: reopen the file system with saved flags after a journal replay

[PATCH] e2fsck: reopen the file system with saved flags after a journal replay

[Theodore Ts'o]

After a journal replay, we close and reopen the file system so that
any changes in the superblock can get reflected in the libext2fs's
internal data structures.  We need to save the flags passed to
ext2fs_open() that we used when we originally opened the file system.

Otherwise we will end up not be able to repair a file system which
requires a journal replay and which has bigalloc enabled or which has
more than 2**32 blocks; e2fsck will abort with the error message:

fsck.ext4: Filesystem too large to use legacy bitmaps while trying to re-open

Addresses-Debian-Bug: 744953
Cc: Андрей Василишин <a.vasilishin@kpi.ua>
Cc: Jon Severinsson <jon@severinsson.net>
Cc: 744953@bugs.debian.org
---

Distributions will almost certainly want to backport this commit.  I
plan to release e2fsprogs 1.42.11, which will include this fix.

 e2fsck/e2fsck.h  | 1 +
 e2fsck/journal.c | 2 +-
 e2fsck/unix.c    | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index c71a0a5..998abdc 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -232,6 +232,7 @@ struct e2fsck_struct {
 	blk64_t free_blocks;
 	ino_t	free_inodes;
 	int	mount_flags;
+	int	openfs_flags;
 	blkid_cache blkid;	/* blkid cache */
 
 #ifdef HAVE_SETJMP_H
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index 905c0bf..9be52cd 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -903,7 +903,7 @@ errcode_t e2fsck_run_ext3_journal(e2fsck_t ctx)
 
 	ext2fs_mmp_stop(ctx->fs);
 	ext2fs_free(ctx->fs);
-	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
+	retval = ext2fs_open(ctx->filesystem_name, ctx->openfs_flags,
 			     ctx->superblock, blocksize, io_ptr,
 			     &ctx->fs);
 	if (retval) {
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index b265c99..03848c7 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1274,6 +1274,7 @@ restart:
 			flags &= ~EXT2_FLAG_EXCLUSIVE;
 	}
 
+	ctx->openfs_flags = flags;
 	retval = try_open_fs(ctx, flags, io_ptr, &fs);
 
 	if (!ctx->superblock && !(ctx->options & E2F_OPT_PREEN) &&
-- 
2.0.0

--
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] e2fsck: reopen the file system with saved flags after a journal replay

[Theodore Ts'o]

After a journal replay, we close and reopen the file system so that
any changes in the superblock can get reflected in the libext2fs's
internal data structures.  We need to save the flags passed to
ext2fs_open() that we used when we originally opened the file system.

Otherwise we could end up triggering the following error message when
checking a large (or bigalloc) file system after an unclean shutdown:

fsck.ext4: Filesystem too large to use legacy bitmaps while trying to re-open

Addresses-Debian-Bug: 744953
Cc: Андрей Василишин <a.vasilishin@kpi.ua>
Cc: Jon Severinsson <jon@severinsson.net>
Cc: 744953@bugs.debian.org
---

Distributions will almost certainly want to backport this patch, since
it breaks running e2fsck on file system with the 64-bit or bigalloc
feature enabled 

 e2fsck/e2fsck.h  | 1 +
 e2fsck/journal.c | 2 +-
 e2fsck/unix.c    | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index c71a0a5..998abdc 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -232,6 +232,7 @@ struct e2fsck_struct {
 	blk64_t free_blocks;
 	ino_t	free_inodes;
 	int	mount_flags;
+	int	openfs_flags;
 	blkid_cache blkid;	/* blkid cache */
 
 #ifdef HAVE_SETJMP_H
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index 905c0bf..9be52cd 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -903,7 +903,7 @@ errcode_t e2fsck_run_ext3_journal(e2fsck_t ctx)
 
 	ext2fs_mmp_stop(ctx->fs);
 	ext2fs_free(ctx->fs);
-	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
+	retval = ext2fs_open(ctx->filesystem_name, ctx->openfs_flags,
 			     ctx->superblock, blocksize, io_ptr,
 			     &ctx->fs);
 	if (retval) {
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index b265c99..03848c7 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1274,6 +1274,7 @@ restart:
 			flags &= ~EXT2_FLAG_EXCLUSIVE;
 	}
 
+	ctx->openfs_flags = flags;
 	retval = try_open_fs(ctx, flags, io_ptr, &fs);
 
 	if (!ctx->superblock && !(ctx->options & E2F_OPT_PREEN) &&
-- 
2.0.0

--
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: [PATCH] e2fsck: reopen the file system with saved flags after a journal replay

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3181 bytes --]

On Sat, 5 Jul 2014, Theodore Ts'o wrote:

> Date: Sat,  5 Jul 2014 23:09:57 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>, Андрей Василишин <a.vasilishin@kpi.ua>,
>     Jon Severinsson <jon@severinsson.net>, 744953@bugs.debian.org
> Subject: [PATCH] e2fsck: reopen the file system with saved flags after a
>     journal replay
> 
> After a journal replay, we close and reopen the file system so that
> any changes in the superblock can get reflected in the libext2fs's
> internal data structures.  We need to save the flags passed to
> ext2fs_open() that we used when we originally opened the file system.
> 
> Otherwise we could end up triggering the following error message when
> checking a large (or bigalloc) file system after an unclean shutdown:
> 
> fsck.ext4: Filesystem too large to use legacy bitmaps while trying to re-open

Hi Ted,

couple of comments below.

> 
> Addresses-Debian-Bug: 744953
> Cc: Андрей Василишин <a.vasilishin@kpi.ua>
> Cc: Jon Severinsson <jon@severinsson.net>
> Cc: 744953@bugs.debian.org
> ---
> 
> Distributions will almost certainly want to backport this patch, since
> it breaks running e2fsck on file system with the 64-bit or bigalloc
> feature enabled 
> 
>  e2fsck/e2fsck.h  | 1 +
>  e2fsck/journal.c | 2 +-
>  e2fsck/unix.c    | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index c71a0a5..998abdc 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -232,6 +232,7 @@ struct e2fsck_struct {
>  	blk64_t free_blocks;
>  	ino_t	free_inodes;
>  	int	mount_flags;
> +	int	openfs_flags;
>  	blkid_cache blkid;	/* blkid cache */
>  
>  #ifdef HAVE_SETJMP_H
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index 905c0bf..9be52cd 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -903,7 +903,7 @@ errcode_t e2fsck_run_ext3_journal(e2fsck_t ctx)
>  
>  	ext2fs_mmp_stop(ctx->fs);
>  	ext2fs_free(ctx->fs);
> -	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
> +	retval = ext2fs_open(ctx->filesystem_name, ctx->openfs_flags,
>  			     ctx->superblock, blocksize, io_ptr,
>  			     &ctx->fs);
>  	if (retval) {
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index b265c99..03848c7 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1274,6 +1274,7 @@ restart:
>  			flags &= ~EXT2_FLAG_EXCLUSIVE;
>  	}
>  
> +	ctx->openfs_flags = flags;
>  	retval = try_open_fs(ctx, flags, io_ptr, &fs);

Maybe we can get rid of 'flags' argument since it's not needed
anymore ?

>  
>  	if (!ctx->superblock && !(ctx->options & E2F_OPT_PREEN) &&
> 

Otherwise the patch looks good, however for some reason I can not
reproduce the problem in the big file system (without bigalloc) even
though looking at bitmap.c it looks like we really should get that
error.

The reason is that after the journal recovery we're going to set the
flags again properly and rerun the fsck so everything should be
fine. But I wonder whether we're actually going to need that flag,
but looking at e2fsck_check_ext3_journal() it looks like we might if
something is going bad ?

Thanks!
-Lukas

Re: [PATCH] e2fsck: reopen the file system with saved flags after a journal replay

[Theodore Ts'o]

On Tue, Jul 08, 2014 at 11:03:53AM +0200, Lukáš Czerner wrote:
> > +	ctx->openfs_flags = flags;
> >  	retval = try_open_fs(ctx, flags, io_ptr, &fs);
> 
> Maybe we can get rid of 'flags' argument since it's not needed
> anymore ?

Yeah, I thought of that, but I was trying to keep the patch small,
since I thought this was one that distros might want to cherry pick.

> Otherwise the patch looks good, however for some reason I can not
> reproduce the problem in the big file system (without bigalloc) even
> though looking at bitmap.c it looks like we really should get that
> error.

Ah, yes, in the big file system case we don't hit it because we don't
actually load the bitmaps before we close and reopen the file system
again.  But in the bigalloc case, we check and fail at openfs time,
which aborts the fsck run.

So we could work around this by moving the bigalloc check to when we
open the file system, but you want get the other openfs flags right,
especially the EXT2_FLAG_SKIP_MMP flags.

Cheers,

						- Ted
--
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: [PATCH] e2fsck: reopen the file system with saved flags after a journal replay

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1891 bytes --]

On Tue, 8 Jul 2014, Theodore Ts'o wrote:

> Date: Tue, 8 Jul 2014 07:58:27 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>,
>     Андрей Василишин <a.vasilishin@kpi.ua>,
>     Jon Severinsson <jon@severinsson.net>, 744953@bugs.debian.org
> Subject: Re: [PATCH] e2fsck: reopen the file system with saved flags after a
>     journal replay
> 
> On Tue, Jul 08, 2014 at 11:03:53AM +0200, Lukáš Czerner wrote:
> > > +	ctx->openfs_flags = flags;
> > >  	retval = try_open_fs(ctx, flags, io_ptr, &fs);
> > 
> > Maybe we can get rid of 'flags' argument since it's not needed
> > anymore ?
> 
> Yeah, I thought of that, but I was trying to keep the patch small,
> since I thought this was one that distros might want to cherry pick.

Makes sense.

> 
> > Otherwise the patch looks good, however for some reason I can not
> > reproduce the problem in the big file system (without bigalloc) even
> > though looking at bitmap.c it looks like we really should get that
> > error.
> 
> Ah, yes, in the big file system case we don't hit it because we don't
> actually load the bitmaps before we close and reopen the file system
> again.  But in the bigalloc case, we check and fail at openfs time,
> which aborts the fsck run.
> 
> So we could work around this by moving the bigalloc check to when we
> open the file system, but you want get the other openfs flags right,
> especially the EXT2_FLAG_SKIP_MMP flags.

Yep, I agree with the fix, I was just wondering about the big fs
case. Anyway you can add:

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

> 
> Cheers,
> 
> 						- Ted
> --
> 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: [PATCH] e2fsck: reopen the file system with saved flags after a journal replay

[Darrick J. Wong]

On Sat, Jul 05, 2014 at 11:09:57PM -0400, Theodore Ts'o wrote:
> After a journal replay, we close and reopen the file system so that
> any changes in the superblock can get reflected in the libext2fs's
> internal data structures.  We need to save the flags passed to
> ext2fs_open() that we used when we originally opened the file system.
> 
> Otherwise we could end up triggering the following error message when
> checking a large (or bigalloc) file system after an unclean shutdown:
> 
> fsck.ext4: Filesystem too large to use legacy bitmaps while trying to re-open

I had a patch in my tree that fixed this too, so you can add:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Addresses-Debian-Bug: 744953
> Cc: Андрей Василишин <a.vasilishin@kpi.ua>
> Cc: Jon Severinsson <jon@severinsson.net>
> Cc: 744953@bugs.debian.org
> ---
> 
> Distributions will almost certainly want to backport this patch, since
> it breaks running e2fsck on file system with the 64-bit or bigalloc
> feature enabled 
> 
>  e2fsck/e2fsck.h  | 1 +
>  e2fsck/journal.c | 2 +-
>  e2fsck/unix.c    | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index c71a0a5..998abdc 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -232,6 +232,7 @@ struct e2fsck_struct {
>  	blk64_t free_blocks;
>  	ino_t	free_inodes;
>  	int	mount_flags;
> +	int	openfs_flags;
>  	blkid_cache blkid;	/* blkid cache */
>  
>  #ifdef HAVE_SETJMP_H
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index 905c0bf..9be52cd 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -903,7 +903,7 @@ errcode_t e2fsck_run_ext3_journal(e2fsck_t ctx)
>  
>  	ext2fs_mmp_stop(ctx->fs);
>  	ext2fs_free(ctx->fs);
> -	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
> +	retval = ext2fs_open(ctx->filesystem_name, ctx->openfs_flags,
>  			     ctx->superblock, blocksize, io_ptr,
>  			     &ctx->fs);
>  	if (retval) {
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index b265c99..03848c7 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1274,6 +1274,7 @@ restart:
>  			flags &= ~EXT2_FLAG_EXCLUSIVE;
>  	}
>  
> +	ctx->openfs_flags = flags;
>  	retval = try_open_fs(ctx, flags, io_ptr, &fs);
>  
>  	if (!ctx->superblock && !(ctx->options & E2F_OPT_PREEN) &&
> -- 
> 2.0.0
> 
> --
> 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
--
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