linux-kernel - Re: [f2fs-dev] [PATCH] f2fs: fix to avoid forcing direct write to use buffered IO on inline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <7f5af1ac-ff50-4e9d-b4a8-94d7a6a2f378@kernel.org>
Date: Mon, 4 Nov 2024 11:40:20 +0800
From: Chao Yu <chao@...nel.org>
To: jinsu1.lee@...sung.com,
 "linux-f2fs-devel@...ts.sourceforge.net"
 <linux-f2fs-devel@...ts.sourceforge.net>
Cc: Chao Yu <chao@...nel.org>, "jaegeuk@...nel.org" <jaegeuk@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to avoid forcing direct write to use
 buffered IO on inline_data inode

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@...sung.com>
> 
>> Closes: https://lore.kernel.org/linux-f2fs-devel/af03dd2c-e361-4f80-b2fd-39440766cf6e@kernel.org
> 
>> Signed-off-by: Chao Yu <chao@...nel.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@...ts.sourceforge.net>
> 
> Date:   Mon Nov 4 09:35:51 2024 +0800
> 
> 
>      [f2fs-dev] [PATCH v2] f2fs: fix to map blocks correctly for direct write
> 
> 
>