Skip to contentSkip to conversation

f2fs: fix to set superblock dirty correctly

PATCH In review Chao Yu 3 participants 3 messages Aug 28, 2016 – Aug 30, 2016 F2FS file system

lore
Share & tools
lore

Chao Yu

author maintainer
#0 lore raw

PATCH v2 1/1 f2fs: fix to set superblock dirty correctly

From: Chao Yu <[email protected]>

tests/generic/251 of fstest suit complains us with below message:

------------[ cut here ]------------
invalid opcode: 0000 [#1] PREEMPT SMP
CPU: 2 PID: 7698 Comm: fstrim Tainted: G           O    4.7.0+ #21

task: e9f4e000 task.stack: e7262000
EIP: 0060:[<f89fcefe>] EFLAGS: 00010202 CPU: 2 EIP is at write_checkpoint+0xfde/0x1020 [f2fs]
EAX: f33eb300 EBX: eecac310 ECX: 00000001 EDX: ffff0001
ESI: eecac000 EDI: eecac5f0 EBP: e7263dec ESP: e7263d18
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 80050033 CR2: b76ab01c CR3: 2eb89de0 CR4: 000406f0

Stack:
 00000001 a220fb7b e9f4e000 00000002 419ff2d3 b3a05151 00000002 e9f4e5d8
 e9f4e000 419ff2d3 b3a05151 eecac310 c10b8154 b3a05151 419ff2d3 c10b78bd
 e9f4e000 e9f4e000 e9f4e5d8 00000001 e9f4e000 ec409000 eecac2cc eecac288
Call Trace:
 [<c10b8154>] ? __lock_acquire+0x3c4/0x760
 [<c10b78bd>] ? mark_held_locks+0x5d/0x80
 [<f8a10632>] f2fs_trim_fs+0x1c2/0x2e0 [f2fs]
 [<f89e9f56>] f2fs_ioctl+0x6b6/0x10b0 [f2fs]
 [<c13d51df>] ? __this_cpu_preempt_check+0xf/0x20
 [<c10b4281>] ? trace_hardirqs_off_caller+0x91/0x120
 [<f89e98a0>] ? __exchange_data_block+0xd30/0xd30 [f2fs]
 [<c120b2e1>] do_vfs_ioctl+0x81/0x7f0
 [<c11d57c5>] ? kmem_cache_free+0x245/0x2e0
 [<c1217840>] ? get_unused_fd_flags+0x40/0x40
 [<c1206eec>] ? putname+0x4c/0x50
 [<c11f631e>] ? do_sys_open+0x16e/0x1d0
 [<c1001990>] ? do_fast_syscall_32+0x30/0x1c0
 [<c13d51df>] ? __this_cpu_preempt_check+0xf/0x20
 [<c120baa8>] SyS_ioctl+0x58/0x80
 [<c1001a01>] do_fast_syscall_32+0xa1/0x1c0
 [<c178cc54>] sysenter_past_esp+0x45/0x74
EIP: [<f89fcefe>] write_checkpoint+0xfde/0x1020 [f2fs] SS:ESP 0068:e7263d18
---[ end trace 4de95d7e6b3aa7c6 ]---

The reason is: with below call stack, we will encounter BUG_ON during doing fstrim.

Thread A Thread B
- write_checkpoint
- do_checkpoint
- f2fs_write_inode
- update_inode_page
- update_inode
- set_page_dirty
- f2fs_set_node_page_dirty
- inc_page_count
- percpu_counter_inc
- set_sbi_flag(SBI_IS_DIRTY)
- clear_sbi_flag(SBI_IS_DIRTY)

Thread C Thread D
- f2fs_write_node_page
- set_node_addr
- __set_nat_cache_dirty
- nm_i->dirty_nat_cnt++
- do_vfs_ioctl
- f2fs_ioctl
- f2fs_trim_fs
- write_checkpoint
- f2fs_bug_on(nm_i->dirty_nat_cnt)

Fix it by setting superblock dirty correctly in do_checkpoint and f2fs_write_node_page.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 8 ++++++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 9 insertions(+)
fs/f2fs/checkpoint.c +8 -0
index cd0443d..1864078 100644
@@ -1153,6 +1153,14 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	clear_prefree_segments(sbi, cpc);
 	clear_sbi_flag(sbi, SBI_IS_DIRTY);
 
+	/*
+	 * redirty superblock if metadata like node page or inode cache is
+	 * updated during writting checkpoint.
+	 */
+	if (get_pages(sbi, F2FS_DIRTY_NODES) ||
+			get_pages(sbi, F2FS_DIRTY_IMETA))
+		set_sbi_flag(sbi, SBI_IS_DIRTY);
+
 	return 0;
 }
