Re: [f2fs-dev] [PATCH v3 1/2] f2fs: Check write pointer consistency of open zones

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Nov 28, 2019 / 20:26, Chao Yu wrote:
> On 2019/11/28 12:07, Shinichiro Kawasaki wrote:
> > On Nov 25, 2019 / 14:59, Chao Yu wrote:
> >> On 2019/11/14 16:19, Shin'ichiro Kawasaki wrote:
> >>> On sudden f2fs shutdown, write pointers of zoned block devices can go
> >>> further but f2fs meta data keeps current segments at positions before the
> >>> write operations. After remounting the f2fs, this inconsistency causes
> >>> write operations not at write pointers and "Unaligned write command"
> >>> error is reported.
> >>>
> >>> To avoid the error, compare current segments with write pointers of open
> >>> zones the current segments point to, during mount operation. If the write
> >>> pointer position is not aligned with the current segment position, assign
> >>> a new zone to the current segment. Also check the newly assigned zone has
> >>> write pointer at zone start. If not, make mount fail and ask users to run
> >>> fsck.
> >>>
> >>> Perform the consistency check during fsync recovery. Not to lose the
> >>> fsync data, do the check after fsync data gets restored and before
> >>> checkpoint commit which flushes data at current segment positions. Not to
> >>> cause conflict with kworker's dirfy data/node flush, do the fix within
> >>> SBI_POR_DOING protection.
> >>>
> >>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >>> ---
> >>>  fs/f2fs/f2fs.h     |   1 +
> >>>  fs/f2fs/recovery.c |  17 ++++++-
> >>>  fs/f2fs/segment.c  | 120 +++++++++++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 136 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index 4024790028aa..a2e24718c13b 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -3136,6 +3136,7 @@ void f2fs_write_node_summaries(struct f2fs_sb_info *sbi, block_t start_blk);
> >>>  int f2fs_lookup_journal_in_cursum(struct f2fs_journal *journal, int type,
> >>>  			unsigned int val, int alloc);
> >>>  void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>> +int f2fs_fix_curseg_write_pointer(struct f2fs_sb_info *sbi);
> >>>  int f2fs_build_segment_manager(struct f2fs_sb_info *sbi);
> >>>  void f2fs_destroy_segment_manager(struct f2fs_sb_info *sbi);
> >>>  int __init f2fs_create_segment_manager_caches(void);
> >>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>> index 783773e4560d..712054ed8d64 100644
> >>> --- a/fs/f2fs/recovery.c
> >>> +++ b/fs/f2fs/recovery.c
> >>> @@ -784,9 +784,22 @@ int f2fs_recover_fsync_data(struct f2fs_sb_info *sbi, bool check_only)
> >>>  	if (err) {
> >>>  		truncate_inode_pages_final(NODE_MAPPING(sbi));
> >>>  		truncate_inode_pages_final(META_MAPPING(sbi));
> >>> -	} else {
> >>> -		clear_sbi_flag(sbi, SBI_POR_DOING);
> >>>  	}
> >>> +
> >>> +	/*
> >>> +	 * If fsync data succeeds or there is no fsync data to recover,
> >>> +	 * and the f2fs is not read only, check and fix zoned block devices'
> >>> +	 * write pointer consistency.
> >>> +	 */
> >>> +	if (!ret && !err && !f2fs_readonly(sbi->sb)
> >>
> >> Using !check_only will be more readable?
> >>
> >> if (!err && !check_only && !f2fs_readonly(sbi->sb)
> > 
> > When check_only is on and there is no fsync data, I think we should fix the
> > write pointer inconsistency. With the condition you suggested, this case can
> > not be covered.
> 
> Alright.
> 
> > 
> > Having said that, my expression with !ret is not good from readability point
> > of view. How about this?
> > 
> > 
> > bool fix_curseg_write_pointer;
> > fix_curseg_write_pointer = !check_only || (check_only && list_empty(&inode_list));
> 
> fix_curseg_write_pointer = !check_only || list_empty(&inode_list); is enough.

Oops, thanks.

> 
> > 
> > ...
> > 
> > if (!err && fix_curseg_write_pointer && !f2fs_readonly(sbi->sb)
> > 	&& f2fs_sb_has_blkzoned(sbi)) {
> > 	err = f2fs_fix_curseg_write_pointer(sbi);
> > 	ret = err;
> > }
> 
> It's okay to me.

Will update the patch. Thanks!

--
Best Regards,
Shin'ichiro Kawasaki

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