Re: [f2fs-dev] [PATCH] f2fs: fix to set superblock dirty correctly

[Chao Yu]

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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>> .
>>
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>