f2fs: fix to set superblock dirty correctly

PATCH Superseded Chao Yu 3 participants 5 messages Aug 26, 2016 – Aug 27, 2016 F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2016-08-26 14:22 UTC #0 lore raw

PATCH 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 | 4 ++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 5 insertions(+)

fs/f2fs/checkpoint.c +4 -0

index cd0443d..68c723c 100644
@@ -1153,6 +1153,10 @@ 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 node page is updated by ->write_inode */
+	if (get_pages(sbi, F2FS_DIRTY_NODES))
+		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

Jaegeuk Kim

maintainer

2016-08-27 01:01 UTC #1 lore raw

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 | 4 ++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 5 insertions(+)
fs/f2fs/checkpoint.c +0 -0
index cd0443d..68c723c 100644

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

Quoted 5 lines

fs/f2fs/checkpoint.c +2 -0

@@ -1153,6 +1153,10 @@ 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 node page is updated by ->write_inode */
+	if (get_pages(sbi, F2FS_DIRTY_NODES))

Need to check F2FS_DIRTY_IMETA and F2FS_DIRTY_DENTS as well?
And, if we have this, I don't think we need to worry about f2fs_lock_op() for update_inode_page() as well.

Thanks,

Quoted 19 lines

fs/f2fs/checkpoint.c +2 -0

+		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

------------------------------------------------------------------------------

heyunlei

2016-08-27 01:54 UTC #2 lore raw

Hi all,

Quoted 84 lines · Jaegeuk Kim

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 | 4 ++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 5 insertions(+)
fs/f2fs/checkpoint.c +0 -0
index cd0443d..68c723c 100644
@@ -1153,6 +1153,10 @@ 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 8 lines

fs/f2fs/checkpoint.c static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc) · line 1157
Quoted 2 lines
fs/f2fs/checkpoint.c +2 -0
+	/* redirty superblock if node page is updated by ->write_inode */
+	if (get_pages(sbi, F2FS_DIRTY_NODES))
Need to check F2FS_DIRTY_IMETA and F2FS_DIRTY_DENTS as well?
And, if we have this, I don't think we need to worry about f2fs_lock_op() for update_inode_page() as well.
Thanks,

Maybe we can add this:

fs/f2fs/data.c +3 -0

index 7c8e219..d32539a 100644
@@ -267,6 +267,9 @@ void f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 
         down_write(&io->io_rwsem);
 
+       if (!is_read)
+               set_sbi_flag(sbi, SBI_IS_DIRTY);
+
         if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
             (io->fio.op != fio->op || io->fio.op_flags != fio->op_flags)))
                 __submit_merged_bio(io);

This is deleted by this patch:

f2fs: use bio count instead of F2FS_WRITEBACK page count

which one is better?

Thanks.

Quoted 27 lines

Quoted 19 lines
fs/f2fs/checkpoint.c +2 -0
+		set_sbi_flag(sbi, SBI_IS_DIRTY);
+
 	return 0;
 }
 
fs/f2fs/node.c +1 -1
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);
 
--
Quoted 1 line
2.7.2
------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Quoted 2 lines

.

------------------------------------------------------------------------------

Chao Yu

author maintainer

2016-08-27 15:04 UTC #3 lore raw

Hi Yunlei,

Quoted 112 lines · heyunlei

Hi all,
Quoted 84 lines · Jaegeuk Kim
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 | 4 ++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 5 insertions(+)
fs/f2fs/checkpoint.c +0 -0
index cd0443d..68c723c 100644
@@ -1153,6 +1153,10 @@ 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 8 lines
fs/f2fs/checkpoint.c static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc) · line 1157
Quoted 2 lines
fs/f2fs/checkpoint.c +2 -0
+	/* redirty superblock if node page is updated by ->write_inode */
+	if (get_pages(sbi, F2FS_DIRTY_NODES))
Need to check F2FS_DIRTY_IMETA and F2FS_DIRTY_DENTS as well?
And, if we have this, I don't think we need to worry about f2fs_lock_op() for update_inode_page() as well.
Thanks,
Maybe we can add this:
fs/f2fs/data.c +3 -0
index 7c8e219..d32539a 100644
@@ -267,6 +267,9 @@ void f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 
         down_write(&io->io_rwsem);
 
+       if (!is_read)
+               set_sbi_flag(sbi, SBI_IS_DIRTY);
+
         if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
             (io->fio.op != fio->op || io->fio.op_flags != fio->op_flags)))
                 __submit_merged_bio(io);

Quoted 5 lines

This is deleted by this patch:
f2fs: use bio count instead of F2FS_WRITEBACK page count
which one is better?

I think we don't need to set fs as dirty state, if IPU is triggered, metadata of filesystem is not updated, so we should not set the flag to avoid further unneeded checkpoint.

Thanks,

Quoted 42 lines

Thanks.
Quoted 27 lines
Quoted 19 lines
fs/f2fs/checkpoint.c +2 -0
+		set_sbi_flag(sbi, SBI_IS_DIRTY);
+
 	return 0;
 }
 
fs/f2fs/node.c +1 -1
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);
 
--
Quoted 1 line
2.7.2
------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Quoted 2 lines
.
------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

author maintainer

2016-08-27 14:59 UTC #4 lore raw

In reply to Jaegeuk Kim

Quoted 84 lines · Jaegeuk Kim

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 | 4 ++++
 fs/f2fs/node.c       | 1 +
 2 files changed, 5 insertions(+)
fs/f2fs/checkpoint.c +0 -0
index cd0443d..68c723c 100644
@@ -1153,6 +1153,10 @@ 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 4 lines

fs/f2fs/checkpoint.c static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc) · line 1157
Quoted 2 lines
fs/f2fs/checkpoint.c +2 -0
+	/* redirty superblock if node page is updated by ->write_inode */
+	if (get_pages(sbi, F2FS_DIRTY_NODES))
Need to check F2FS_DIRTY_IMETA and F2FS_DIRTY_DENTS as well?

Need to check F2FS_DIRTY_IMETA additionally? since F2FS_DIRTY_DENTS will not be updated during checkpoint.

Quoted 2 lines

And, if we have this, I don't think we need to worry about f2fs_lock_op() for update_inode_page() as well.

OK, I didn't find any data consistency issue related to this so far.

Thanks,

Quoted 22 lines

Thanks,

Quoted 19 lines

fs/f2fs/checkpoint.c +2 -0

+		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