* [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs
@ 2015-08-05 22:13 Eric Sandeen
2015-08-06 18:10 ` Eric Sandeen
2015-08-11 19:22 ` Jan Kara
0 siblings, 2 replies; 4+ messages in thread
From: Eric Sandeen @ 2015-08-05 22:13 UTC (permalink / raw
To: ext4 development; +Cc: Stu Mark
At some point along this sequence of changes:
f6e63f9 ext4: fold ext4_nojournal_sops into ext4_sops
bb04457 ext4: support freezing ext2 (nojournal) file systems
9ca9238 ext4: Use separate super_operations structure for no_journal filesystems
ext4 started setting needs_recovery on filesystems without journals
when they are unfrozen. This makes no sense, and in fact confuses
blkid to the point where it doesn't recognize the filesystem at all.
(freeze ext2; unfreeze ext2; run blkid; see no output; run dumpe2fs,
see needs_recovery set on fs w/ no journal).
To fix this, don't manipulate the INCOMPAT_RECOVER feature on
filesystems without journals.
Reported-by: Stu Mark <smark@datto.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
Note, is there a reason that in ext4_freeze, if journal_flush
fails, we skip the ext4_commit_super call? I didn't change that
here, but it seems odd.
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 58987b5..e7b345d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4833,10 +4833,11 @@ static int ext4_freeze(struct super_block *sb)
error = jbd2_journal_flush(journal);
if (error < 0)
goto out;
+
+ /* Journal blocked and flushed, clear needs_recovery flag. */
+ EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
}
- /* Journal blocked and flushed, clear needs_recovery flag. */
- EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
error = ext4_commit_super(sb, 1);
out:
if (journal)
@@ -4854,8 +4855,11 @@ static int ext4_unfreeze(struct super_block *sb)
if (sb->s_flags & MS_RDONLY)
return 0;
- /* Reset the needs_recovery flag before the fs is unlocked. */
- EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
+ if (EXT4_SB(sb)->s_journal) {
+ /* Reset the needs_recovery flag before the fs is unlocked. */
+ EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
+ }
+
ext4_commit_super(sb, 1);
return 0;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs
2015-08-05 22:13 [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs Eric Sandeen
@ 2015-08-06 18:10 ` Eric Sandeen
2015-08-11 19:22 ` Jan Kara
1 sibling, 0 replies; 4+ messages in thread
From: Eric Sandeen @ 2015-08-06 18:10 UTC (permalink / raw
To: ext4 development; +Cc: Stu Mark
On 8/5/15 3:13 PM, Eric Sandeen wrote:
> At some point along this sequence of changes:
>
> f6e63f9 ext4: fold ext4_nojournal_sops into ext4_sops
> bb04457 ext4: support freezing ext2 (nojournal) file systems
> 9ca9238 ext4: Use separate super_operations structure for no_journal filesystems
>
> ext4 started setting needs_recovery on filesystems without journals
> when they are unfrozen. This makes no sense, and in fact confuses
> blkid to the point where it doesn't recognize the filesystem at all.
>
> (freeze ext2; unfreeze ext2; run blkid; see no output; run dumpe2fs,
> see needs_recovery set on fs w/ no journal).
>
> To fix this, don't manipulate the INCOMPAT_RECOVER feature on
> filesystems without journals.
Ted, if this passes review you might cc: stable on the commit as well.
Thanks,
-Eric
> Reported-by: Stu Mark <smark@datto.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Note, is there a reason that in ext4_freeze, if journal_flush
> fails, we skip the ext4_commit_super call? I didn't change that
> here, but it seems odd.
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 58987b5..e7b345d 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4833,10 +4833,11 @@ static int ext4_freeze(struct super_block *sb)
> error = jbd2_journal_flush(journal);
> if (error < 0)
> goto out;
> +
> + /* Journal blocked and flushed, clear needs_recovery flag. */
> + EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> }
>
> - /* Journal blocked and flushed, clear needs_recovery flag. */
> - EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> error = ext4_commit_super(sb, 1);
> out:
> if (journal)
> @@ -4854,8 +4855,11 @@ static int ext4_unfreeze(struct super_block *sb)
> if (sb->s_flags & MS_RDONLY)
> return 0;
>
> - /* Reset the needs_recovery flag before the fs is unlocked. */
> - EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> + if (EXT4_SB(sb)->s_journal) {
> + /* Reset the needs_recovery flag before the fs is unlocked. */
> + EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> + }
> +
> ext4_commit_super(sb, 1);
> return 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs
2015-08-05 22:13 [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs Eric Sandeen
2015-08-06 18:10 ` Eric Sandeen
@ 2015-08-11 19:22 ` Jan Kara
2015-08-15 14:50 ` Theodore Ts'o
1 sibling, 1 reply; 4+ messages in thread
From: Jan Kara @ 2015-08-11 19:22 UTC (permalink / raw
To: Eric Sandeen; +Cc: ext4 development, Stu Mark
On Wed 05-08-15 15:13:58, Eric Sandeen wrote:
> At some point along this sequence of changes:
>
> f6e63f9 ext4: fold ext4_nojournal_sops into ext4_sops
> bb04457 ext4: support freezing ext2 (nojournal) file systems
> 9ca9238 ext4: Use separate super_operations structure for no_journal filesystems
>
> ext4 started setting needs_recovery on filesystems without journals
> when they are unfrozen. This makes no sense, and in fact confuses
> blkid to the point where it doesn't recognize the filesystem at all.
>
> (freeze ext2; unfreeze ext2; run blkid; see no output; run dumpe2fs,
> see needs_recovery set on fs w/ no journal).
>
> To fix this, don't manipulate the INCOMPAT_RECOVER feature on
> filesystems without journals.
>
> Reported-by: Stu Mark <smark@datto.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.com>
Honza
> ---
>
> Note, is there a reason that in ext4_freeze, if journal_flush
> fails, we skip the ext4_commit_super call? I didn't change that
> here, but it seems odd.
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 58987b5..e7b345d 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4833,10 +4833,11 @@ static int ext4_freeze(struct super_block *sb)
> error = jbd2_journal_flush(journal);
> if (error < 0)
> goto out;
> +
> + /* Journal blocked and flushed, clear needs_recovery flag. */
> + EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> }
>
> - /* Journal blocked and flushed, clear needs_recovery flag. */
> - EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> error = ext4_commit_super(sb, 1);
> out:
> if (journal)
> @@ -4854,8 +4855,11 @@ static int ext4_unfreeze(struct super_block *sb)
> if (sb->s_flags & MS_RDONLY)
> return 0;
>
> - /* Reset the needs_recovery flag before the fs is unlocked. */
> - EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> + if (EXT4_SB(sb)->s_journal) {
> + /* Reset the needs_recovery flag before the fs is unlocked. */
> + EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
> + }
> +
> ext4_commit_super(sb, 1);
> return 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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs
2015-08-11 19:22 ` Jan Kara
@ 2015-08-15 14:50 ` Theodore Ts'o
0 siblings, 0 replies; 4+ messages in thread
From: Theodore Ts'o @ 2015-08-15 14:50 UTC (permalink / raw
To: Jan Kara; +Cc: Eric Sandeen, ext4 development, Stu Mark
On Tue, Aug 11, 2015 at 09:22:37PM +0200, Jan Kara wrote:
> On Wed 05-08-15 15:13:58, Eric Sandeen wrote:
> > ext4 started setting needs_recovery on filesystems without journals
> > when they are unfrozen. This makes no sense, and in fact confuses
> > blkid to the point where it doesn't recognize the filesystem at all.
> >
> > (freeze ext2; unfreeze ext2; run blkid; see no output; run dumpe2fs,
> > see needs_recovery set on fs w/ no journal).
> >
> > To fix this, don't manipulate the INCOMPAT_RECOVER feature on
> > filesystems without journals.
> >
> > Reported-by: Stu Mark <smark@datto.com>
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>
> The patch looks good. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.com>
Thanks, applied.
> > Note, is there a reason that in ext4_freeze, if journal_flush
> > fails, we skip the ext4_commit_super call? I didn't change that
> > here, but it seems odd.
The only reason to call ext4_commit_super() is to persistent the
changed INCOMPAT_RECOVER flag --- well, and to update the global free
inode/block counts. OTOH, if the journal_flush() has failed, the fact
that the global free counts aren't up to date will be the least of
your problems as far as the consistency of the file system snapshot is
concerned....
- Ted
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-15 14:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-05 22:13 [PATCH] ext4: don't manipulate recovery flag when freezing no-journal fs Eric Sandeen
2015-08-06 18:10 ` Eric Sandeen
2015-08-11 19:22 ` Jan Kara
2015-08-15 14:50 ` Theodore Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.