Skip to contentSkip to conversation

f2fs: remove false-positive bug_on

PATCH In review Jaegeuk Kim 3 participants 5 messages May 26, 2017 – Jun 1, 2017

Subsystems (2)
VFS F2FS file system
lore
Share & tools
lore

Jaegeuk Kim

author maintainer
#0 lore raw

PATCH 1/1 f2fs: remove false-positive bug_on

If we got failure from both of create and evict_inode, we can hit this wrong bug_on.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inode.c | 2 --
 1 file changed, 2 deletions(-)
fs/f2fs/inode.c +0 -2
index e53c784ab11e..5673b0bd83b5 100644
@@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
maintainer
#1 lore raw

Hi Jaegeuk,

Quoted 13 lines · Jaegeuk Kim

If we got failure from both of create and evict_inode, we can hit this wrong bug_on.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inode.c | 2 --
 1 file changed, 2 deletions(-)
fs/f2fs/inode.c +0 -0
index e53c784ab11e..5673b0bd83b5 100644

fs/f2fs/inode.c void f2fs_evict_inode(struct inode *inode) · line 430

Quoted 5 lines
fs/f2fs/inode.c +0 -2
@@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));

We expect that we can keep the inode in orphan list in handle_failed_inode path when inode page have been persisted, so that if there is anything error in evice_inode, we can have another chance to release inode resource during next mount.

Here we need to check this case, additionally, if we failed to add the inode into orphan list in handle_failed_inode, we must have set SBI_NEED_FSCK in cp pack, so we need check the case too.

So we can change the code to:

f2fs_bug_on(err && err != -ENOENT &&

		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

How do you think?

Thanks,

Quoted 4 lines
fs/f2fs/inode.c +0 -0
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
author maintainer
#2 lore raw
Quoted 31 lines · Chao Yu

Hi Jaegeuk,

Quoted 13 lines · Jaegeuk Kim

If we got failure from both of create and evict_inode, we can hit this wrong bug_on.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inode.c | 2 --
 1 file changed, 2 deletions(-)
fs/f2fs/inode.c +0 -0
index e53c784ab11e..5673b0bd83b5 100644

fs/f2fs/inode.c void f2fs_evict_inode(struct inode *inode) · line 430

Quoted 5 lines
fs/f2fs/inode.c +0 -2
@@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));

We expect that we can keep the inode in orphan list in handle_failed_inode path when inode page have been persisted, so that if there is anything error in evice_inode, we can have another chance to release inode resource during next mount.

Here we need to check this case, additionally, if we failed to add the inode into orphan list in handle_failed_inode, we must have set SBI_NEED_FSCK in cp pack, so we need check the case too.

Quoted 5 lines

So we can change the code to:

f2fs_bug_on(err && err != -ENOENT &&

		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

Yup, I'll try this. ;)

Thanks,

Quoted 9 lines

How do you think?

Thanks,

Quoted 4 lines
fs/f2fs/inode.c +0 -0
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
#3 lore raw
Quoted 32 lines · Jaegeuk Kim
Quoted 31 lines · Chao Yu

Hi Jaegeuk,

Quoted 13 lines · Jaegeuk Kim

If we got failure from both of create and evict_inode, we can hit this wrong bug_on.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inode.c | 2 --
 1 file changed, 2 deletions(-)
fs/f2fs/inode.c +0 -0
index e53c784ab11e..5673b0bd83b5 100644

fs/f2fs/inode.c void f2fs_evict_inode(struct inode *inode) · line 430

Quoted 5 lines
fs/f2fs/inode.c +0 -2
@@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));

We expect that we can keep the inode in orphan list in handle_failed_inode path when inode page have been persisted, so that if there is anything error in evice_inode, we can have another chance to release inode resource during next mount.

Here we need to check this case, additionally, if we failed to add the inode into orphan list in handle_failed_inode, we must have set SBI_NEED_FSCK in cp pack, so we need check the case too.

Quoted 5 lines
Quoted 5 lines

So we can change the code to:

f2fs_bug_on(err && err != -ENOENT &&

		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

Oh, looks like it needs to use '&&' in between exist_written_data and is_sbi_flag_set. Could you fix this?

Thanks,

Quoted 17 lines

Yup, I'll try this. ;)

Thanks,

Quoted 9 lines

How do you think?

Thanks,

Quoted 4 lines
fs/f2fs/inode.c +0 -0
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);

.

author maintainer
#4 lore raw

For example,

f2fs_create
- new_node_page is failed
- handle_failed_inode
- skip to add it into orphan list, since ni.blk_addr == NULL_ADDR : set_inode_flag(inode, FI_FREE_NID)

f2fs_evict_inode
- EIO due to fault injection
- f2fs_bug_on() is triggered

So, we don't need to call f2fs_bug_on in this case.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/inode.c +3 -2
index e53c784ab11e..868d71436ebc 100644
@@ -425,9 +425,10 @@ void f2fs_evict_inode(struct inode *inode)
 	if (is_inode_flag_set(inode, FI_FREE_NID)) {
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
+	} else {
+		f2fs_bug_on(sbi, err &&
+			!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog