* [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-10-21 2:31 ` qixiaoyu1 0 siblings, 0 replies; 18+ messages in thread From: qixiaoyu1 @ 2022-10-21 2:31 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. Fix to apply it to all IPU policy. Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> --- fs/f2fs/data.c | 8 +++----- fs/f2fs/file.c | 4 +++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index a71e818cd67b..fec8e15fe820 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && is_inode_flag_set(inode, FI_OPU_WRITE)) return false; + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ + if (is_inode_flag_set(inode, FI_NEED_IPU)) + return true; if (policy & (0x1 << F2FS_IPU_FORCE)) return true; if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, !IS_ENCRYPTED(inode)) return true; - /* this is only set during fdatasync */ - if (policy & (0x1 << F2FS_IPU_FSYNC) && - is_inode_flag_set(inode, FI_NEED_IPU)) - return true; - if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) return true; diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 82cda1258227..08091550cdf2 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, goto go_write; /* if fdatasync is triggered, let's do in-place-update */ - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) set_inode_flag(inode, FI_NEED_IPU); + ret = file_write_and_wait_range(file, start, end); clear_inode_flag(inode, FI_NEED_IPU); -- 2.36.1 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-10-21 2:31 ` qixiaoyu1 0 siblings, 0 replies; 18+ messages in thread From: qixiaoyu1 @ 2022-10-21 2:31 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, qixiaoyu1 Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. Fix to apply it to all IPU policy. Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> --- fs/f2fs/data.c | 8 +++----- fs/f2fs/file.c | 4 +++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index a71e818cd67b..fec8e15fe820 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && is_inode_flag_set(inode, FI_OPU_WRITE)) return false; + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ + if (is_inode_flag_set(inode, FI_NEED_IPU)) + return true; if (policy & (0x1 << F2FS_IPU_FORCE)) return true; if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, !IS_ENCRYPTED(inode)) return true; - /* this is only set during fdatasync */ - if (policy & (0x1 << F2FS_IPU_FSYNC) && - is_inode_flag_set(inode, FI_NEED_IPU)) - return true; - if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) return true; diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 82cda1258227..08091550cdf2 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, goto go_write; /* if fdatasync is triggered, let's do in-place-update */ - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) set_inode_flag(inode, FI_NEED_IPU); + ret = file_write_and_wait_range(file, start, end); clear_inode_flag(inode, FI_NEED_IPU); -- 2.36.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-10-21 2:31 ` qixiaoyu1 @ 2022-10-31 11:27 ` qixiaoyu -1 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-10-31 11:27 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel Friendly ping... On Fri, Oct 21, 2022 at 10:31:36AM +0800, qixiaoyu1 wrote: > Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > Fix to apply it to all IPU policy. > > Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > --- > fs/f2fs/data.c | 8 +++----- > fs/f2fs/file.c | 4 +++- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index a71e818cd67b..fec8e15fe820 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > is_inode_flag_set(inode, FI_OPU_WRITE)) > return false; > + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > + if (is_inode_flag_set(inode, FI_NEED_IPU)) > + return true; > if (policy & (0x1 << F2FS_IPU_FORCE)) > return true; > if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > !IS_ENCRYPTED(inode)) > return true; > > - /* this is only set during fdatasync */ > - if (policy & (0x1 << F2FS_IPU_FSYNC) && > - is_inode_flag_set(inode, FI_NEED_IPU)) > - return true; > - > if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > return true; > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 82cda1258227..08091550cdf2 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > goto go_write; > > /* if fdatasync is triggered, let's do in-place-update */ > - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > set_inode_flag(inode, FI_NEED_IPU); > + > ret = file_write_and_wait_range(file, start, end); > clear_inode_flag(inode, FI_NEED_IPU); > > -- > 2.36.1 > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-10-31 11:27 ` qixiaoyu 0 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-10-31 11:27 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, qixiaoyu1 Friendly ping... On Fri, Oct 21, 2022 at 10:31:36AM +0800, qixiaoyu1 wrote: > Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > Fix to apply it to all IPU policy. > > Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > --- > fs/f2fs/data.c | 8 +++----- > fs/f2fs/file.c | 4 +++- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index a71e818cd67b..fec8e15fe820 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > is_inode_flag_set(inode, FI_OPU_WRITE)) > return false; > + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > + if (is_inode_flag_set(inode, FI_NEED_IPU)) > + return true; > if (policy & (0x1 << F2FS_IPU_FORCE)) > return true; > if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > !IS_ENCRYPTED(inode)) > return true; > > - /* this is only set during fdatasync */ > - if (policy & (0x1 << F2FS_IPU_FSYNC) && > - is_inode_flag_set(inode, FI_NEED_IPU)) > - return true; > - > if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > return true; > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 82cda1258227..08091550cdf2 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > goto go_write; > > /* if fdatasync is triggered, let's do in-place-update */ > - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > set_inode_flag(inode, FI_NEED_IPU); > + > ret = file_write_and_wait_range(file, start, end); > clear_inode_flag(inode, FI_NEED_IPU); > > -- > 2.36.1 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-10-21 2:31 ` qixiaoyu1 @ 2022-11-01 15:14 ` Chao Yu -1 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-01 15:14 UTC (permalink / raw) To: qixiaoyu1, Jaegeuk Kim; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel On 2022/10/21 10:31, qixiaoyu1 wrote: > Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > Fix to apply it to all IPU policy. Xiaoyu, Sorry for the delay. I didn't get the point, can you please explain more about the issue? Thanks, > > Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > --- > fs/f2fs/data.c | 8 +++----- > fs/f2fs/file.c | 4 +++- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index a71e818cd67b..fec8e15fe820 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > is_inode_flag_set(inode, FI_OPU_WRITE)) > return false; > + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > + if (is_inode_flag_set(inode, FI_NEED_IPU)) > + return true; > if (policy & (0x1 << F2FS_IPU_FORCE)) > return true; > if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > !IS_ENCRYPTED(inode)) > return true; > > - /* this is only set during fdatasync */ > - if (policy & (0x1 << F2FS_IPU_FSYNC) && > - is_inode_flag_set(inode, FI_NEED_IPU)) > - return true; > - > if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > return true; > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 82cda1258227..08091550cdf2 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > goto go_write; > > /* if fdatasync is triggered, let's do in-place-update */ > - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > set_inode_flag(inode, FI_NEED_IPU); > + > ret = file_write_and_wait_range(file, start, end); > clear_inode_flag(inode, FI_NEED_IPU); > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-01 15:14 ` Chao Yu 0 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-01 15:14 UTC (permalink / raw) To: qixiaoyu1, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel, qixiaoyu1 On 2022/10/21 10:31, qixiaoyu1 wrote: > Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > Fix to apply it to all IPU policy. Xiaoyu, Sorry for the delay. I didn't get the point, can you please explain more about the issue? Thanks, > > Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > --- > fs/f2fs/data.c | 8 +++----- > fs/f2fs/file.c | 4 +++- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index a71e818cd67b..fec8e15fe820 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > is_inode_flag_set(inode, FI_OPU_WRITE)) > return false; > + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > + if (is_inode_flag_set(inode, FI_NEED_IPU)) > + return true; > if (policy & (0x1 << F2FS_IPU_FORCE)) > return true; > if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > !IS_ENCRYPTED(inode)) > return true; > > - /* this is only set during fdatasync */ > - if (policy & (0x1 << F2FS_IPU_FSYNC) && > - is_inode_flag_set(inode, FI_NEED_IPU)) > - return true; > - > if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > return true; > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 82cda1258227..08091550cdf2 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > goto go_write; > > /* if fdatasync is triggered, let's do in-place-update */ > - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > set_inode_flag(inode, FI_NEED_IPU); > + > ret = file_write_and_wait_range(file, start, end); > clear_inode_flag(inode, FI_NEED_IPU); > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-11-01 15:14 ` Chao Yu @ 2022-11-02 12:25 ` qixiaoyu -1 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-11-02 12:25 UTC (permalink / raw) To: Chao Yu; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel Hi Chao, fdatasync do in-place-update to avoid additional node writes, but currently it only do that with F2FS_IPU_FSYNC as: f2fs_do_sync_file: if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) set_inode_flag(inode, FI_NEED_IPU); check_inplace_update_policy: /* this is only set during fdatasync */ if (policy & (0x1 << F2FS_IPU_FSYNC) && is_inode_flag_set(inode, FI_NEED_IPU)) return true; So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to apply it to all IPU policy. BTW, we found small performance improvement with this patch on AndroBench app using F2FS_IPU_SSR_UTIL on our product: F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 Thanks On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: > On 2022/10/21 10:31, qixiaoyu1 wrote: > >Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > >Fix to apply it to all IPU policy. > > Xiaoyu, > > Sorry for the delay. > > I didn't get the point, can you please explain more about the > issue? > > Thanks, > > > > >Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > >--- > > fs/f2fs/data.c | 8 +++----- > > fs/f2fs/file.c | 4 +++- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > >diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >index a71e818cd67b..fec8e15fe820 100644 > >--- a/fs/f2fs/data.c > >+++ b/fs/f2fs/data.c > >@@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > > if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > > is_inode_flag_set(inode, FI_OPU_WRITE)) > > return false; > >+ /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > >+ if (is_inode_flag_set(inode, FI_NEED_IPU)) > >+ return true; > > if (policy & (0x1 << F2FS_IPU_FORCE)) > > return true; > > if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > >@@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > > !IS_ENCRYPTED(inode)) > > return true; > >- /* this is only set during fdatasync */ > >- if (policy & (0x1 << F2FS_IPU_FSYNC) && > >- is_inode_flag_set(inode, FI_NEED_IPU)) > >- return true; > >- > > if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > > !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > > return true; > >diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >index 82cda1258227..08091550cdf2 100644 > >--- a/fs/f2fs/file.c > >+++ b/fs/f2fs/file.c > >@@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > > goto go_write; > > /* if fdatasync is triggered, let's do in-place-update */ > >- if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >+ if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > >+ get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > > set_inode_flag(inode, FI_NEED_IPU); > >+ > > ret = file_write_and_wait_range(file, start, end); > > clear_inode_flag(inode, FI_NEED_IPU); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-02 12:25 ` qixiaoyu 0 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-11-02 12:25 UTC (permalink / raw) To: Chao Yu; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel Hi Chao, fdatasync do in-place-update to avoid additional node writes, but currently it only do that with F2FS_IPU_FSYNC as: f2fs_do_sync_file: if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) set_inode_flag(inode, FI_NEED_IPU); check_inplace_update_policy: /* this is only set during fdatasync */ if (policy & (0x1 << F2FS_IPU_FSYNC) && is_inode_flag_set(inode, FI_NEED_IPU)) return true; So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to apply it to all IPU policy. BTW, we found small performance improvement with this patch on AndroBench app using F2FS_IPU_SSR_UTIL on our product: F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 Thanks On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: > On 2022/10/21 10:31, qixiaoyu1 wrote: > >Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > >Fix to apply it to all IPU policy. > > Xiaoyu, > > Sorry for the delay. > > I didn't get the point, can you please explain more about the > issue? > > Thanks, > > > > >Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > >--- > > fs/f2fs/data.c | 8 +++----- > > fs/f2fs/file.c | 4 +++- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > >diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >index a71e818cd67b..fec8e15fe820 100644 > >--- a/fs/f2fs/data.c > >+++ b/fs/f2fs/data.c > >@@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > > if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > > is_inode_flag_set(inode, FI_OPU_WRITE)) > > return false; > >+ /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > >+ if (is_inode_flag_set(inode, FI_NEED_IPU)) > >+ return true; > > if (policy & (0x1 << F2FS_IPU_FORCE)) > > return true; > > if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > >@@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > > !IS_ENCRYPTED(inode)) > > return true; > >- /* this is only set during fdatasync */ > >- if (policy & (0x1 << F2FS_IPU_FSYNC) && > >- is_inode_flag_set(inode, FI_NEED_IPU)) > >- return true; > >- > > if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > > !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > > return true; > >diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >index 82cda1258227..08091550cdf2 100644 > >--- a/fs/f2fs/file.c > >+++ b/fs/f2fs/file.c > >@@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > > goto go_write; > > /* if fdatasync is triggered, let's do in-place-update */ > >- if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >+ if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > >+ get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > > set_inode_flag(inode, FI_NEED_IPU); > >+ > > ret = file_write_and_wait_range(file, start, end); > > clear_inode_flag(inode, FI_NEED_IPU); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-11-02 12:25 ` qixiaoyu @ 2022-11-06 13:54 ` Chao Yu -1 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-06 13:54 UTC (permalink / raw) To: qixiaoyu; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel On 2022/11/2 20:25, qixiaoyu wrote: > Hi Chao, > > fdatasync do in-place-update to avoid additional node writes, but currently > it only do that with F2FS_IPU_FSYNC as: > > f2fs_do_sync_file: > if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > set_inode_flag(inode, FI_NEED_IPU); > > check_inplace_update_policy: > /* this is only set during fdatasync */ > if (policy & (0x1 << F2FS_IPU_FSYNC) && > is_inode_flag_set(inode, FI_NEED_IPU)) > return true; > > So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to > apply it to all IPU policy. > > BTW, we found small performance improvement with this patch on AndroBench app > using F2FS_IPU_SSR_UTIL on our product: How this patch affects performance when F2FS_IPU_SSR_UTIL is on? Thanks, > > F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) > SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 > SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 > SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 > > Thanks > > On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: >> On 2022/10/21 10:31, qixiaoyu1 wrote: >>> Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. >>> Fix to apply it to all IPU policy. >> >> Xiaoyu, >> >> Sorry for the delay. >> >> I didn't get the point, can you please explain more about the >> issue? >> >> Thanks, >> >>> >>> Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> >>> --- >>> fs/f2fs/data.c | 8 +++----- >>> fs/f2fs/file.c | 4 +++- >>> 2 files changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index a71e818cd67b..fec8e15fe820 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && >>> is_inode_flag_set(inode, FI_OPU_WRITE)) >>> return false; >>> + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ >>> + if (is_inode_flag_set(inode, FI_NEED_IPU)) >>> + return true; >>> if (policy & (0x1 << F2FS_IPU_FORCE)) >>> return true; >>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) >>> @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>> !IS_ENCRYPTED(inode)) >>> return true; >>> - /* this is only set during fdatasync */ >>> - if (policy & (0x1 << F2FS_IPU_FSYNC) && >>> - is_inode_flag_set(inode, FI_NEED_IPU)) >>> - return true; >>> - >>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) >>> return true; >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index 82cda1258227..08091550cdf2 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, >>> goto go_write; >>> /* if fdatasync is triggered, let's do in-place-update */ >>> - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>> + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && >>> + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) >>> set_inode_flag(inode, FI_NEED_IPU); >>> + >>> ret = file_write_and_wait_range(file, start, end); >>> clear_inode_flag(inode, FI_NEED_IPU); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-06 13:54 ` Chao Yu 0 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-06 13:54 UTC (permalink / raw) To: qixiaoyu; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel On 2022/11/2 20:25, qixiaoyu wrote: > Hi Chao, > > fdatasync do in-place-update to avoid additional node writes, but currently > it only do that with F2FS_IPU_FSYNC as: > > f2fs_do_sync_file: > if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > set_inode_flag(inode, FI_NEED_IPU); > > check_inplace_update_policy: > /* this is only set during fdatasync */ > if (policy & (0x1 << F2FS_IPU_FSYNC) && > is_inode_flag_set(inode, FI_NEED_IPU)) > return true; > > So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to > apply it to all IPU policy. > > BTW, we found small performance improvement with this patch on AndroBench app > using F2FS_IPU_SSR_UTIL on our product: How this patch affects performance when F2FS_IPU_SSR_UTIL is on? Thanks, > > F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) > SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 > SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 > SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 > > Thanks > > On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: >> On 2022/10/21 10:31, qixiaoyu1 wrote: >>> Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. >>> Fix to apply it to all IPU policy. >> >> Xiaoyu, >> >> Sorry for the delay. >> >> I didn't get the point, can you please explain more about the >> issue? >> >> Thanks, >> >>> >>> Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> >>> --- >>> fs/f2fs/data.c | 8 +++----- >>> fs/f2fs/file.c | 4 +++- >>> 2 files changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index a71e818cd67b..fec8e15fe820 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && >>> is_inode_flag_set(inode, FI_OPU_WRITE)) >>> return false; >>> + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ >>> + if (is_inode_flag_set(inode, FI_NEED_IPU)) >>> + return true; >>> if (policy & (0x1 << F2FS_IPU_FORCE)) >>> return true; >>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) >>> @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>> !IS_ENCRYPTED(inode)) >>> return true; >>> - /* this is only set during fdatasync */ >>> - if (policy & (0x1 << F2FS_IPU_FSYNC) && >>> - is_inode_flag_set(inode, FI_NEED_IPU)) >>> - return true; >>> - >>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) >>> return true; >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index 82cda1258227..08091550cdf2 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, >>> goto go_write; >>> /* if fdatasync is triggered, let's do in-place-update */ >>> - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>> + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && >>> + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) >>> set_inode_flag(inode, FI_NEED_IPU); >>> + >>> ret = file_write_and_wait_range(file, start, end); >>> clear_inode_flag(inode, FI_NEED_IPU); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-11-06 13:54 ` Chao Yu @ 2022-11-08 12:32 ` qixiaoyu -1 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-11-08 12:32 UTC (permalink / raw) To: Chao Yu; +Cc: Jaegeuk Kim, qixiaoyu1, linux-kernel, linux-f2fs-devel On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: > On 2022/11/2 20:25, qixiaoyu wrote: > >Hi Chao, > > > >fdatasync do in-place-update to avoid additional node writes, but currently > >it only do that with F2FS_IPU_FSYNC as: > > > >f2fs_do_sync_file: > > if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > > set_inode_flag(inode, FI_NEED_IPU); > > > >check_inplace_update_policy: > > /* this is only set during fdatasync */ > > if (policy & (0x1 << F2FS_IPU_FSYNC) && > > is_inode_flag_set(inode, FI_NEED_IPU)) > > return true; > > > >So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to > >apply it to all IPU policy. > > > >BTW, we found small performance improvement with this patch on AndroBench app > >using F2FS_IPU_SSR_UTIL on our product: > > How this patch affects performance when F2FS_IPU_SSR_UTIL is on? > > Thanks, > SQLite test in AndroBench app use fdatasync to sync file to the disk. When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update even though SQLite calls fdatasync, which will introduce extra meta data write. Thanks. > > > > F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) > >SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 > >SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 > >SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 > > > >Thanks > > > >On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: > >>On 2022/10/21 10:31, qixiaoyu1 wrote: > >>>Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > >>>Fix to apply it to all IPU policy. > >> > >>Xiaoyu, > >> > >>Sorry for the delay. > >> > >>I didn't get the point, can you please explain more about the > >>issue? > >> > >>Thanks, > >> > >>> > >>>Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > >>>--- > >>> fs/f2fs/data.c | 8 +++----- > >>> fs/f2fs/file.c | 4 +++- > >>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>> > >>>diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>>index a71e818cd67b..fec8e15fe820 100644 > >>>--- a/fs/f2fs/data.c > >>>+++ b/fs/f2fs/data.c > >>>@@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > >>> is_inode_flag_set(inode, FI_OPU_WRITE)) > >>> return false; > >>>+ /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > >>>+ if (is_inode_flag_set(inode, FI_NEED_IPU)) > >>>+ return true; > >>> if (policy & (0x1 << F2FS_IPU_FORCE)) > >>> return true; > >>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > >>>@@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>> !IS_ENCRYPTED(inode)) > >>> return true; > >>>- /* this is only set during fdatasync */ > >>>- if (policy & (0x1 << F2FS_IPU_FSYNC) && > >>>- is_inode_flag_set(inode, FI_NEED_IPU)) > >>>- return true; > >>>- > >>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > >>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > >>> return true; > >>>diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>index 82cda1258227..08091550cdf2 100644 > >>>--- a/fs/f2fs/file.c > >>>+++ b/fs/f2fs/file.c > >>>@@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > >>> goto go_write; > >>> /* if fdatasync is triggered, let's do in-place-update */ > >>>- if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >>>+ if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > >>>+ get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > >>> set_inode_flag(inode, FI_NEED_IPU); > >>>+ > >>> ret = file_write_and_wait_range(file, start, end); > >>> clear_inode_flag(inode, FI_NEED_IPU); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-08 12:32 ` qixiaoyu 0 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-11-08 12:32 UTC (permalink / raw) To: Chao Yu; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, qixiaoyu1 On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: > On 2022/11/2 20:25, qixiaoyu wrote: > >Hi Chao, > > > >fdatasync do in-place-update to avoid additional node writes, but currently > >it only do that with F2FS_IPU_FSYNC as: > > > >f2fs_do_sync_file: > > if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > > set_inode_flag(inode, FI_NEED_IPU); > > > >check_inplace_update_policy: > > /* this is only set during fdatasync */ > > if (policy & (0x1 << F2FS_IPU_FSYNC) && > > is_inode_flag_set(inode, FI_NEED_IPU)) > > return true; > > > >So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to > >apply it to all IPU policy. > > > >BTW, we found small performance improvement with this patch on AndroBench app > >using F2FS_IPU_SSR_UTIL on our product: > > How this patch affects performance when F2FS_IPU_SSR_UTIL is on? > > Thanks, > SQLite test in AndroBench app use fdatasync to sync file to the disk. When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update even though SQLite calls fdatasync, which will introduce extra meta data write. Thanks. > > > > F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) > >SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 > >SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 > >SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 > > > >Thanks > > > >On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: > >>On 2022/10/21 10:31, qixiaoyu1 wrote: > >>>Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > >>>Fix to apply it to all IPU policy. > >> > >>Xiaoyu, > >> > >>Sorry for the delay. > >> > >>I didn't get the point, can you please explain more about the > >>issue? > >> > >>Thanks, > >> > >>> > >>>Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > >>>--- > >>> fs/f2fs/data.c | 8 +++----- > >>> fs/f2fs/file.c | 4 +++- > >>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>> > >>>diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>>index a71e818cd67b..fec8e15fe820 100644 > >>>--- a/fs/f2fs/data.c > >>>+++ b/fs/f2fs/data.c > >>>@@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > >>> is_inode_flag_set(inode, FI_OPU_WRITE)) > >>> return false; > >>>+ /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > >>>+ if (is_inode_flag_set(inode, FI_NEED_IPU)) > >>>+ return true; > >>> if (policy & (0x1 << F2FS_IPU_FORCE)) > >>> return true; > >>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > >>>@@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>> !IS_ENCRYPTED(inode)) > >>> return true; > >>>- /* this is only set during fdatasync */ > >>>- if (policy & (0x1 << F2FS_IPU_FSYNC) && > >>>- is_inode_flag_set(inode, FI_NEED_IPU)) > >>>- return true; > >>>- > >>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > >>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > >>> return true; > >>>diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>index 82cda1258227..08091550cdf2 100644 > >>>--- a/fs/f2fs/file.c > >>>+++ b/fs/f2fs/file.c > >>>@@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > >>> goto go_write; > >>> /* if fdatasync is triggered, let's do in-place-update */ > >>>- if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >>>+ if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > >>>+ get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > >>> set_inode_flag(inode, FI_NEED_IPU); > >>>+ > >>> ret = file_write_and_wait_range(file, start, end); > >>> clear_inode_flag(inode, FI_NEED_IPU); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-11-08 12:32 ` qixiaoyu @ 2022-11-08 14:30 ` Chao Yu -1 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-08 14:30 UTC (permalink / raw) To: qixiaoyu; +Cc: Jaegeuk Kim, qixiaoyu1, linux-kernel, linux-f2fs-devel On 2022/11/8 20:32, qixiaoyu wrote: > On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: >> On 2022/11/2 20:25, qixiaoyu wrote: >>> Hi Chao, >>> >>> fdatasync do in-place-update to avoid additional node writes, but currently >>> it only do that with F2FS_IPU_FSYNC as: >>> >>> f2fs_do_sync_file: >>> if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>> set_inode_flag(inode, FI_NEED_IPU); >>> >>> check_inplace_update_policy: >>> /* this is only set during fdatasync */ >>> if (policy & (0x1 << F2FS_IPU_FSYNC) && >>> is_inode_flag_set(inode, FI_NEED_IPU)) >>> return true; >>> >>> So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to >>> apply it to all IPU policy. >>> >>> BTW, we found small performance improvement with this patch on AndroBench app >>> using F2FS_IPU_SSR_UTIL on our product: >> >> How this patch affects performance when F2FS_IPU_SSR_UTIL is on? >> >> Thanks, >> > > SQLite test in AndroBench app use fdatasync to sync file to the disk. > When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update > even though SQLite calls fdatasync, which will introduce extra meta data write. Why not using F2FS_IPU_SSR_UTIL | F2FS_IPU_FSYNC, I guess these two flags cover different scenarios, F2FS_IPU_SSR_UTIL for ssr case, and F2FS_IPU_FSYNC for f{data,}sync case. Thanks, > > Thanks. > >>> >>> F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) >>> SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 >>> SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 >>> SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 >>> >>> Thanks >>> >>> On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: >>>> On 2022/10/21 10:31, qixiaoyu1 wrote: >>>>> Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. >>>>> Fix to apply it to all IPU policy. >>>> >>>> Xiaoyu, >>>> >>>> Sorry for the delay. >>>> >>>> I didn't get the point, can you please explain more about the >>>> issue? >>>> >>>> Thanks, >>>> >>>>> >>>>> Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> >>>>> --- >>>>> fs/f2fs/data.c | 8 +++----- >>>>> fs/f2fs/file.c | 4 +++- >>>>> 2 files changed, 6 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>>> index a71e818cd67b..fec8e15fe820 100644 >>>>> --- a/fs/f2fs/data.c >>>>> +++ b/fs/f2fs/data.c >>>>> @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && >>>>> is_inode_flag_set(inode, FI_OPU_WRITE)) >>>>> return false; >>>>> + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ >>>>> + if (is_inode_flag_set(inode, FI_NEED_IPU)) >>>>> + return true; >>>>> if (policy & (0x1 << F2FS_IPU_FORCE)) >>>>> return true; >>>>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) >>>>> @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>> !IS_ENCRYPTED(inode)) >>>>> return true; >>>>> - /* this is only set during fdatasync */ >>>>> - if (policy & (0x1 << F2FS_IPU_FSYNC) && >>>>> - is_inode_flag_set(inode, FI_NEED_IPU)) >>>>> - return true; >>>>> - >>>>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>>>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) >>>>> return true; >>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>> index 82cda1258227..08091550cdf2 100644 >>>>> --- a/fs/f2fs/file.c >>>>> +++ b/fs/f2fs/file.c >>>>> @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, >>>>> goto go_write; >>>>> /* if fdatasync is triggered, let's do in-place-update */ >>>>> - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>>>> + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && >>>>> + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) >>>>> set_inode_flag(inode, FI_NEED_IPU); >>>>> + >>>>> ret = file_write_and_wait_range(file, start, end); >>>>> clear_inode_flag(inode, FI_NEED_IPU); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-08 14:30 ` Chao Yu 0 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-08 14:30 UTC (permalink / raw) To: qixiaoyu; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, qixiaoyu1 On 2022/11/8 20:32, qixiaoyu wrote: > On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: >> On 2022/11/2 20:25, qixiaoyu wrote: >>> Hi Chao, >>> >>> fdatasync do in-place-update to avoid additional node writes, but currently >>> it only do that with F2FS_IPU_FSYNC as: >>> >>> f2fs_do_sync_file: >>> if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>> set_inode_flag(inode, FI_NEED_IPU); >>> >>> check_inplace_update_policy: >>> /* this is only set during fdatasync */ >>> if (policy & (0x1 << F2FS_IPU_FSYNC) && >>> is_inode_flag_set(inode, FI_NEED_IPU)) >>> return true; >>> >>> So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to >>> apply it to all IPU policy. >>> >>> BTW, we found small performance improvement with this patch on AndroBench app >>> using F2FS_IPU_SSR_UTIL on our product: >> >> How this patch affects performance when F2FS_IPU_SSR_UTIL is on? >> >> Thanks, >> > > SQLite test in AndroBench app use fdatasync to sync file to the disk. > When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update > even though SQLite calls fdatasync, which will introduce extra meta data write. Why not using F2FS_IPU_SSR_UTIL | F2FS_IPU_FSYNC, I guess these two flags cover different scenarios, F2FS_IPU_SSR_UTIL for ssr case, and F2FS_IPU_FSYNC for f{data,}sync case. Thanks, > > Thanks. > >>> >>> F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) >>> SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 >>> SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 >>> SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 >>> >>> Thanks >>> >>> On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: >>>> On 2022/10/21 10:31, qixiaoyu1 wrote: >>>>> Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. >>>>> Fix to apply it to all IPU policy. >>>> >>>> Xiaoyu, >>>> >>>> Sorry for the delay. >>>> >>>> I didn't get the point, can you please explain more about the >>>> issue? >>>> >>>> Thanks, >>>> >>>>> >>>>> Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> >>>>> --- >>>>> fs/f2fs/data.c | 8 +++----- >>>>> fs/f2fs/file.c | 4 +++- >>>>> 2 files changed, 6 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>>> index a71e818cd67b..fec8e15fe820 100644 >>>>> --- a/fs/f2fs/data.c >>>>> +++ b/fs/f2fs/data.c >>>>> @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && >>>>> is_inode_flag_set(inode, FI_OPU_WRITE)) >>>>> return false; >>>>> + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ >>>>> + if (is_inode_flag_set(inode, FI_NEED_IPU)) >>>>> + return true; >>>>> if (policy & (0x1 << F2FS_IPU_FORCE)) >>>>> return true; >>>>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) >>>>> @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>> !IS_ENCRYPTED(inode)) >>>>> return true; >>>>> - /* this is only set during fdatasync */ >>>>> - if (policy & (0x1 << F2FS_IPU_FSYNC) && >>>>> - is_inode_flag_set(inode, FI_NEED_IPU)) >>>>> - return true; >>>>> - >>>>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>>>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) >>>>> return true; >>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>> index 82cda1258227..08091550cdf2 100644 >>>>> --- a/fs/f2fs/file.c >>>>> +++ b/fs/f2fs/file.c >>>>> @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, >>>>> goto go_write; >>>>> /* if fdatasync is triggered, let's do in-place-update */ >>>>> - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>>>> + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && >>>>> + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) >>>>> set_inode_flag(inode, FI_NEED_IPU); >>>>> + >>>>> ret = file_write_and_wait_range(file, start, end); >>>>> clear_inode_flag(inode, FI_NEED_IPU); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-11-08 14:30 ` Chao Yu @ 2022-11-09 12:56 ` qixiaoyu -1 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-11-09 12:56 UTC (permalink / raw) To: Chao Yu; +Cc: Jaegeuk Kim, qixiaoyu1, linux-kernel, linux-f2fs-devel On Tue, Nov 08, 2022 at 10:30:13PM +0800, Chao Yu wrote: > On 2022/11/8 20:32, qixiaoyu wrote: > >On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: > >>On 2022/11/2 20:25, qixiaoyu wrote: > >>>Hi Chao, > >>> > >>>fdatasync do in-place-update to avoid additional node writes, but currently > >>>it only do that with F2FS_IPU_FSYNC as: > >>> > >>>f2fs_do_sync_file: > >>> if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >>> set_inode_flag(inode, FI_NEED_IPU); > >>> > >>>check_inplace_update_policy: > >>> /* this is only set during fdatasync */ > >>> if (policy & (0x1 << F2FS_IPU_FSYNC) && > >>> is_inode_flag_set(inode, FI_NEED_IPU)) > >>> return true; > >>> > >>>So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to > >>>apply it to all IPU policy. > >>> > >>>BTW, we found small performance improvement with this patch on AndroBench app > >>>using F2FS_IPU_SSR_UTIL on our product: > >> > >>How this patch affects performance when F2FS_IPU_SSR_UTIL is on? > >> > >>Thanks, > >> > > > >SQLite test in AndroBench app use fdatasync to sync file to the disk. > >When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update > >even though SQLite calls fdatasync, which will introduce extra meta data write. > > Why not using F2FS_IPU_SSR_UTIL | F2FS_IPU_FSYNC, I guess these two flags > cover different scenarios, F2FS_IPU_SSR_UTIL for ssr case, and F2FS_IPU_FSYNC > for f{data,}sync case. > > Thanks, > As fsync(2) says: fdatasync() is similar to fsync(), but does not flush modified metadata unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled. I think fdatasync should try to perform in-place-update to avoid unnecessary metadata update whatever the ipu_policy is, and F2FS_IPU_FSYNC is used for fsync independently. Thanks > > > >Thanks. > > > >>> > >>> F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) > >>>SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 > >>>SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 > >>>SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 > >>> > >>>Thanks > >>> > >>>On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: > >>>>On 2022/10/21 10:31, qixiaoyu1 wrote: > >>>>>Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > >>>>>Fix to apply it to all IPU policy. > >>>> > >>>>Xiaoyu, > >>>> > >>>>Sorry for the delay. > >>>> > >>>>I didn't get the point, can you please explain more about the > >>>>issue? > >>>> > >>>>Thanks, > >>>> > >>>>> > >>>>>Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > >>>>>--- > >>>>> fs/f2fs/data.c | 8 +++----- > >>>>> fs/f2fs/file.c | 4 +++- > >>>>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>>>> > >>>>>diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>>>>index a71e818cd67b..fec8e15fe820 100644 > >>>>>--- a/fs/f2fs/data.c > >>>>>+++ b/fs/f2fs/data.c > >>>>>@@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>>>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > >>>>> is_inode_flag_set(inode, FI_OPU_WRITE)) > >>>>> return false; > >>>>>+ /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > >>>>>+ if (is_inode_flag_set(inode, FI_NEED_IPU)) > >>>>>+ return true; > >>>>> if (policy & (0x1 << F2FS_IPU_FORCE)) > >>>>> return true; > >>>>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > >>>>>@@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>>>> !IS_ENCRYPTED(inode)) > >>>>> return true; > >>>>>- /* this is only set during fdatasync */ > >>>>>- if (policy & (0x1 << F2FS_IPU_FSYNC) && > >>>>>- is_inode_flag_set(inode, FI_NEED_IPU)) > >>>>>- return true; > >>>>>- > >>>>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > >>>>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > >>>>> return true; > >>>>>diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>>>index 82cda1258227..08091550cdf2 100644 > >>>>>--- a/fs/f2fs/file.c > >>>>>+++ b/fs/f2fs/file.c > >>>>>@@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > >>>>> goto go_write; > >>>>> /* if fdatasync is triggered, let's do in-place-update */ > >>>>>- if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >>>>>+ if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > >>>>>+ get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > >>>>> set_inode_flag(inode, FI_NEED_IPU); > >>>>>+ > >>>>> ret = file_write_and_wait_range(file, start, end); > >>>>> clear_inode_flag(inode, FI_NEED_IPU); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-09 12:56 ` qixiaoyu 0 siblings, 0 replies; 18+ messages in thread From: qixiaoyu @ 2022-11-09 12:56 UTC (permalink / raw) To: Chao Yu; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, qixiaoyu1 On Tue, Nov 08, 2022 at 10:30:13PM +0800, Chao Yu wrote: > On 2022/11/8 20:32, qixiaoyu wrote: > >On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: > >>On 2022/11/2 20:25, qixiaoyu wrote: > >>>Hi Chao, > >>> > >>>fdatasync do in-place-update to avoid additional node writes, but currently > >>>it only do that with F2FS_IPU_FSYNC as: > >>> > >>>f2fs_do_sync_file: > >>> if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >>> set_inode_flag(inode, FI_NEED_IPU); > >>> > >>>check_inplace_update_policy: > >>> /* this is only set during fdatasync */ > >>> if (policy & (0x1 << F2FS_IPU_FSYNC) && > >>> is_inode_flag_set(inode, FI_NEED_IPU)) > >>> return true; > >>> > >>>So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to > >>>apply it to all IPU policy. > >>> > >>>BTW, we found small performance improvement with this patch on AndroBench app > >>>using F2FS_IPU_SSR_UTIL on our product: > >> > >>How this patch affects performance when F2FS_IPU_SSR_UTIL is on? > >> > >>Thanks, > >> > > > >SQLite test in AndroBench app use fdatasync to sync file to the disk. > >When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update > >even though SQLite calls fdatasync, which will introduce extra meta data write. > > Why not using F2FS_IPU_SSR_UTIL | F2FS_IPU_FSYNC, I guess these two flags > cover different scenarios, F2FS_IPU_SSR_UTIL for ssr case, and F2FS_IPU_FSYNC > for f{data,}sync case. > > Thanks, > As fsync(2) says: fdatasync() is similar to fsync(), but does not flush modified metadata unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled. I think fdatasync should try to perform in-place-update to avoid unnecessary metadata update whatever the ipu_policy is, and F2FS_IPU_FSYNC is used for fsync independently. Thanks > > > >Thanks. > > > >>> > >>> F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) > >>>SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 > >>>SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 > >>>SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 > >>> > >>>Thanks > >>> > >>>On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: > >>>>On 2022/10/21 10:31, qixiaoyu1 wrote: > >>>>>Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. > >>>>>Fix to apply it to all IPU policy. > >>>> > >>>>Xiaoyu, > >>>> > >>>>Sorry for the delay. > >>>> > >>>>I didn't get the point, can you please explain more about the > >>>>issue? > >>>> > >>>>Thanks, > >>>> > >>>>> > >>>>>Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> > >>>>>--- > >>>>> fs/f2fs/data.c | 8 +++----- > >>>>> fs/f2fs/file.c | 4 +++- > >>>>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>>>> > >>>>>diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>>>>index a71e818cd67b..fec8e15fe820 100644 > >>>>>--- a/fs/f2fs/data.c > >>>>>+++ b/fs/f2fs/data.c > >>>>>@@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>>>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && > >>>>> is_inode_flag_set(inode, FI_OPU_WRITE)) > >>>>> return false; > >>>>>+ /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ > >>>>>+ if (is_inode_flag_set(inode, FI_NEED_IPU)) > >>>>>+ return true; > >>>>> if (policy & (0x1 << F2FS_IPU_FORCE)) > >>>>> return true; > >>>>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) > >>>>>@@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, > >>>>> !IS_ENCRYPTED(inode)) > >>>>> return true; > >>>>>- /* this is only set during fdatasync */ > >>>>>- if (policy & (0x1 << F2FS_IPU_FSYNC) && > >>>>>- is_inode_flag_set(inode, FI_NEED_IPU)) > >>>>>- return true; > >>>>>- > >>>>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && > >>>>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) > >>>>> return true; > >>>>>diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>>>index 82cda1258227..08091550cdf2 100644 > >>>>>--- a/fs/f2fs/file.c > >>>>>+++ b/fs/f2fs/file.c > >>>>>@@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > >>>>> goto go_write; > >>>>> /* if fdatasync is triggered, let's do in-place-update */ > >>>>>- if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) > >>>>>+ if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && > >>>>>+ get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) > >>>>> set_inode_flag(inode, FI_NEED_IPU); > >>>>>+ > >>>>> ret = file_write_and_wait_range(file, start, end); > >>>>> clear_inode_flag(inode, FI_NEED_IPU); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC 2022-11-09 12:56 ` qixiaoyu @ 2022-11-09 13:39 ` Chao Yu -1 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-09 13:39 UTC (permalink / raw) To: qixiaoyu, Jaegeuk Kim; +Cc: qixiaoyu1, linux-kernel, linux-f2fs-devel On 2022/11/9 20:56, qixiaoyu wrote: > On Tue, Nov 08, 2022 at 10:30:13PM +0800, Chao Yu wrote: >> On 2022/11/8 20:32, qixiaoyu wrote: >>> On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: >>>> On 2022/11/2 20:25, qixiaoyu wrote: >>>>> Hi Chao, >>>>> >>>>> fdatasync do in-place-update to avoid additional node writes, but currently >>>>> it only do that with F2FS_IPU_FSYNC as: >>>>> >>>>> f2fs_do_sync_file: >>>>> if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>>>> set_inode_flag(inode, FI_NEED_IPU); >>>>> >>>>> check_inplace_update_policy: >>>>> /* this is only set during fdatasync */ >>>>> if (policy & (0x1 << F2FS_IPU_FSYNC) && >>>>> is_inode_flag_set(inode, FI_NEED_IPU)) >>>>> return true; >>>>> >>>>> So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to >>>>> apply it to all IPU policy. >>>>> >>>>> BTW, we found small performance improvement with this patch on AndroBench app >>>>> using F2FS_IPU_SSR_UTIL on our product: >>>> >>>> How this patch affects performance when F2FS_IPU_SSR_UTIL is on? >>>> >>>> Thanks, >>>> >>> >>> SQLite test in AndroBench app use fdatasync to sync file to the disk. >>> When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update >>> even though SQLite calls fdatasync, which will introduce extra meta data write. >> >> Why not using F2FS_IPU_SSR_UTIL | F2FS_IPU_FSYNC, I guess these two flags >> cover different scenarios, F2FS_IPU_SSR_UTIL for ssr case, and F2FS_IPU_FSYNC >> for f{data,}sync case. >> >> Thanks, >> > > As fsync(2) says: > fdatasync() is similar to fsync(), but does not flush modified metadata unless that > metadata is needed in order to allow a subsequent data retrieval to be correctly handled. I guess it says it allows fdatasync to flush metatdata in order to recovery data in SPO case. > > I think fdatasync should try to perform in-place-update to avoid unnecessary metadata > update whatever the ipu_policy is, and F2FS_IPU_FSYNC is used for fsync independently. IMO, FSYNC key word in F2FS_IPU_FSYNC means fsync path or interface name as below: int (*fsync) (struct file *, loff_t, loff_t, int datasync); And by default, f2fs enables F2FS_IPU_FSYNC, I didn't get why we need to disable it. To Jaegeuk, any comments? Thanks, > > Thanks > >>> >>> Thanks. >>> >>>>> >>>>> F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) >>>>> SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 >>>>> SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 >>>>> SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 >>>>> >>>>> Thanks >>>>> >>>>> On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: >>>>>> On 2022/10/21 10:31, qixiaoyu1 wrote: >>>>>>> Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. >>>>>>> Fix to apply it to all IPU policy. >>>>>> >>>>>> Xiaoyu, >>>>>> >>>>>> Sorry for the delay. >>>>>> >>>>>> I didn't get the point, can you please explain more about the >>>>>> issue? >>>>>> >>>>>> Thanks, >>>>>> >>>>>>> >>>>>>> Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> >>>>>>> --- >>>>>>> fs/f2fs/data.c | 8 +++----- >>>>>>> fs/f2fs/file.c | 4 +++- >>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>>>>> index a71e818cd67b..fec8e15fe820 100644 >>>>>>> --- a/fs/f2fs/data.c >>>>>>> +++ b/fs/f2fs/data.c >>>>>>> @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>>>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && >>>>>>> is_inode_flag_set(inode, FI_OPU_WRITE)) >>>>>>> return false; >>>>>>> + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ >>>>>>> + if (is_inode_flag_set(inode, FI_NEED_IPU)) >>>>>>> + return true; >>>>>>> if (policy & (0x1 << F2FS_IPU_FORCE)) >>>>>>> return true; >>>>>>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) >>>>>>> @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>>>> !IS_ENCRYPTED(inode)) >>>>>>> return true; >>>>>>> - /* this is only set during fdatasync */ >>>>>>> - if (policy & (0x1 << F2FS_IPU_FSYNC) && >>>>>>> - is_inode_flag_set(inode, FI_NEED_IPU)) >>>>>>> - return true; >>>>>>> - >>>>>>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>>>>>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) >>>>>>> return true; >>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>>>> index 82cda1258227..08091550cdf2 100644 >>>>>>> --- a/fs/f2fs/file.c >>>>>>> +++ b/fs/f2fs/file.c >>>>>>> @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, >>>>>>> goto go_write; >>>>>>> /* if fdatasync is triggered, let's do in-place-update */ >>>>>>> - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>>>>>> + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && >>>>>>> + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) >>>>>>> set_inode_flag(inode, FI_NEED_IPU); >>>>>>> + >>>>>>> ret = file_write_and_wait_range(file, start, end); >>>>>>> clear_inode_flag(inode, FI_NEED_IPU); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC @ 2022-11-09 13:39 ` Chao Yu 0 siblings, 0 replies; 18+ messages in thread From: Chao Yu @ 2022-11-09 13:39 UTC (permalink / raw) To: qixiaoyu, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel, qixiaoyu1 On 2022/11/9 20:56, qixiaoyu wrote: > On Tue, Nov 08, 2022 at 10:30:13PM +0800, Chao Yu wrote: >> On 2022/11/8 20:32, qixiaoyu wrote: >>> On Sun, Nov 06, 2022 at 09:54:59PM +0800, Chao Yu wrote: >>>> On 2022/11/2 20:25, qixiaoyu wrote: >>>>> Hi Chao, >>>>> >>>>> fdatasync do in-place-update to avoid additional node writes, but currently >>>>> it only do that with F2FS_IPU_FSYNC as: >>>>> >>>>> f2fs_do_sync_file: >>>>> if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>>>> set_inode_flag(inode, FI_NEED_IPU); >>>>> >>>>> check_inplace_update_policy: >>>>> /* this is only set during fdatasync */ >>>>> if (policy & (0x1 << F2FS_IPU_FSYNC) && >>>>> is_inode_flag_set(inode, FI_NEED_IPU)) >>>>> return true; >>>>> >>>>> So this patch separate in-place-update of fdatasync from F2FS_IPU_FSYNC to >>>>> apply it to all IPU policy. >>>>> >>>>> BTW, we found small performance improvement with this patch on AndroBench app >>>>> using F2FS_IPU_SSR_UTIL on our product: >>>> >>>> How this patch affects performance when F2FS_IPU_SSR_UTIL is on? >>>> >>>> Thanks, >>>> >>> >>> SQLite test in AndroBench app use fdatasync to sync file to the disk. >>> When switch to F2FS_IPU_SSR_UTIL ipu_policy, it will use out-of-place-update >>> even though SQLite calls fdatasync, which will introduce extra meta data write. >> >> Why not using F2FS_IPU_SSR_UTIL | F2FS_IPU_FSYNC, I guess these two flags >> cover different scenarios, F2FS_IPU_SSR_UTIL for ssr case, and F2FS_IPU_FSYNC >> for f{data,}sync case. >> >> Thanks, >> > > As fsync(2) says: > fdatasync() is similar to fsync(), but does not flush modified metadata unless that > metadata is needed in order to allow a subsequent data retrieval to be correctly handled. I guess it says it allows fdatasync to flush metatdata in order to recovery data in SPO case. > > I think fdatasync should try to perform in-place-update to avoid unnecessary metadata > update whatever the ipu_policy is, and F2FS_IPU_FSYNC is used for fsync independently. IMO, FSYNC key word in F2FS_IPU_FSYNC means fsync path or interface name as below: int (*fsync) (struct file *, loff_t, loff_t, int datasync); And by default, f2fs enables F2FS_IPU_FSYNC, I didn't get why we need to disable it. To Jaegeuk, any comments? Thanks, > > Thanks > >>> >>> Thanks. >>> >>>>> >>>>> F2FS_IPU_FSYNC F2FS_IPU_SSR_UTIL F2FS_IPU_SSR_UTIL(with patch) >>>>> SQLite Insert(QPS) 6818.08 6327.09(-7.20%) 6757.72 >>>>> SQLite Update(QPS) 6528.81 6336.57(-2.94%) 6490.77 >>>>> SQLite Delete(QPS) 9724.68 9378.37(-3.56%) 9622.27 >>>>> >>>>> Thanks >>>>> >>>>> On Tue, Nov 01, 2022 at 11:14:55PM +0800, Chao Yu wrote: >>>>>> On 2022/10/21 10:31, qixiaoyu1 wrote: >>>>>>> Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC. >>>>>>> Fix to apply it to all IPU policy. >>>>>> >>>>>> Xiaoyu, >>>>>> >>>>>> Sorry for the delay. >>>>>> >>>>>> I didn't get the point, can you please explain more about the >>>>>> issue? >>>>>> >>>>>> Thanks, >>>>>> >>>>>>> >>>>>>> Signed-off-by: qixiaoyu1 <qixiaoyu1@xiaomi.com> >>>>>>> --- >>>>>>> fs/f2fs/data.c | 8 +++----- >>>>>>> fs/f2fs/file.c | 4 +++- >>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>>>>> index a71e818cd67b..fec8e15fe820 100644 >>>>>>> --- a/fs/f2fs/data.c >>>>>>> +++ b/fs/f2fs/data.c >>>>>>> @@ -2518,6 +2518,9 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>>>> if (policy & (0x1 << F2FS_IPU_HONOR_OPU_WRITE) && >>>>>>> is_inode_flag_set(inode, FI_OPU_WRITE)) >>>>>>> return false; >>>>>>> + /* this is set by fdatasync or F2FS_IPU_FSYNC policy */ >>>>>>> + if (is_inode_flag_set(inode, FI_NEED_IPU)) >>>>>>> + return true; >>>>>>> if (policy & (0x1 << F2FS_IPU_FORCE)) >>>>>>> return true; >>>>>>> if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi)) >>>>>>> @@ -2538,11 +2541,6 @@ static inline bool check_inplace_update_policy(struct inode *inode, >>>>>>> !IS_ENCRYPTED(inode)) >>>>>>> return true; >>>>>>> - /* this is only set during fdatasync */ >>>>>>> - if (policy & (0x1 << F2FS_IPU_FSYNC) && >>>>>>> - is_inode_flag_set(inode, FI_NEED_IPU)) >>>>>>> - return true; >>>>>>> - >>>>>>> if (unlikely(fio && is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>>>>>> !f2fs_is_checkpointed_data(sbi, fio->old_blkaddr))) >>>>>>> return true; >>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>>>> index 82cda1258227..08091550cdf2 100644 >>>>>>> --- a/fs/f2fs/file.c >>>>>>> +++ b/fs/f2fs/file.c >>>>>>> @@ -270,8 +270,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, >>>>>>> goto go_write; >>>>>>> /* if fdatasync is triggered, let's do in-place-update */ >>>>>>> - if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks) >>>>>>> + if (datasync || (SM_I(sbi)->ipu_policy & (0x1 << F2FS_IPU_FSYNC) && >>>>>>> + get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)) >>>>>>> set_inode_flag(inode, FI_NEED_IPU); >>>>>>> + >>>>>>> ret = file_write_and_wait_range(file, start, end); >>>>>>> clear_inode_flag(inode, FI_NEED_IPU); ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-11-09 13:40 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-21 2:31 [f2fs-dev] [PATCH] f2fs: separate IPU policy for fdatasync from F2FS_IPU_FSYNC qixiaoyu1 2022-10-21 2:31 ` qixiaoyu1 2022-10-31 11:27 ` [f2fs-dev] " qixiaoyu 2022-10-31 11:27 ` qixiaoyu 2022-11-01 15:14 ` [f2fs-dev] " Chao Yu 2022-11-01 15:14 ` Chao Yu 2022-11-02 12:25 ` [f2fs-dev] " qixiaoyu 2022-11-02 12:25 ` qixiaoyu 2022-11-06 13:54 ` [f2fs-dev] " Chao Yu 2022-11-06 13:54 ` Chao Yu 2022-11-08 12:32 ` [f2fs-dev] " qixiaoyu 2022-11-08 12:32 ` qixiaoyu 2022-11-08 14:30 ` [f2fs-dev] " Chao Yu 2022-11-08 14:30 ` Chao Yu 2022-11-09 12:56 ` [f2fs-dev] " qixiaoyu 2022-11-09 12:56 ` qixiaoyu 2022-11-09 13:39 ` [f2fs-dev] " Chao Yu 2022-11-09 13:39 ` Chao Yu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.