Re: [f2fs-dev] [PATCH 1/2] f2fs: fix fsck inconsistency caused by incorrect nat_entry flag usage

[Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On 2026/3/12 02:16, Jaegeuk Kim wrote:
> On 03/10, Yongpeng Yang wrote:
>> From: Yongpeng Yang <yangyongpeng@xiaomi.com>
>>
>> f2fs_need_dentry_mark() reads nat_entry flags without mutual exclusion
>> with the checkpoint path, which can result in an incorrect inode block
>> marking state. The scenario is as follows:
>>
>> create & write & fsync 'file A'                 write checkpoint
>> - f2fs_do_sync_file // inline inode
>>   - f2fs_write_inode // inode folio is dirty
>>                                                  - f2fs_write_checkpoint
>>                                                   - f2fs_flush_merged_writes
>>                                                   - f2fs_sync_node_pages
>>   - f2fs_fsync_node_pages // no dirty node
>>   - f2fs_need_inode_block_update // return true
>>   - f2fs_fsync_node_pages // inode dirtied
>>    - f2fs_need_dentry_mark //return true
>>                                                   - f2fs_flush_nat_entries
>>                                                  - f2fs_write_checkpoint end
>>    - __write_node_folio // inode with DENT_BIT_SHIFT set
>>    SPO, "fsck --dry-run" find inode has already checkpointed but still
>>    with DENT_BIT_SHIFT set
>>
>> The state observed by f2fs_need_dentry_mark() can differ from the state
>> observed in __write_node_folio() after acquiring sbi->node_write. The
>> root cause is that the semantics of IS_CHECKPOINTED and
>> HAS_FSYNCED_INODE are only guaranteed after the checkpoint write has
>> fully completed.
>>
>> This patch moves set_dentry_mark() into __write_node_folio() and
>> protects it with the sbi->node_write lock.
>>
>> Fixes: 88bd02c9472a ("f2fs: fix conditions to remain recovery information in f2fs_sync_file")
>> Signed-off-by: Yongpeng Yang <yangyongpeng@xiaomi.com>
>> ---
>>   fs/f2fs/node.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 2fbfecaf3f7b..7d3b377cbc17 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1807,7 +1807,9 @@ static bool __write_node_folio(struct folio *folio, bool atomic, bool *submitted
>>   		if (IS_INODE(folio))
>>   			set_dentry_mark(folio,
>>   				f2fs_need_dentry_mark(sbi, ino_of_node(folio)));
>> -	}
>> +	} else if (IS_INODE(folio) && is_fsync_dnode(folio))
>> +		set_dentry_mark(folio,
>> +				f2fs_need_dentry_mark(sbi, ino_of_node(folio)));
> 
> Thanks, I applied with some clean-up as below. Could you please review this?
> 
> -       if (atomic) {
> -               if (!test_opt(sbi, NOBARRIER))
> -                       fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> -               if (IS_INODE(folio))
> -                       set_dentry_mark(folio,
> +       if (atomic && !test_opt(sbi, NOBARRIER))
> +               fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> +
> +       if (IS_INODE(folio) && (atomic || is_fsync_dnode(folio)))
> +               set_dentry_mark(folio,
>                                  f2fs_need_dentry_mark(sbi, ino_of_node(folio)));
> -       }

Looks good.

Thanks,

> 
> 
>>   
>>   	/* should add to global list before clearing PAGECACHE status */
>>   	if (f2fs_in_warm_node_list(folio)) {
>> @@ -1948,9 +1950,6 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>   					if (is_inode_flag_set(inode,
>>   								FI_DIRTY_INODE))
>>   						f2fs_update_inode(inode, folio);
>> -					if (!atomic)
>> -						set_dentry_mark(folio,
>> -							f2fs_need_dentry_mark(sbi, ino));
>>   				}
>>   				/* may be written by other thread */
>>   				if (!folio_test_dirty(folio))
>> -- 
>> 2.43.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel