Re: [f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
On 2018/2/28 12:49, Jaegeuk Kim wrote: > On 02/13, Chao Yu wrote: >> Hi Jaegeuk, >> >> On 2018/2/12 8:07, Jaegeuk Kim wrote: >>> On 02/11, Junling Zheng wrote: Hi, Jaegeuk On 2018/2/10 8:44, Jaegeuk Kim wrote: > On 02/02, 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 heavily fsync actually. >> >> So we introduce a new mount option "strict_fsync" to control the >> policy of fsync. It's set by default, and means that fsync follows >> POSIX semantics. And "nostrict_fsync" means that the behaviour is >> in line with xfs, ext4 and btrfs, where generic/342 will pass. > > How about adding "fsync=%s" to give another chance for fsync policies? >> >> Agreed. >> > OK, I'll give patch v3 to change to "fsync=%s" format. BTW, which policy do u think should be the default behavior for f2fs? Posix or ext4? >>> >>> The default should be like ext4 as fsync=strict. We may add fsync=posix for >>> this. >> >> I'd like to suggest using fsync=posix option by default, because in most >> popular >> in-used scenario of f2fs: android, all users of filesystem are >> posix-compliant, >> so there is no such requirement that fs needs do more than posix as >> generic/342 >> restricted. >> >> The performance of fsync=strict mode regressed as expected, so if we enable >> fsync=strict mode by default, I suspect it may make f2fs losing some kinds of >> benchmark competition. >> >> How do you think? > > Okay, I have no objection on that. Well to hear that, so let's wait Junling to update the patch. ;) Thanks, > > Thanks, > >> >> Thanks, >> >>> >>> Thanks, >>> Thanks Junling > Thanks, > >> >> Signed-off-by: Junling Zheng >> Reviewed-by: Chao Yu >> --- >> Documentation/filesystems/f2fs.txt | 4 >> fs/f2fs/dir.c | 3 ++- >> fs/f2fs/f2fs.h | 1 + >> fs/f2fs/file.c | 3 ++- >> fs/f2fs/namei.c| 9 ++--- >> fs/f2fs/super.c| 13 + >> 6 files changed, 28 insertions(+), 5 deletions(-) >> >>> >>> . >>> > > . > -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
On 02/13, Chao Yu wrote: > Hi Jaegeuk, > > On 2018/2/12 8:07, Jaegeuk Kim wrote: > > On 02/11, Junling Zheng wrote: > >> Hi, Jaegeuk > >> > >> On 2018/2/10 8:44, Jaegeuk Kim wrote: > >>> On 02/02, 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 heavily fsync actually. > > So we introduce a new mount option "strict_fsync" to control the > policy of fsync. It's set by default, and means that fsync follows > POSIX semantics. And "nostrict_fsync" means that the behaviour is > in line with xfs, ext4 and btrfs, where generic/342 will pass. > >>> > >>> How about adding "fsync=%s" to give another chance for fsync policies? > > Agreed. > > >>> > >> > >> OK, I'll give patch v3 to change to "fsync=%s" format. > >> BTW, which policy do u think should be the default behavior for f2fs? Posix > >> or ext4? > > > > The default should be like ext4 as fsync=strict. We may add fsync=posix for > > this. > > I'd like to suggest using fsync=posix option by default, because in most > popular > in-used scenario of f2fs: android, all users of filesystem are > posix-compliant, > so there is no such requirement that fs needs do more than posix as > generic/342 > restricted. > > The performance of fsync=strict mode regressed as expected, so if we enable > fsync=strict mode by default, I suspect it may make f2fs losing some kinds of > benchmark competition. > > How do you think? Okay, I have no objection on that. Thanks, > > Thanks, > > > > > Thanks, > > > >> > >> Thanks > >> Junling > >> > >>> Thanks, > >>> > > Signed-off-by: Junling Zheng > Reviewed-by: Chao Yu > --- > Documentation/filesystems/f2fs.txt | 4 > fs/f2fs/dir.c | 3 ++- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/file.c | 3 ++- > fs/f2fs/namei.c| 9 ++--- > fs/f2fs/super.c| 13 + > 6 files changed, 28 insertions(+), 5 deletions(-) > > > > > . > > -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
Hi Jaegeuk, On 2018/2/12 8:07, Jaegeuk Kim wrote: > On 02/11, Junling Zheng wrote: >> Hi, Jaegeuk >> >> On 2018/2/10 8:44, Jaegeuk Kim wrote: >>> On 02/02, 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 heavily fsync actually. So we introduce a new mount option "strict_fsync" to control the policy of fsync. It's set by default, and means that fsync follows POSIX semantics. And "nostrict_fsync" means that the behaviour is in line with xfs, ext4 and btrfs, where generic/342 will pass. >>> >>> How about adding "fsync=%s" to give another chance for fsync policies? Agreed. >>> >> >> OK, I'll give patch v3 to change to "fsync=%s" format. >> BTW, which policy do u think should be the default behavior for f2fs? Posix >> or ext4? > > The default should be like ext4 as fsync=strict. We may add fsync=posix for > this. I'd like to suggest using fsync=posix option by default, because in most popular in-used scenario of f2fs: android, all users of filesystem are posix-compliant, so there is no such requirement that fs needs do more than posix as generic/342 restricted. The performance of fsync=strict mode regressed as expected, so if we enable fsync=strict mode by default, I suspect it may make f2fs losing some kinds of benchmark competition. How do you think? Thanks, > > Thanks, > >> >> Thanks >> Junling >> >>> Thanks, >>> Signed-off-by: Junling Zheng Reviewed-by: Chao Yu --- Documentation/filesystems/f2fs.txt | 4 fs/f2fs/dir.c | 3 ++- fs/f2fs/f2fs.h | 1 + fs/f2fs/file.c | 3 ++- fs/f2fs/namei.c| 9 ++--- fs/f2fs/super.c| 13 + 6 files changed, 28 insertions(+), 5 deletions(-) > > . > -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
On 02/11, Junling Zheng wrote: > Hi, Jaegeuk > > On 2018/2/10 8:44, Jaegeuk Kim wrote: > > On 02/02, 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 heavily fsync actually. > >> > >> So we introduce a new mount option "strict_fsync" to control the > >> policy of fsync. It's set by default, and means that fsync follows > >> POSIX semantics. And "nostrict_fsync" means that the behaviour is > >> in line with xfs, ext4 and btrfs, where generic/342 will pass. > > > > How about adding "fsync=%s" to give another chance for fsync policies? > > > > OK, I'll give patch v3 to change to "fsync=%s" format. > BTW, which policy do u think should be the default behavior for f2fs? Posix > or ext4? The default should be like ext4 as fsync=strict. We may add fsync=posix for this. Thanks, > > Thanks > Junling > > > Thanks, > > > >> > >> Signed-off-by: Junling Zheng > >> Reviewed-by: Chao Yu > >> --- > >> Documentation/filesystems/f2fs.txt | 4 > >> fs/f2fs/dir.c | 3 ++- > >> fs/f2fs/f2fs.h | 1 + > >> fs/f2fs/file.c | 3 ++- > >> fs/f2fs/namei.c| 9 ++--- > >> fs/f2fs/super.c| 13 + > >> 6 files changed, 28 insertions(+), 5 deletions(-) > >> -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
Hi, Jaegeuk On 2018/2/10 8:44, Jaegeuk Kim wrote: > On 02/02, 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 heavily fsync actually. >> >> So we introduce a new mount option "strict_fsync" to control the >> policy of fsync. It's set by default, and means that fsync follows >> POSIX semantics. And "nostrict_fsync" means that the behaviour is >> in line with xfs, ext4 and btrfs, where generic/342 will pass. > > How about adding "fsync=%s" to give another chance for fsync policies? > OK, I'll give patch v3 to change to "fsync=%s" format. BTW, which policy do u think should be the default behavior for f2fs? Posix or ext4? Thanks Junling > Thanks, > >> >> Signed-off-by: Junling Zheng >> Reviewed-by: Chao Yu >> --- >> Documentation/filesystems/f2fs.txt | 4 >> fs/f2fs/dir.c | 3 ++- >> fs/f2fs/f2fs.h | 1 + >> fs/f2fs/file.c | 3 ++- >> fs/f2fs/namei.c| 9 ++--- >> fs/f2fs/super.c| 13 + >> 6 files changed, 28 insertions(+), 5 deletions(-) >> -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
On 02/02, 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 heavily fsync actually. > > So we introduce a new mount option "strict_fsync" to control the > policy of fsync. It's set by default, and means that fsync follows > POSIX semantics. And "nostrict_fsync" means that the behaviour is > in line with xfs, ext4 and btrfs, where generic/342 will pass. How about adding "fsync=%s" to give another chance for fsync policies? Thanks, > > Signed-off-by: Junling Zheng > Reviewed-by: Chao Yu > --- > Documentation/filesystems/f2fs.txt | 4 > fs/f2fs/dir.c | 3 ++- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/file.c | 3 ++- > fs/f2fs/namei.c| 9 ++--- > fs/f2fs/super.c| 13 + > 6 files changed, 28 insertions(+), 5 deletions(-) > > diff --git a/Documentation/filesystems/f2fs.txt > b/Documentation/filesystems/f2fs.txt > index 0caf7da0a532..c484ce8d1f4c 100644 > --- a/Documentation/filesystems/f2fs.txt > +++ b/Documentation/filesystems/f2fs.txt > @@ -180,6 +180,10 @@ whint_mode=%s Control which write hints are > passed down to block > down hints. In "user-based" mode, f2fs tries to pass > down hints given by users. And in "fs-based" mode, > f2fs > passes down hints with its policy. > +{,no}strict_fsync Control the policy of fsync. Set "strict_fsync" by > default, > + which means that fsync will follow POSIX semantics. > Use > + "nostrict_fsync" if you expect fsync to behave in > line with > + xfs, ext4 and btrfs, where xfstest generic/342 will > pass. > > > > DEBUGFS ENTRIES > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index f00b5ed8c011..7487b7e77a36 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 (!test_opt(F2FS_I_SB(dir), STRICT_FSYNC)) > + 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 dbe87c7a266e..8cf914d12f17 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -97,6 +97,7 @@ extern char *fault_name[FAULT_MAX]; > #define F2FS_MOUNT_QUOTA 0x0040 > #define F2FS_MOUNT_INLINE_XATTR_SIZE 0x0080 > #define F2FS_MOUNT_RESERVE_ROOT 0x0100 > +#define F2FS_MOUNT_STRICT_FSYNC 0x0200 > > #define clear_opt(sbi, option) ((sbi)->mount_opt.opt &= > ~F2FS_MOUNT_##option) > #define set_opt(sbi, option) ((sbi)->mount_opt.opt |= F2FS_MOUNT_##option) > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 672a542e5464..9b39254f5b48 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 (!test_opt(sbi, STRICT_FSYNC) && > + 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 c4c94c7e9f4f..ef86ae327f91 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -936,7 +936,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 (!test_opt(sbi, STRICT_FSYNC)) > + add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO); > > f2fs_unlock_op(sbi); > > @@ -1091,8 +1092,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 (!test_opt(sbi, STRICT_FSYNC)) { > + 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 7966cf7bfb8
[f2fs-dev] [PATCH v2] f2fs: introduce "strict_fsync" for posix standard fsync
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 heavily fsync actually. So we introduce a new mount option "strict_fsync" to control the policy of fsync. It's set by default, and means that fsync follows POSIX semantics. And "nostrict_fsync" means that the behaviour is in line with xfs, ext4 and btrfs, where generic/342 will pass. Signed-off-by: Junling Zheng Reviewed-by: Chao Yu --- Documentation/filesystems/f2fs.txt | 4 fs/f2fs/dir.c | 3 ++- fs/f2fs/f2fs.h | 1 + fs/f2fs/file.c | 3 ++- fs/f2fs/namei.c| 9 ++--- fs/f2fs/super.c| 13 + 6 files changed, 28 insertions(+), 5 deletions(-) diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt index 0caf7da0a532..c484ce8d1f4c 100644 --- a/Documentation/filesystems/f2fs.txt +++ b/Documentation/filesystems/f2fs.txt @@ -180,6 +180,10 @@ whint_mode=%s Control which write hints are passed down to block down hints. In "user-based" mode, f2fs tries to pass down hints given by users. And in "fs-based" mode, f2fs passes down hints with its policy. +{,no}strict_fsync Control the policy of fsync. Set "strict_fsync" by default, + which means that fsync will follow POSIX semantics. Use + "nostrict_fsync" if you expect fsync to behave in line with + xfs, ext4 and btrfs, where xfstest generic/342 will pass. DEBUGFS ENTRIES diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index f00b5ed8c011..7487b7e77a36 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 (!test_opt(F2FS_I_SB(dir), STRICT_FSYNC)) + 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 dbe87c7a266e..8cf914d12f17 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -97,6 +97,7 @@ extern char *fault_name[FAULT_MAX]; #define F2FS_MOUNT_QUOTA 0x0040 #define F2FS_MOUNT_INLINE_XATTR_SIZE 0x0080 #define F2FS_MOUNT_RESERVE_ROOT0x0100 +#define F2FS_MOUNT_STRICT_FSYNC0x0200 #define clear_opt(sbi, option) ((sbi)->mount_opt.opt &= ~F2FS_MOUNT_##option) #define set_opt(sbi, option) ((sbi)->mount_opt.opt |= F2FS_MOUNT_##option) diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 672a542e5464..9b39254f5b48 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 (!test_opt(sbi, STRICT_FSYNC) && + 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 c4c94c7e9f4f..ef86ae327f91 100644 --- a/fs/f2fs/namei.c +++ b/fs/f2fs/namei.c @@ -936,7 +936,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 (!test_opt(sbi, STRICT_FSYNC)) + add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO); f2fs_unlock_op(sbi); @@ -1091,8 +1092,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 (!test_opt(sbi, STRICT_FSYNC)) { + 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 7966cf7bfb8e..3066fc9d8985 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -130,6 +130,8 @@ enum { Opt_jqfmt_vfsv0, Opt_jqfmt_vfsv1, Opt_whint, + Opt_strict_fsync, + Opt_nostrict_fsync, Opt_err, }; @@ -184,6 +186,8 @@ static match_table_t f2fs_to