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

[Jaegeuk Kim via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

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)));
-       }


>  
>  	/* 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