fs/f2fs/node.c +1 -0
index 8a28800..365c6ff 100644
@@ -1597,6 +1597,7 @@ static int f2fs_write_node_page(struct page *page,
 	fio.old_blkaddr = ni.blk_addr;
 	write_node_page(nid, &fio);
 	set_node_addr(sbi, &ni, fio.new_blkaddr, is_fsync_dnode(page));
+	set_sbi_flag(sbi, SBI_IS_DIRTY);
 	dec_page_count(sbi, F2FS_DIRTY_NODES);
 	up_read(&sbi->node_write);
Signature · 1 line
2.7.2
maintainer
#1 lore raw

Hi Chao,

Quoted 80 lines · Chao Yu

From: Chao Yu <[email protected]>

tests/generic/251 of fstest suit complains us with below message:

------------[ cut here ]------------
invalid opcode: 0000 [#1] PREEMPT SMP
CPU: 2 PID: 7698 Comm: fstrim Tainted: G           O    4.7.0+ #21

task: e9f4e000 task.stack: e7262000
EIP: 0060:[<f89fcefe>] EFLAGS: 00010202 CPU: 2 EIP is at write_checkpoint+0xfde/0x1020 [f2fs]
EAX: f33eb300 EBX: eecac310 ECX: 00000001 EDX: ffff0001
ESI: eecac000 EDI: eecac5f0 EBP: e7263dec ESP: e7263d18
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 80050033 CR2: b76ab01c CR3: 2eb89de0 CR4: 000406f0

Stack:
 00000001 a220fb7b e9f4e000 00000002 419ff2d3 b3a05151 00000002 e9f4e5d8
 e9f4e000 419ff2d3 b3a05151 eecac310 c10b8154 b3a05151 419ff2d3 c10b78bd
 e9f4e000 e9f4e000 e9f4e5d8 00000001 e9f4e000 ec409000 eecac2cc eecac288
Call Trace:
 [<c10b8154>] ? __lock_acquire+0x3c4/0x760
 [<c10b78bd>] ? mark_held_locks+0x5d/0x80
 [<f8a10632>] f2fs_trim_fs+0x1c2/0x2e0 [f2fs]
 [<f89e9f56>] f2fs_ioctl+0x6b6/0x10b0 [f2fs]
 [<c13d51df>] ? __this_cpu_preempt_check+0xf/0x20
 [<c10b4281>] ? trace_hardirqs_off_caller+0x91/0x120
 [<f89e98a0>] ? __exchange_data_block+0xd30/0xd30 [f2fs]
 [<c120b2e1>] do_vfs_ioctl+0x81/0x7f0
 [<c11d57c5>] ? kmem_cache_free+0x245/0x2e0
 [<c1217840>] ? get_unused_fd_flags+0x40/0x40
 [<c1206eec>] ? putname+0x4c/0x50
 [<c11f631e>] ? do_sys_open+0x16e/0x1d0
 [<c1001990>] ? do_fast_syscall_32+0x30/0x1c0
 [<c13d51df>] ? __this_cpu_preempt_check+0xf/0x20
 [<c120baa8>] SyS_ioctl+0x58/0x80
 [<c1001a01>] do_fast_syscall_32+0xa1/0x1c0
 [<c178cc54>] sysenter_past_esp+0x45/0x74
EIP: [<f89fcefe>] write_checkpoint+0xfde/0x1020 [f2fs] SS:ESP 0068:e7263d18
---[ end trace 4de95d7e6b3aa7c6 ]---

The reason is: with below call stack, we will encounter BUG_ON during doing fstrim.

Thread A Thread B
- write_checkpoint
- do_checkpoint
- f2fs_write_inode
- update_inode_page
- update_inode
- set_page_dirty
- f2fs_set_node_page_dirty
- inc_page_count
- percpu_counter_inc
- set_sbi_flag(SBI_IS_DIRTY)
- clear_sbi_flag(SBI_IS_DIRTY)

Thread C Thread D
- f2fs_write_node_page
- set_node_addr
- __set_nat_cache_dirty
- nm_i->dirty_nat_cnt++
- do_vfs_ioctl
- f2fs_ioctl
- f2fs_trim_fs
- write_checkpoint
- f2fs_bug_on(nm_i->dirty_nat_cnt)

Fix it by setting superblock dirty correctly in do_checkpoint and f2fs_write_node_page.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 8 ++++++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 9 insertions(+)
fs/f2fs/checkpoint.c +0 -0
index cd0443d..1864078 100644

fs/f2fs/checkpoint.c static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc) · line 1162

Quoted 10 lines
fs/f2fs/checkpoint.c +7 -0
@@ -1153,6 +1153,14 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	clear_prefree_segments(sbi, cpc);
 	clear_sbi_flag(sbi, SBI_IS_DIRTY);
 
+	/*
+	 * redirty superblock if metadata like node page or inode cache is
+	 * updated during writting checkpoint.
+	 */
+	if (get_pages(sbi, F2FS_DIRTY_NODES) ||
+			get_pages(sbi, F2FS_DIRTY_IMETA))
+		set_sbi_flag(sbi, SBI_IS_DIRTY);

How about adding f2fs_bug_on(get_pages(sb, F2FS_DIRTY_DENTS)); ?

fs/f2fs/node.c static int f2fs_write_node_page(struct page *page, · line 1600

Quoted 13 lines
fs/f2fs/checkpoint.c +1 -0
+
 	return 0;
 }
fs/f2fs/node.c +1 -0
index 8a28800..365c6ff 100644
@@ -1597,6 +1597,7 @@ static int f2fs_write_node_page(struct page *page,
 	fio.old_blkaddr = ni.blk_addr;
 	write_node_page(nid, &fio);
 	set_node_addr(sbi, &ni, fio.new_blkaddr, is_fsync_dnode(page));
+	set_sbi_flag(sbi, SBI_IS_DIRTY);

It doesn't need to set this, right?

Thanks,

Quoted 5 lines
fs/f2fs/node.c +0 -0
 	dec_page_count(sbi, F2FS_DIRTY_NODES);
 	up_read(&sbi->node_write);
Signature · 1 line
2.7.2
#2 lore raw

Hi Jaegeuk,

Quoted 86 lines · Jaegeuk Kim

Hi Chao,

Quoted 83 lines · Chao Yu

From: Chao Yu <[email protected]>

tests/generic/251 of fstest suit complains us with below message:

------------[ cut here ]------------
invalid opcode: 0000 [#1] PREEMPT SMP
CPU: 2 PID: 7698 Comm: fstrim Tainted: G           O    4.7.0+ #21

task: e9f4e000 task.stack: e7262000
EIP: 0060:[<f89fcefe>] EFLAGS: 00010202 CPU: 2 EIP is at write_checkpoint+0xfde/0x1020 [f2fs]
EAX: f33eb300 EBX: eecac310 ECX: 00000001 EDX: ffff0001
ESI: eecac000 EDI: eecac5f0 EBP: e7263dec ESP: e7263d18
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 80050033 CR2: b76ab01c CR3: 2eb89de0 CR4: 000406f0

Stack:
 00000001 a220fb7b e9f4e000 00000002 419ff2d3 b3a05151 00000002 e9f4e5d8
 e9f4e000 419ff2d3 b3a05151 eecac310 c10b8154 b3a05151 419ff2d3 c10b78bd
 e9f4e000 e9f4e000 e9f4e5d8 00000001 e9f4e000 ec409000 eecac2cc eecac288
Call Trace:
 [<c10b8154>] ? __lock_acquire+0x3c4/0x760
 [<c10b78bd>] ? mark_held_locks+0x5d/0x80
 [<f8a10632>] f2fs_trim_fs+0x1c2/0x2e0 [f2fs]
 [<f89e9f56>] f2fs_ioctl+0x6b6/0x10b0 [f2fs]
 [<c13d51df>] ? __this_cpu_preempt_check+0xf/0x20
 [<c10b4281>] ? trace_hardirqs_off_caller+0x91/0x120
 [<f89e98a0>] ? __exchange_data_block+0xd30/0xd30 [f2fs]
 [<c120b2e1>] do_vfs_ioctl+0x81/0x7f0
 [<c11d57c5>] ? kmem_cache_free+0x245/0x2e0
 [<c1217840>] ? get_unused_fd_flags+0x40/0x40
 [<c1206eec>] ? putname+0x4c/0x50
 [<c11f631e>] ? do_sys_open+0x16e/0x1d0
 [<c1001990>] ? do_fast_syscall_32+0x30/0x1c0
 [<c13d51df>] ? __this_cpu_preempt_check+0xf/0x20
 [<c120baa8>] SyS_ioctl+0x58/0x80
 [<c1001a01>] do_fast_syscall_32+0xa1/0x1c0
 [<c178cc54>] sysenter_past_esp+0x45/0x74
EIP: [<f89fcefe>] write_checkpoint+0xfde/0x1020 [f2fs] SS:ESP 0068:e7263d18
---[ end trace 4de95d7e6b3aa7c6 ]---

The reason is: with below call stack, we will encounter BUG_ON during doing fstrim.

Thread A Thread B
- write_checkpoint
- do_checkpoint
- f2fs_write_inode
- update_inode_page
- update_inode
- set_page_dirty
- f2fs_set_node_page_dirty
- inc_page_count
- percpu_counter_inc
- set_sbi_flag(SBI_IS_DIRTY)
- clear_sbi_flag(SBI_IS_DIRTY)

Thread C Thread D
- f2fs_write_node_page
- set_node_addr
- __set_nat_cache_dirty
- nm_i->dirty_nat_cnt++
- do_vfs_ioctl
- f2fs_ioctl
- f2fs_trim_fs
- write_checkpoint
- f2fs_bug_on(nm_i->dirty_nat_cnt)

Fix it by setting superblock dirty correctly in do_checkpoint and f2fs_write_node_page.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 8 ++++++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 9 insertions(+)
fs/f2fs/checkpoint.c +0 -0
index cd0443d..1864078 100644
@@ -1153,6 +1153,14 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	clear_prefree_segments(sbi, cpc);
 	clear_sbi_flag(sbi, SBI_IS_DIRTY);
Quoted 9 lines

fs/f2fs/checkpoint.c static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc) · line 1162

Quoted 7 lines
fs/f2fs/checkpoint.c +7 -0
+	/*
+	 * redirty superblock if metadata like node page or inode cache is
+	 * updated during writting checkpoint.
+	 */
+	if (get_pages(sbi, F2FS_DIRTY_NODES) ||
+			get_pages(sbi, F2FS_DIRTY_IMETA))
+		set_sbi_flag(sbi, SBI_IS_DIRTY);

How about adding f2fs_bug_on(get_pages(sb, F2FS_DIRTY_DENTS)); ?

That's right, I will add it.

Quoted 5 lines
Quoted 5 lines
fs/f2fs/checkpoint.c +1 -0
+
 	return 0;
 }
Quoted 11 lines

fs/f2fs/node.c static int f2fs_write_node_page(struct page *page, · line 1600

Quoted 9 lines
fs/f2fs/node.c +1 -0
index 8a28800..365c6ff 100644
@@ -1597,6 +1597,7 @@ static int f2fs_write_node_page(struct page *page,
 	fio.old_blkaddr = ni.blk_addr;
 	write_node_page(nid, &fio);
 	set_node_addr(sbi, &ni, fio.new_blkaddr, is_fsync_dnode(page));
+	set_sbi_flag(sbi, SBI_IS_DIRTY);

It doesn't need to set this, right?

Agree, let me resend the patch. :)

Thanks,

Quoted 11 lines

Thanks,

Quoted 5 lines
fs/f2fs/node.c +0 -0
 	dec_page_count(sbi, F2FS_DIRTY_NODES);
 	up_read(&sbi->node_write);
Signature · 1 line
2.7.2

.