f2fs: recovering broken superblock during mount

RFC PATCH In review hujianyang 2 participants 5 messages May 20, 2015 – May 22, 2015 F2FS file system

lore

Share & tools

lore

hujianyang

author

2015-05-20 07:01 UTC #0 lore raw

RFC PATCH 1/1 f2fs: recovering broken superblock during mount

Hi Jaegeuk,

There are two superblocks in f2fs. I wrapped one of them by 'dd' in a reliability test. After this, the polluted f2fs partition can be mounted with an error message:

F2FS-fs (sdd3): Magic Mismatch, valid(0xf2f52010) - read(0x0) F2FS-fs (sdd3): Can't find valid F2FS filesystem in 1th superblock

Seems the broken superblock can't be recovered by f2fs driver or fsck.f2fs. Tell me how to recover if I was wrong.:)

So I'd like to provide a patch to recover broken f2fs superblock during mount. But I'm not pleased with my patch. I wish you and others could give me some suggestions.

Do you think it's necessary to recover the superblock? and can we change 'f2fs_commit_super' a little to reuse this function during superblock recovery?

Thanks,
Hu

fs/f2fs/super.c +57 ~~-26~~

index ae14fc4..126f8fa 100644
@@ -965,6 +965,33 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
 	clear_sbi_flag(sbi, SBI_NEED_FSCK);
 }
 
