Currently IPU policy for fdatasync is coupled with F2FS_IPU_FSYNC.
Fix to apply it to all IPU policy.
Signed-off-by: qixiaoyu1 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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);
>
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 <[email protected]>
> >---
> > 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);
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 <[email protected]>
>>> ---
>>> 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);
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 <[email protected]>
> >>>---
> >>> 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);
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 <[email protected]>
>>>>> ---
>>>>> 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);
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 <[email protected]>
> >>>>>---
> >>>>> 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);
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 <[email protected]>
>>>>>>> ---
>>>>>>> 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);