[PATCH] f2fs: fix to avoid forcing direct write to use buffered IO on inline_data inode

[PATCH] f2fs: fix to avoid forcing direct write to use buffered IO on inline_data inode

[Chao Yu]

Jinsu Lee reported a performance regression issue, after commit
5c8764f8679e ("f2fs: fix to force buffered IO on inline_data
inode"), we forced direct write to use buffered IO on inline_data
inode, it will cause performace regression due to memory copy
and data flush.

It's fine to not force direct write to use buffered IO, as it
can convert inline inode before committing direct write IO.

Fixes: 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data inode")
Reported-by: Jinsu Lee <jinsu1.lee@samsung.com>
Closes: https://lore.kernel.org/linux-f2fs-devel/af03dd2c-e361-4f80-b2fd-39440766cf6e@kernel.org
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/file.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 0e7a0195eca8..377a10b81bf3 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -883,7 +883,11 @@ static bool f2fs_force_buffered_io(struct inode *inode, int rw)
 		return true;
 	if (f2fs_compressed_file(inode))
 		return true;
-	if (f2fs_has_inline_data(inode))
+	/*
+	 * only force direct read to use buffered IO, for direct write,
+	 * it expects inline data conversion before committing IO.
+	 */
+	if (f2fs_has_inline_data(inode) && rw == READ)
 		return true;
 
 	/* disallow direct IO if any of devices has unaligned blksize */
-- 
2.40.1

Re: [f2fs-dev] [PATCH] f2fs: fix to avoid forcing direct write to use buffered IO on inline_data inode

[patchwork-bot+f2fs]

Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Mon,  4 Nov 2024 09:50:16 +0800 you wrote:
> Jinsu Lee reported a performance regression issue, after commit
> 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data
> inode"), we forced direct write to use buffered IO on inline_data
> inode, it will cause performace regression due to memory copy
> and data flush.
> 
> It's fine to not force direct write to use buffered IO, as it
> can convert inline inode before committing direct write IO.
> 
> [...]

Here is the summary with links:
  - [f2fs-dev] f2fs: fix to avoid forcing direct write to use buffered IO on inline_data inode
    https://git.kernel.org/jaegeuk/f2fs/c/26e6f59d0bba

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [f2fs-dev] [PATCH] f2fs: fix to avoid forcing direct write to use buffered IO on inline_data inode

[Chao Yu]

On 2024/11/4 10:28, Jinsu Lee wrote:
>> Jinsu Lee reported a performance regression issue, after commit
> 
>> 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data
> 
>> inode"), we forced direct write to use buffered IO on inline_data
> 
>> inode, it will cause performace regression due to memory copy
> 
>> and data flush.
> 
> 
>> It's fine to not force direct write to use buffered IO, as it
> 
>> can convert inline inode before committing direct write IO.
> 
> 
>> Fixes: 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data inode")
> 
>> Reported-by: Jinsu Lee <jinsu1.lee@samsung.com>
> 
>> Closes: https://lore.kernel.org/linux-f2fs-devel/af03dd2c-e361-4f80-b2fd-39440766cf6e@kernel.org
> 
>> Signed-off-by: Chao Yu <chao@kernel.org>
> 
>> ---
> 
>> fs/f2fs/file.c | 6 +++++-
> 
>> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> 
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> 
>> index 0e7a0195eca8..377a10b81bf3 100644
> 
>> --- a/fs/f2fs/file.c
> 
>> +++ b/fs/f2fs/file.c
> 
>> @@ -883,7 +883,11 @@ static bool f2fs_force_buffered_io(struct inode *inode, int rw)
> 
>>                  return true;
> 
>>          if (f2fs_compressed_file(inode))
> 
>>                  return true;
> 
>> -        if (f2fs_has_inline_data(inode))
> 
>> +        /*
> 
>> +         * only force direct read to use buffered IO, for direct write,
> 
>> +         * it expects inline data conversion before committing IO.
> 
>> +         */
> 
>> +        if (f2fs_has_inline_data(inode) && rw == READ)
> 
> 
> Chao Yu,
> 
> The fio direct performance problem I reported did not improve when reflecting this commit.
> 
> Rather, it has been improved when reflecting your commit below.
> 
> 
> The previous commit seems to be mistitled as read and the following commit appears to be the final version.
> 
> The reason for the improvement with the commit below is that there is no "m_may_create" condition
> 
> when performing "io_submit" directly, so performance regression issue may occur.
> 
> 
> I wonder if "rw == READ" should be additionally reflected.

Alright, thanks for your feedback.

I thought you have bisected this performance issue to commit
5c8764f8679e ("f2fs: fix to force buffered IO on inline_data inode"),
so I sent this patch for comments.

Can you please apply both below dio fixes, and help to check final
performance?

https://lore.kernel.org/linux-f2fs-devel/20241104015016.228963-1-chao@kernel.org
https://lore.kernel.org/linux-f2fs-devel/20241104013551.218037-1-chao@kernel.org

Thanks,

> 
> 
> commit 2b6bb0cd3bdcb1108189301ec4ec76c89f939310
> 
> Author: Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
> 
> Date:   Mon Nov 4 09:35:51 2024 +0800
> 
> 
>      [f2fs-dev] [PATCH v2] f2fs: fix to map blocks correctly for direct write
> 
> 
> 

Re: [f2fs-dev] [PATCH] f2fs: fix to avoid forcing direct write to use buffered IO on inline_data inode

[이진수]

>>> Jinsu Lee reported a performance regression issue, after commit
>> 
>>> 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data
>> 
>>> inode"), we forced direct write to use buffered IO on inline_data
>> 
>>> inode, it will cause performace regression due to memory copy
>> 
>>> and data flush.
>> 
>> 
>>> It's fine to not force direct write to use buffered IO, as it
>> 
>>> can convert inline inode before committing direct write IO.
>> 
>> 
>>>> Fixes: 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data inode")
>> 
>>> Reported-by: Jinsu Lee <jinsu1.lee@samsung.com>
>> 
>>> Closes: https://lore.kernel.org/linux-f2fs-devel/af03dd2c-e361-4f80-b2fd-39440766cf6e@kernel.org
>> 
>>> Signed-off-by: Chao Yu <chao@kernel.org>
>> 
>>> ---
>> 
>>> fs/f2fs/file.c | 6 +++++-
>> 
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> 
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> 
>>> index 0e7a0195eca8..377a10b81bf3 100644
>> 
>>> --- a/fs/f2fs/file.c
>> 
>>> +++ b/fs/f2fs/file.c
>> 
>>> @@ -883,7 +883,11 @@ static bool f2fs_force_buffered_io(struct inode *inode, int rw)
>> 
>>>                  return true;
>> 
>>>          if (f2fs_compressed_file(inode))
>> 
>>>                  return true;
>> 
>>> -        if (f2fs_has_inline_data(inode))
>> 
>>> +        /*
>> 
>>> +         * only force direct read to use buffered IO, for direct write,
>> 
>>> +         * it expects inline data conversion before committing IO.
>> 
>>> +         */
>> 
>>> +        if (f2fs_has_inline_data(inode) && rw == READ)
>> 
>> 
>> Chao Yu,
>> 
>> The fio direct performance problem I reported did not improve when reflecting this commit.
>> 
>> Rather, it has been improved when reflecting your commit below.
>> 
>> 
>> The previous commit seems to be mistitled as read and the following commit appears to be the final version.
>> 
>> The reason for the improvement with the commit below is that there is no "m_may_create" condition
>> 
>> when performing "io_submit" directly, so performance regression issue may occur.
>> 
>> 
>> I wonder if "rw == READ" should be additionally reflected.
>
> Alright, thanks for your feedback.
>
> I thought you have bisected this performance issue to commit
> 5c8764f8679e ("f2fs: fix to force buffered IO on inline_data inode"),
> so I sent this patch for comments.
>
> Can you please apply both below dio fixes, and help to check final
> performance?
>
> https://lore.kernel.org/linux-f2fs-devel/20241104015016.228963-1-chao@kernel.org
> https://lore.kernel.org/linux-f2fs-devel/20241104013551.218037-1-chao@kernel.org
>
> Thanks,

Chao Yu,
After reflecting the following two commits, the fio DIO seq write operates with normal performance.

https://lore.kernel.org/linux-f2fs-devel/20241104015016.228963-1-chao@kernel.org
https://lore.kernel.org/linux-f2fs-devel/20241104013551.218037-1-chao@kernel.org

However, Antutu Apk's "AI READ" performance has more than tripled compared to before patch reflection, so it seems necessary to check if there is any problem with DIO performance.