+static int get_valid_super_block(struct super_block *sb,
+			struct buffer_head **buffer, int block)
+{
+	struct f2fs_super_block *super;
+
+	*buffer = sb_bread(sb, block);
+	if (!*buffer) {
+		f2fs_msg(sb, KERN_ERR, "Unable to read %dth superblock",
+				block + 1);
+		return -EIO;
+	}
+
+	super = (struct f2fs_super_block *)
+		((char *)(*buffer)->b_data + F2FS_SUPER_OFFSET);
+
+	/* sanity checking of raw super */
+	if (sanity_check_raw_super(sb, super)) {
+		brelse(*buffer);
+		f2fs_msg(sb, KERN_ERR,
+			"Can't find valid F2FS filesystem in %dth superblock",
+								block + 1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * Read f2fs raw super block.
  * Because we have two copies of super block, so read the first one at first,

@@ -975,37 +1002,41 @@ static int read_raw_super_block(struct super_block *sb,
 			struct buffer_head **raw_super_buf)
 {
 	int block = 0;
-
-retry:
-	*raw_super_buf = sb_bread(sb, block);
-	if (!*raw_super_buf) {
-		f2fs_msg(sb, KERN_ERR, "Unable to read %dth superblock",
-				block + 1);
-		if (block == 0) {
-			block++;
-			goto retry;
-		} else {
-			return -EIO;
-		}
+	struct buffer_head *buffer0 = NULL, *buffer1 = NULL;
+	bool readonly = f2fs_readonly(sb) || bdev_read_only(sb->s_bdev);
+	int err0, err1;
+
+	err0 = get_valid_super_block(sb, &buffer0, 0);
+	if (!err0 && readonly)
+		goto set_raw_super;
+
+	err1 = get_valid_super_block(sb, &buffer1, 1);
+	if (err0 && err1)
+		return err1;
+
+	if (err0 && !err1) {
+		buffer0 = buffer1;
+		if (readonly)
+			goto set_raw_super;
+	} else if (!err0 && !err1) {
+		brelse(buffer1);
+		goto set_raw_super;
 	}
 
+	f2fs_msg(sb, KERN_INFO, "Recover superblock");
+	block = buffer0->b_blocknr;
+	buffer0->b_blocknr = block ? 0 : 1;
+	mark_buffer_dirty(buffer0);
+	sync_dirty_buffer(buffer0);
+	buffer0->b_blocknr = block;
+	clear_buffer_write_io_error(buffer0);
+	set_buffer_uptodate(buffer0);
+
+set_raw_super:
+	*raw_super_buf = buffer0;
 	*raw_super = (struct f2fs_super_block *)
 		((char *)(*raw_super_buf)->b_data + F2FS_SUPER_OFFSET);
 
-	/* sanity checking of raw super */
-	if (sanity_check_raw_super(sb, *raw_super)) {
-		brelse(*raw_super_buf);
-		f2fs_msg(sb, KERN_ERR,
-			"Can't find valid F2FS filesystem in %dth superblock",
-								block + 1);
-		if (block == 0) {
-			block++;
-			goto retry;
-		} else {
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Jaegeuk Kim

maintainer

2015-05-21 01:08 UTC #1 lore raw

Hi Hu,

Quoted 12 lines · hujianyang

Hi Jaegeuk,
There are two superblocks in f2fs. I wrapped one of them by 'dd' in a reliability test. After this, the polluted f2fs partition can be mounted with an error message:
F2FS-fs (sdd3): Magic Mismatch, valid(0xf2f52010) - read(0x0) F2FS-fs (sdd3): Can't find valid F2FS filesystem in 1th superblock
Seems the broken superblock can't be recovered by f2fs driver or fsck.f2fs. Tell me how to recover if I was wrong.:)

Quoted 7 lines

So I'd like to provide a patch to recover broken f2fs superblock during mount. But I'm not pleased with my patch. I wish you and others could give me some suggestions.
Do you think it's necessary to recover the superblock? and can we change 'f2fs_commit_super' a little to reuse this function during superblock recovery?

What is the real problem in the original code?
It's the message problem, isn't it?
Even though the first superblock is missing with that message, mount must be done with the second superblock, right?

If so, it would be good to use f2fs_commit_super to recover that and leave some messages.

Thanks,

Quoted 100 lines

Thanks,
Hu

fs/f2fs/super.c +57 ~~-18~~

index ae14fc4..126f8fa 100644
@@ -965,6 +965,33 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
 	clear_sbi_flag(sbi, SBI_NEED_FSCK);
 }
 
+static int get_valid_super_block(struct super_block *sb,
+			struct buffer_head **buffer, int block)
+{
+	struct f2fs_super_block *super;
+
+	*buffer = sb_bread(sb, block);
+	if (!*buffer) {
+		f2fs_msg(sb, KERN_ERR, "Unable to read %dth superblock",
+				block + 1);
+		return -EIO;
+	}
+
+	super = (struct f2fs_super_block *)
+		((char *)(*buffer)->b_data + F2FS_SUPER_OFFSET);
+
+	/* sanity checking of raw super */
+	if (sanity_check_raw_super(sb, super)) {
+		brelse(*buffer);
+		f2fs_msg(sb, KERN_ERR,
+			"Can't find valid F2FS filesystem in %dth superblock",
+								block + 1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * Read f2fs raw super block.
  * Because we have two copies of super block, so read the first one at first,

@@ -975,37 +1002,41 @@ static int read_raw_super_block(struct super_block *sb,
 			struct buffer_head **raw_super_buf)
 {
 	int block = 0;
-
-retry:
-	*raw_super_buf = sb_bread(sb, block);
-	if (!*raw_super_buf) {
-		f2fs_msg(sb, KERN_ERR, "Unable to read %dth superblock",
-				block + 1);
-		if (block == 0) {
-			block++;
-			goto retry;
-		} else {
-			return -EIO;
-		}
+	struct buffer_head *buffer0 = NULL, *buffer1 = NULL;
+	bool readonly = f2fs_readonly(sb) || bdev_read_only(sb->s_bdev);
+	int err0, err1;
+
+	err0 = get_valid_super_block(sb, &buffer0, 0);
+	if (!err0 && readonly)
+		goto set_raw_super;
+
+	err1 = get_valid_super_block(sb, &buffer1, 1);
+	if (err0 && err1)
+		return err1;
+
+	if (err0 && !err1) {
+		buffer0 = buffer1;
+		if (readonly)
+			goto set_raw_super;
+	} else if (!err0 && !err1) {
+		brelse(buffer1);
+		goto set_raw_super;
 	}
 
+	f2fs_msg(sb, KERN_INFO, "Recover superblock");
+	block = buffer0->b_blocknr;
+	buffer0->b_blocknr = block ? 0 : 1;
+	mark_buffer_dirty(buffer0);
+	sync_dirty_buffer(buffer0);
+	buffer0->b_blocknr = block;
+	clear_buffer_write_io_error(buffer0);
+	set_buffer_uptodate(buffer0);
+
+set_raw_super:
+	*raw_super_buf = buffer0;
 	*raw_super = (struct f2fs_super_block *)
 		((char *)(*raw_super_buf)->b_data + F2FS_SUPER_OFFSET);
 
-	/* sanity checking of raw super */
-	if (sanity_check_raw_super(sb, *raw_super)) {
-		brelse(*raw_super_buf);
-		f2fs_msg(sb, KERN_ERR,
-			"Can't find valid F2FS filesystem in %dth superblock",
-								block + 1);

fs/f2fs/super.c static int read_raw_super_block(struct super_block *sb, · line 1041

Quoted 10 lines

fs/f2fs/super.c +0 -8

-		if (block == 0) {
-			block++;
-			goto retry;
-		} else {
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }

hujianyang

author

2015-05-21 06:43 UTC #2 lore raw

Quoted 28 lines · Jaegeuk Kim

Hi Hu,
Quoted 12 lines · hujianyang
Hi Jaegeuk,
There are two superblocks in f2fs. I wrapped one of them by 'dd' in a reliability test. After this, the polluted f2fs partition can be mounted with an error message:
F2FS-fs (sdd3): Magic Mismatch, valid(0xf2f52010) - read(0x0) F2FS-fs (sdd3): Can't find valid F2FS filesystem in 1th superblock
Seems the broken superblock can't be recovered by f2fs driver or fsck.f2fs. Tell me how to recover if I was wrong.:)
Quoted 7 lines
So I'd like to provide a patch to recover broken f2fs superblock during mount. But I'm not pleased with my patch. I wish you and others could give me some suggestions.
Do you think it's necessary to recover the superblock? and can we change 'f2fs_commit_super' a little to reuse this function during superblock recovery?
What is the real problem in the original code?
It's the message problem, isn't it?
Even though the first superblock is missing with that message, mount must be done with the second superblock, right?

Quoted 5 lines

If so, it would be good to use f2fs_commit_super to recover that and leave some messages.
Thanks,

Hi Jaegeuk,

Thanks for your suggestions. I've reworked the patch, how do you feel about the following one?

Current f2fs doesn't check the 2th superblock if the first one is valid, my patch changes to check both of superblocks during mount because I think either of them may corrupted and we should detect the corruption at once, then recover it.

Thanks,
Hu

Signed-off-by hujianyang <[email protected]>

---
 fs/f2fs/super.c |   56 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 45 insertions(+), 11 deletions(-)

fs/f2fs/super.c +45 ~~-11~~

index ae14fc4..8e5cd83 100644
@@ -972,29 +972,36 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
  */
 static int read_raw_super_block(struct super_block *sb,
 			struct f2fs_super_block **raw_super,
-			struct buffer_head **raw_super_buf)
+			struct buffer_head **raw_super_buf,
+			int *recovery)
 {
 	int block = 0;
+	struct buffer_head *buffer;
+	struct f2fs_super_block *super;
+	int err = 0;
 
 retry:
-	*raw_super_buf = sb_bread(sb, block);
-	if (!*raw_super_buf) {
+	buffer = sb_bread(sb, block);
+	if (!buffer) {
+		*recovery = 1;
 		f2fs_msg(sb, KERN_ERR, "Unable to read %dth superblock",
 				block + 1);
 		if (block == 0) {
 			block++;
 			goto retry;
 		} else {
-			return -EIO;
+			err = -EIO;
+			goto out;
 		}
 	}
 
-	*raw_super = (struct f2fs_super_block *)
-		((char *)(*raw_super_buf)->b_data + F2FS_SUPER_OFFSET);
+	super = (struct f2fs_super_block *)
+		((char *)(buffer)->b_data + F2FS_SUPER_OFFSET);
 
 	/* sanity checking of raw super */
-	if (sanity_check_raw_super(sb, *raw_super)) {
-		brelse(*raw_super_buf);
+	if (sanity_check_raw_super(sb, super)) {
+		brelse(buffer);
+		*recovery = 1;
 		f2fs_msg(sb, KERN_ERR,
 			"Can't find valid F2FS filesystem in %dth superblock",
 								block + 1);

@@ -1002,10 +1009,30 @@ retry:
 			block++;
 			goto retry;
 		} else {
-			return -EINVAL;
+			err = -EINVAL;
+			goto out;
 		}
 	}
 
+	if (!*raw_super) {
+		*raw_super_buf = buffer;
+		*raw_super = super;
+	} else {
+		/* already have a valid superblock */
+		brelse(buffer);
+	}
+
+	/* check the validity of the second superblock */
+	if (block == 0) {
+		block++;
+		goto retry;
+	}
+
+out:
+	/* No valid superblock */
+	if (!*raw_super)
+		return err;
+
 	return 0;
 }

@@ -1042,7 +1069,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	long err = -EINVAL;
 	bool retry = true, need_fsck = false;
 	char *options = NULL;
-	int i;
+	int recovery = 0, i;
 
 try_onemore:
 	/* allocate memory for f2fs-specific super block info */

@@ -1056,7 +1083,7 @@ try_onemore:
 		goto free_sbi;
 	}
 
-	err = read_raw_super_block(sb, &raw_super, &raw_super_buf);
+	err = read_raw_super_block(sb, &raw_super, &raw_super_buf, &recovery);
 	if (err)
 		goto free_sbi;

@@ -1255,6 +1282,13 @@ try_onemore:
 			goto free_kobj;
 	}
 	kfree(options);
+
+	/* recover broken superblock */
+	if (recovery && !f2fs_readonly(sb) && !bdev_read_only(sb->s_bdev)) {
+		f2fs_msg(sb, KERN_INFO, "Superblock recovery");
+		f2fs_commit_super(sbi);
+	}
+
 	return 0;
 
 free_kobj:

Signature · 9 lines

1.6.0.2


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Jaegeuk Kim

maintainer

2015-05-21 22:44 UTC #3 lore raw

Hi Hu,

Looks good to me, except one minor thing where

+ int recovery = 0, i;
-> AFAIK, kernel coding style recommends to split them. Let me change that to

 	int recovery = 0;
	int i;

Merged.

Thanks,

Quoted 168 lines · hujianyang

Quoted 28 lines · Jaegeuk Kim
Hi Hu,
Quoted 12 lines · hujianyang
Hi Jaegeuk,
There are two superblocks in f2fs. I wrapped one of them by 'dd' in a reliability test. After this, the polluted f2fs partition can be mounted with an error message:
F2FS-fs (sdd3): Magic Mismatch, valid(0xf2f52010) - read(0x0) F2FS-fs (sdd3): Can't find valid F2FS filesystem in 1th superblock
Seems the broken superblock can't be recovered by f2fs driver or fsck.f2fs. Tell me how to recover if I was wrong.:)
Quoted 7 lines
So I'd like to provide a patch to recover broken f2fs superblock during mount. But I'm not pleased with my patch. I wish you and others could give me some suggestions.
Do you think it's necessary to recover the superblock? and can we change 'f2fs_commit_super' a little to reuse this function during superblock recovery?
What is the real problem in the original code?
It's the message problem, isn't it?
Even though the first superblock is missing with that message, mount must be done with the second superblock, right?
Quoted 5 lines
If so, it would be good to use f2fs_commit_super to recover that and leave some messages.
Thanks,
Hi Jaegeuk,
Thanks for your suggestions. I've reworked the patch, how do you feel about the following one?
Current f2fs doesn't check the 2th superblock if the first one is valid, my patch changes to check both of superblocks during mount because I think either of them may corrupted and we should detect the corruption at once, then recover it.
Thanks,
Hu
Signed-off-by hujianyang <[email protected]>
---
 fs/f2fs/super.c |   56 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 45 insertions(+), 11 deletions(-)
fs/f2fs/super.c +45 ~~-11~~
index ae14fc4..8e5cd83 100644
@@ -972,29 +972,36 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
  */
 static int read_raw_super_block(struct super_block *sb,
 			struct f2fs_super_block **raw_super,
-			struct buffer_head **raw_super_buf)
+			struct buffer_head **raw_super_buf,
+			int *recovery)
 {
 	int block = 0;
+	struct buffer_head *buffer;
+	struct f2fs_super_block *super;
+	int err = 0;
 
 retry:
-	*raw_super_buf = sb_bread(sb, block);
-	if (!*raw_super_buf) {
+	buffer = sb_bread(sb, block);
+	if (!buffer) {
+		*recovery = 1;
 		f2fs_msg(sb, KERN_ERR, "Unable to read %dth superblock",
 				block + 1);
 		if (block == 0) {
 			block++;
 			goto retry;
 		} else {
-			return -EIO;
+			err = -EIO;
+			goto out;
 		}
 	}
 
-	*raw_super = (struct f2fs_super_block *)
-		((char *)(*raw_super_buf)->b_data + F2FS_SUPER_OFFSET);
+	super = (struct f2fs_super_block *)
+		((char *)(buffer)->b_data + F2FS_SUPER_OFFSET);
 
 	/* sanity checking of raw super */
-	if (sanity_check_raw_super(sb, *raw_super)) {
-		brelse(*raw_super_buf);
+	if (sanity_check_raw_super(sb, super)) {
+		brelse(buffer);
+		*recovery = 1;
 		f2fs_msg(sb, KERN_ERR,
 			"Can't find valid F2FS filesystem in %dth superblock",
 								block + 1);
@@ -1002,10 +1009,30 @@ retry:
 			block++;
 			goto retry;
 		} else {
-			return -EINVAL;
+			err = -EINVAL;
+			goto out;
 		}
 	}
 
+	if (!*raw_super) {
+		*raw_super_buf = buffer;
+		*raw_super = super;
+	} else {
+		/* already have a valid superblock */
+		brelse(buffer);
+	}
+
+	/* check the validity of the second superblock */
+	if (block == 0) {
+		block++;
+		goto retry;
+	}
+
+out:
+	/* No valid superblock */
+	if (!*raw_super)
+		return err;
+
 	return 0;
 }
 
@@ -1042,7 +1069,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	long err = -EINVAL;
 	bool retry = true, need_fsck = false;
 	char *options = NULL;
-	int i;
+	int recovery = 0, i;
 
 try_onemore:
 	/* allocate memory for f2fs-specific super block info */
@@ -1056,7 +1083,7 @@ try_onemore:
 		goto free_sbi;
 	}
 
-	err = read_raw_super_block(sb, &raw_super, &raw_super_buf);
+	err = read_raw_super_block(sb, &raw_super, &raw_super_buf, &recovery);
 	if (err)
 		goto free_sbi;
 
@@ -1255,6 +1282,13 @@ try_onemore:
 			goto free_kobj;
 	}
 	kfree(options);
+
+	/* recover broken superblock */
+	if (recovery && !f2fs_readonly(sb) && !bdev_read_only(sb->s_bdev)) {
+		f2fs_msg(sb, KERN_INFO, "Superblock recovery");
+		f2fs_commit_super(sbi);
+	}
+
 	return 0;
 
 free_kobj:
Signature · 1 line
1.6.0.2

hujianyang

author

2015-05-22 00:20 UTC #4 lore raw

Quoted 12 lines · Jaegeuk Kim

Hi Hu,
Looks good to me, except one minor thing where
(quoted diff) +1 -0
+	int recovery = 0, i;
 -> AFAIK, kernel coding style recommends to split them.
 Let me change that to
 
 	int recovery = 0;
	int i;

Quoted 3 lines

Merged.
Thanks,

Hi Jaegeuk,

Thank you!:)