* [PATCH] f2fs: fix to set superblock dirty correctly
@ 2016-08-26 14:20 Chao Yu
2016-08-27 1:01 ` Jaegeuk Kim
0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu @ 2016-08-26 14:20 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu
From: Chao Yu <yuchao0@huawei.com>
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 <yuchao0@huawei.com>
---
fs/f2fs/checkpoint.c | 4 ++++
fs/f2fs/node.c | 1 +
2 files changed, 5 insertions(+)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index cd0443d..68c723c 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -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;
}
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 8a28800..365c6ff 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -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);
--
2.7.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: fix to set superblock dirty correctly
2016-08-26 14:20 [PATCH] f2fs: fix to set superblock dirty correctly Chao Yu
@ 2016-08-27 1:01 ` Jaegeuk Kim
2016-08-27 1:54 ` [f2fs-dev] " heyunlei
2016-08-27 14:59 ` Chao Yu
0 siblings, 2 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2016-08-27 1:01 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu
On Fri, Aug 26, 2016 at 10:20:18PM +0800, Chao Yu wrote:
> From: Chao Yu <yuchao0@huawei.com>
>
> 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 <yuchao0@huawei.com>
> ---
> fs/f2fs/checkpoint.c | 4 ++++
> fs/f2fs/node.c | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index cd0443d..68c723c 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -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,
> + set_sbi_flag(sbi, SBI_IS_DIRTY);
> +
> return 0;
> }
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 8a28800..365c6ff 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -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);
>
> --
> 2.7.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set superblock dirty correctly
2016-08-27 1:01 ` Jaegeuk Kim
@ 2016-08-27 1:54 ` heyunlei
2016-08-27 15:04 ` Chao Yu
2016-08-27 14:59 ` Chao Yu
1 sibling, 1 reply; 5+ messages in thread
From: heyunlei @ 2016-08-27 1:54 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
Hi all,
On 2016/8/27 9:01, Jaegeuk Kim wrote:
> On Fri, Aug 26, 2016 at 10:20:18PM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> 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 <yuchao0@huawei.com>
>> ---
>> fs/f2fs/checkpoint.c | 4 ++++
>> fs/f2fs/node.c | 1 +
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index cd0443d..68c723c 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -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,
Maybe we can add this:
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7c8e219..d32539a 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -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.
>
>> + set_sbi_flag(sbi, SBI_IS_DIRTY);
>> +
>> return 0;
>> }
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 8a28800..365c6ff 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -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);
>>
>> --
>> 2.7.2
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: fix to set superblock dirty correctly
2016-08-27 1:01 ` Jaegeuk Kim
2016-08-27 1:54 ` [f2fs-dev] " heyunlei
@ 2016-08-27 14:59 ` Chao Yu
1 sibling, 0 replies; 5+ messages in thread
From: Chao Yu @ 2016-08-27 14:59 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu
On 2016/8/27 9:01, Jaegeuk Kim wrote:
> On Fri, Aug 26, 2016 at 10:20:18PM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> 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 <yuchao0@huawei.com>
>> ---
>> fs/f2fs/checkpoint.c | 4 ++++
>> fs/f2fs/node.c | 1 +
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index cd0443d..68c723c 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -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?
Need to check F2FS_DIRTY_IMETA additionally? since F2FS_DIRTY_DENTS will not be
updated during checkpoint.
> 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,
>
> Thanks,
>
>> + set_sbi_flag(sbi, SBI_IS_DIRTY);
>> +
>> return 0;
>> }
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 8a28800..365c6ff 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -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);
>>
>> --
>> 2.7.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set superblock dirty correctly
2016-08-27 1:54 ` [f2fs-dev] " heyunlei
@ 2016-08-27 15:04 ` Chao Yu
0 siblings, 0 replies; 5+ messages in thread
From: Chao Yu @ 2016-08-27 15:04 UTC (permalink / raw)
To: heyunlei, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
Hi Yunlei,
On 2016/8/27 9:54, heyunlei wrote:
> Hi all,
>
> On 2016/8/27 9:01, Jaegeuk Kim wrote:
>> On Fri, Aug 26, 2016 at 10:20:18PM +0800, Chao Yu wrote:
>>> From: Chao Yu <yuchao0@huawei.com>
>>>
>>> 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 <yuchao0@huawei.com>
>>> ---
>>> fs/f2fs/checkpoint.c | 4 ++++
>>> fs/f2fs/node.c | 1 +
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index cd0443d..68c723c 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -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,
> Maybe we can add this:
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 7c8e219..d32539a 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -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?
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,
>
> Thanks.
>
>
>
>>
>>> + set_sbi_flag(sbi, SBI_IS_DIRTY);
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index 8a28800..365c6ff 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -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);
>>>
>>> --
>>> 2.7.2
>>
>> ------------------------------------------------------------------------------
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>> .
>>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-27 15:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-26 14:20 [PATCH] f2fs: fix to set superblock dirty correctly Chao Yu
2016-08-27 1:01 ` Jaegeuk Kim
2016-08-27 1:54 ` [f2fs-dev] " heyunlei
2016-08-27 15:04 ` Chao Yu
2016-08-27 14:59 ` Chao Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).