Re: [PATCH v4] f2fs: introduce mount option for fsync mode

[Chao Yu <chao@kernel.org>]

On 2018/2/28 20:09, Junling Zheng wrote:
> Commit "0a007b97aad6"(f2fs: recover directory operations by fsync)
> fixed xfstest generic/342 case, but it also increased the written
> data and caused the performance degradation. In most cases, there's
> no need to do so heavy fsync actually.
> 
> So we introduce new mount option "fsync_mode={posix,strict}" to
> control the policy of fsync. "fsync_mode=posix" is set by default,
> and means that f2fs uses a light fsync, which follows POSIX semantics.
> And "fsync_mode=strict" means that it's a heavy fsync, which behaves
> in line with xfs, ext4 and btrfs, where generic/342 will pass, but
> the performance will regress.

Let's handle to recover old fsync_mode in error path of ->remount_fs.

Thanks,

> 
> Signed-off-by: Junling Zheng <zhengjunling@huawei.com>
> ---
> Changes from v3:
>  - add fsync_mode in sbi to record fsync mode
>  - keep the extensibility for fsync_mode
> Changes from v2:
>  - Change to "fsync={posix,strict}" format
>  - Set "fsync=posix" default
> Changes from v1:
>  - Add document modify
>  - Add reviewer
>  Documentation/filesystems/f2fs.txt |  7 +++++++
>  fs/f2fs/dir.c                      |  3 ++-
>  fs/f2fs/f2fs.h                     |  8 ++++++++
>  fs/f2fs/file.c                     |  3 ++-
>  fs/f2fs/namei.c                    |  9 ++++++---
>  fs/f2fs/super.c                    | 24 ++++++++++++++++++++++++
>  6 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt
> index 0409c47584ea..514e44983a8e 100644
> --- a/Documentation/filesystems/f2fs.txt
> +++ b/Documentation/filesystems/f2fs.txt
> @@ -182,6 +182,13 @@ whint_mode=%s          Control which write hints are passed down to block
>                         passes down hints with its policy.
>  alloc_mode=%s          Adjust block allocation policy, which supports "reuse"
>                         and "default".
> +fsync_mode=%s          Control the policy of fsync. Currently supports "posix"
> +                       and "strict". In "posix" mode, which is default, fsync
> +                       will follow POSIX semantics and does a light operation
> +                       to improve the filesystem performance. In "strict" mode,
> +                       fsync will be heavy and behaves in line with xfs, ext4
> +                       and btrfs, where xfstest generic/342 will pass, but the
> +                       performance will regress.
>  
>  ================================================================================
>  DEBUGFS ENTRIES
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index f00b5ed8c011..cce8ce11c2cc 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -713,7 +713,8 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
>  
>  	f2fs_update_time(F2FS_I_SB(dir), REQ_TIME);
>  
> -	add_ino_entry(F2FS_I_SB(dir), dir->i_ino, TRANS_DIR_INO);
> +	if (F2FS_I_SB(dir)->fsync_mode == FSYNC_MODE_STRICT)
> +		add_ino_entry(F2FS_I_SB(dir), dir->i_ino, TRANS_DIR_INO);
>  
>  	if (f2fs_has_inline_dentry(dir))
>  		return f2fs_delete_inline_entry(dentry, page, dir, inode);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index e4a782b23e66..b85dcfb30bac 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1052,6 +1052,11 @@ enum {
>  	ALLOC_MODE_REUSE,	/* reuse segments as much as possible */
>  };
>  
> +enum fsync_mode {
> +	FSYNC_MODE_POSIX,	/* fsync follows posix semantics */
> +	FSYNC_MODE_STRICT,	/* fsync behaves in line with ext4 */
> +};
> +
>  struct f2fs_sb_info {
>  	struct super_block *sb;			/* pointer to VFS super block */
>  	struct proc_dir_entry *s_proc;		/* proc entry */
> @@ -1241,6 +1246,9 @@ struct f2fs_sb_info {
>  
>  	/* segment allocation policy */
>  	int alloc_mode;
> +
> +	/* fsync policy */
> +	int fsync_mode;
>  };
>  
>  #ifdef CONFIG_F2FS_FAULT_INJECTION
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 6a202e5a6386..99fa207fc310 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -165,7 +165,8 @@ static inline enum cp_reason_type need_do_checkpoint(struct inode *inode)
>  		cp_reason = CP_FASTBOOT_MODE;
>  	else if (sbi->active_logs == 2)
>  		cp_reason = CP_SPEC_LOG_NUM;
> -	else if (need_dentry_mark(sbi, inode->i_ino) &&
> +	else if (sbi->fsync_mode == FSYNC_MODE_STRICT &&
> +		need_dentry_mark(sbi, inode->i_ino) &&
>  		exist_written_data(sbi, F2FS_I(inode)->i_pino, TRANS_DIR_INO))
>  		cp_reason = CP_RECOVER_DIR;
>  
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 75135191a73d..010fc9a4a589 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -977,7 +977,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		}
>  		f2fs_i_links_write(old_dir, false);
>  	}
> -	add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
> +	if (sbi->fsync_mode == FSYNC_MODE_STRICT)
> +		add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
>  
>  	f2fs_unlock_op(sbi);
>  
> @@ -1132,8 +1133,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	}
>  	f2fs_mark_inode_dirty_sync(new_dir, false);
>  
> -	add_ino_entry(sbi, old_dir->i_ino, TRANS_DIR_INO);
> -	add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
> +	if (sbi->fsync_mode == FSYNC_MODE_STRICT) {
> +		add_ino_entry(sbi, old_dir->i_ino, TRANS_DIR_INO);
> +		add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
> +	}
>  
>  	f2fs_unlock_op(sbi);
>  
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 359dc031fb8c..67c07de87dad 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -131,6 +131,7 @@ enum {
>  	Opt_jqfmt_vfsv1,
>  	Opt_whint,
>  	Opt_alloc,
> +	Opt_fsync,
>  	Opt_err,
>  };
>  
> @@ -186,6 +187,7 @@ static match_table_t f2fs_tokens = {
>  	{Opt_jqfmt_vfsv1, "jqfmt=vfsv1"},
>  	{Opt_whint, "whint_mode=%s"},
>  	{Opt_alloc, "alloc_mode=%s"},
> +	{Opt_fsync, "fsync_mode=%s"},
>  	{Opt_err, NULL},
>  };
>  
> @@ -719,6 +721,22 @@ static int parse_options(struct super_block *sb, char *options)
>  			}
>  			kfree(name);
>  			break;
> +		case Opt_fsync:
> +			name = match_strdup(&args[0]);
> +			if (!name)
> +				return -ENOMEM;
> +			if (strlen(name) == 5 &&
> +					!strncmp(name, "posix", 5)) {
> +				sbi->fsync_mode = FSYNC_MODE_POSIX;
> +			} else if (strlen(name) == 6 &&
> +					!strncmp(name, "strict", 6)) {
> +				sbi->fsync_mode = FSYNC_MODE_STRICT;
> +			} else {
> +				kfree(name);
> +				return -EINVAL;
> +			}
> +			kfree(name);
> +			break;
>  		default:
>  			f2fs_msg(sb, KERN_ERR,
>  				"Unrecognized mount option \"%s\" or missing value",
> @@ -1287,6 +1305,11 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root)
>  		seq_printf(seq, ",alloc_mode=%s", "default");
>  	else if (sbi->alloc_mode == ALLOC_MODE_REUSE)
>  		seq_printf(seq, ",alloc_mode=%s", "reuse");
> +
> +	if (sbi->fsync_mode == FSYNC_MODE_POSIX)
> +		seq_printf(seq, ",fsync_mode=%s", "posix");
> +	else if (sbi->fsync_mode == FSYNC_MODE_STRICT)
> +		seq_printf(seq, ",fsync_mode=%s", "strict");
>  	return 0;
>  }
>  
> @@ -1297,6 +1320,7 @@ static void default_options(struct f2fs_sb_info *sbi)
>  	sbi->inline_xattr_size = DEFAULT_INLINE_XATTR_ADDRS;
>  	sbi->whint_mode = WHINT_MODE_OFF;
>  	sbi->alloc_mode = ALLOC_MODE_DEFAULT;
> +	sbi->fsync_mode = FSYNC_MODE_POSIX;
>  	sbi->readdir_ra = 1;
>  
>  	set_opt(sbi, BG_GC);
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot