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@xxxxxxxxxxx>
>
> > Closes: https://lore.kernel.org/linux-f2fs-devel/af03dd2c-e361-4f80-b2fd-39440766cf6e@xxxxxxxxxx
>
> > Signed-off-by: Chao Yu <chao@xxxxxxxxxx>
>
> > ---
>
> > 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@xxxxxxxxxx
https://lore.kernel.org/linux-f2fs-devel/20241104013551.218037-1-chao@xxxxxxxxxx

Thanks,

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