* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid atomicity corruption of atomic file
2025-03-14 12:06 ` [PATCH] f2fs: fix to avoid atomicity corruption of atomic file Yeongjin Gil
@ 2025-03-14 19:04 ` Daeho Jeong
2025-03-14 21:50 ` Jaegeuk Kim
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Daeho Jeong @ 2025-03-14 19:04 UTC (permalink / raw)
To: Yeongjin Gil
Cc: jaegeuk, chao, daehojeong, linux-f2fs-devel, linux-kernel,
sj1557.seo
On Fri, Mar 14, 2025 at 5:28 AM Yeongjin Gil <youngjin.gil@samsung.com> wrote:
>
> In the case of the following call stack for an atomic file,
> FI_DIRTY_INODE is set, but FI_ATOMIC_DIRTIED is not subsequently set.
>
> f2fs_file_write_iter
> f2fs_map_blocks
> f2fs_reserve_new_blocks
> inc_valid_block_count
> __mark_inode_dirty(dquot)
> f2fs_dirty_inode
>
> If FI_ATOMIC_DIRTIED is not set, atomic file can encounter corruption
> due to a mismatch between old file size and new data.
>
> To resolve this issue, I changed to set FI_ATOMIC_DIRTIED when
> FI_DIRTY_INODE is set. This ensures that FI_DIRTY_INODE, which was
> previously cleared by the Writeback thread during the commit atomic, is
> set and i_size is updated.
>
> Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit")
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com>
> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com>
> ---
> fs/f2fs/inode.c | 4 +---
> fs/f2fs/super.c | 4 ++++
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index aa2f41696a88..83f862578fc8 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -34,10 +34,8 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> if (f2fs_inode_dirtied(inode, sync))
> return;
>
> - if (f2fs_is_atomic_file(inode)) {
> - set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> + if (f2fs_is_atomic_file(inode))
> return;
> - }
>
> mark_inode_dirty_sync(inode);
> }
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 397df271885c..c08d52c6467a 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1534,6 +1534,10 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync)
> inc_page_count(sbi, F2FS_DIRTY_IMETA);
> }
> spin_unlock(&sbi->inode_lock[DIRTY_META]);
> +
> + if (!ret && f2fs_is_atomic_file(inode))
> + set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +
> return ret;
> }
>
> --
> 2.34.1
>
>
Reviewed-by: Daeho Jeong <daehojeong@google.com>
Thanks!
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] f2fs: fix to avoid atomicity corruption of atomic file
2025-03-14 12:06 ` [PATCH] f2fs: fix to avoid atomicity corruption of atomic file Yeongjin Gil
2025-03-14 19:04 ` [f2fs-dev] " Daeho Jeong
@ 2025-03-14 21:50 ` Jaegeuk Kim
2025-03-14 22:21 ` Jaegeuk Kim
2025-03-14 22:30 ` [f2fs-dev] " patchwork-bot+f2fs
2025-03-17 3:19 ` Chao Yu
3 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2025-03-14 21:50 UTC (permalink / raw)
To: Yeongjin Gil
Cc: chao, daehojeong, linux-f2fs-devel, linux-kernel, sj1557.seo,
s_min.jeong
On 03/14, Yeongjin Gil wrote:
> In the case of the following call stack for an atomic file,
> FI_DIRTY_INODE is set, but FI_ATOMIC_DIRTIED is not subsequently set.
>
> f2fs_file_write_iter
> f2fs_map_blocks
> f2fs_reserve_new_blocks
> inc_valid_block_count
> __mark_inode_dirty(dquot)
> f2fs_dirty_inode
>
> If FI_ATOMIC_DIRTIED is not set, atomic file can encounter corruption
> due to a mismatch between old file size and new data.
>
> To resolve this issue, I changed to set FI_ATOMIC_DIRTIED when
> FI_DIRTY_INODE is set. This ensures that FI_DIRTY_INODE, which was
> previously cleared by the Writeback thread during the commit atomic, is
> set and i_size is updated.
>
> Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit")
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com>
> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com>
> ---
> fs/f2fs/inode.c | 4 +---
> fs/f2fs/super.c | 4 ++++
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index aa2f41696a88..83f862578fc8 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -34,10 +34,8 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> if (f2fs_inode_dirtied(inode, sync))
> return;
>
> - if (f2fs_is_atomic_file(inode)) {
> - set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> + if (f2fs_is_atomic_file(inode))
> return;
> - }
>
> mark_inode_dirty_sync(inode);
> }
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 397df271885c..c08d52c6467a 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1534,6 +1534,10 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync)
> inc_page_count(sbi, F2FS_DIRTY_IMETA);
> }
> spin_unlock(&sbi->inode_lock[DIRTY_META]);
> +
> + if (!ret && f2fs_is_atomic_file(inode))
> + set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +
I'm not sure what's different here.
Before and after this patch, set_inode_flag(inode, FI_ATOMIC_DIRTIED) is called
only if f2fs_inode_dirtied() returns zero and f2fs_is_atomic_file(inode).
Note, f2fs_mark_inode_dirty_sync() looks the single caller of f2fs_inode_dirtied.
> return ret;
> }
>
> --
> 2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] f2fs: fix to avoid atomicity corruption of atomic file
2025-03-14 21:50 ` Jaegeuk Kim
@ 2025-03-14 22:21 ` Jaegeuk Kim
0 siblings, 0 replies; 6+ messages in thread
From: Jaegeuk Kim @ 2025-03-14 22:21 UTC (permalink / raw)
To: Yeongjin Gil
Cc: chao, daehojeong, linux-f2fs-devel, linux-kernel, sj1557.seo,
s_min.jeong
On 03/14, Jaegeuk Kim wrote:
> On 03/14, Yeongjin Gil wrote:
> > In the case of the following call stack for an atomic file,
> > FI_DIRTY_INODE is set, but FI_ATOMIC_DIRTIED is not subsequently set.
> >
> > f2fs_file_write_iter
> > f2fs_map_blocks
> > f2fs_reserve_new_blocks
> > inc_valid_block_count
> > __mark_inode_dirty(dquot)
> > f2fs_dirty_inode
> >
> > If FI_ATOMIC_DIRTIED is not set, atomic file can encounter corruption
> > due to a mismatch between old file size and new data.
> >
> > To resolve this issue, I changed to set FI_ATOMIC_DIRTIED when
> > FI_DIRTY_INODE is set. This ensures that FI_DIRTY_INODE, which was
> > previously cleared by the Writeback thread during the commit atomic, is
> > set and i_size is updated.
> >
> > Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit")
> > Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> > Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com>
> > Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com>
> > ---
> > fs/f2fs/inode.c | 4 +---
> > fs/f2fs/super.c | 4 ++++
> > 2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index aa2f41696a88..83f862578fc8 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -34,10 +34,8 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> > if (f2fs_inode_dirtied(inode, sync))
> > return;
> >
> > - if (f2fs_is_atomic_file(inode)) {
> > - set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > + if (f2fs_is_atomic_file(inode))
> > return;
> > - }
> >
> > mark_inode_dirty_sync(inode);
> > }
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 397df271885c..c08d52c6467a 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -1534,6 +1534,10 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync)
> > inc_page_count(sbi, F2FS_DIRTY_IMETA);
> > }
> > spin_unlock(&sbi->inode_lock[DIRTY_META]);
> > +
> > + if (!ret && f2fs_is_atomic_file(inode))
> > + set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > +
>
> I'm not sure what's different here.
>
> Before and after this patch, set_inode_flag(inode, FI_ATOMIC_DIRTIED) is called
> only if f2fs_inode_dirtied() returns zero and f2fs_is_atomic_file(inode).
>
> Note, f2fs_mark_inode_dirty_sync() looks the single caller of f2fs_inode_dirtied.
Ok, I missed another caller, f2fs_dirty_inode, per discussion with Daeho.
Let me apply this.
>
>
> > return ret;
> > }
> >
> > --
> > 2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid atomicity corruption of atomic file
2025-03-14 12:06 ` [PATCH] f2fs: fix to avoid atomicity corruption of atomic file Yeongjin Gil
2025-03-14 19:04 ` [f2fs-dev] " Daeho Jeong
2025-03-14 21:50 ` Jaegeuk Kim
@ 2025-03-14 22:30 ` patchwork-bot+f2fs
2025-03-17 3:19 ` Chao Yu
3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+f2fs @ 2025-03-14 22:30 UTC (permalink / raw)
To: Yeongjin Gil
Cc: jaegeuk, chao, daehojeong, linux-f2fs-devel, linux-kernel,
sj1557.seo
Hello:
This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:
On Fri, 14 Mar 2025 21:06:51 +0900 you wrote:
> In the case of the following call stack for an atomic file,
> FI_DIRTY_INODE is set, but FI_ATOMIC_DIRTIED is not subsequently set.
>
> f2fs_file_write_iter
> f2fs_map_blocks
> f2fs_reserve_new_blocks
> inc_valid_block_count
> __mark_inode_dirty(dquot)
> f2fs_dirty_inode
>
> [...]
Here is the summary with links:
- [f2fs-dev] f2fs: fix to avoid atomicity corruption of atomic file
https://git.kernel.org/jaegeuk/f2fs/c/c60b556bfe0a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] f2fs: fix to avoid atomicity corruption of atomic file
2025-03-14 12:06 ` [PATCH] f2fs: fix to avoid atomicity corruption of atomic file Yeongjin Gil
` (2 preceding siblings ...)
2025-03-14 22:30 ` [f2fs-dev] " patchwork-bot+f2fs
@ 2025-03-17 3:19 ` Chao Yu
3 siblings, 0 replies; 6+ messages in thread
From: Chao Yu @ 2025-03-17 3:19 UTC (permalink / raw)
To: Yeongjin Gil, jaegeuk, daehojeong, linux-f2fs-devel, linux-kernel
Cc: chao, sj1557.seo, s_min.jeong
On 3/14/25 20:06, Yeongjin Gil wrote:
> In the case of the following call stack for an atomic file,
> FI_DIRTY_INODE is set, but FI_ATOMIC_DIRTIED is not subsequently set.
>
> f2fs_file_write_iter
> f2fs_map_blocks
> f2fs_reserve_new_blocks
> inc_valid_block_count
> __mark_inode_dirty(dquot)
> f2fs_dirty_inode
>
> If FI_ATOMIC_DIRTIED is not set, atomic file can encounter corruption
> due to a mismatch between old file size and new data.
>
> To resolve this issue, I changed to set FI_ATOMIC_DIRTIED when
> FI_DIRTY_INODE is set. This ensures that FI_DIRTY_INODE, which was
> previously cleared by the Writeback thread during the commit atomic, is
> set and i_size is updated.
I guess we need to add a regression testcase to cover this issue.
>
> Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit")
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com>
> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com>
Reviewed-by: Chao Yu <chao@kernel.org>
Thanks,
^ permalink raw reply [flat|nested] 6+ messages in thread