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

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

[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

[이진수]

>>> 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.