[PATCH 1/2] f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag

[PATCH 1/2] f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag

[Sunmin Jeong]

In f2fs_update_inode, i_size of the atomic file isn't updated until
FI_ATOMIC_COMMITTED flag is set. When committing atomic write right
after the writeback of the inode, i_size of the raw inode will not be
updated. It can cause the atomicity corruption due to a mismatch between
old file size and new data.

To prevent the problem, let's mark inode dirty for FI_ATOMIC_COMMITTED

Atomic write thread                   Writeback thread
                                        __writeback_single_inode
                                          write_inode
                                            f2fs_update_inode
                                              - skip i_size update
  f2fs_ioc_commit_atomic_write
    f2fs_commit_atomic_write
      set_inode_flag(inode, FI_ATOMIC_COMMITTED)
    f2fs_do_sync_file
      f2fs_fsync_node_pages
        - skip f2fs_update_inode since the inode is clean

Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Cc: stable@vger.kernel.org #v5.19+
Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>
---
 fs/f2fs/f2fs.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 543898482f8b..a000cb024dbe 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3039,6 +3039,7 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
 	case FI_INLINE_DOTS:
 	case FI_PIN_FILE:
 	case FI_COMPRESS_RELEASED:
+	case FI_ATOMIC_COMMITTED:
 		f2fs_mark_inode_dirty_sync(inode, true);
 	}
 }
-- 
2.25.1

[PATCH 2/2] f2fs: truncate page cache before clearing flags when aborting atomic write

[Sunmin Jeong]

In f2fs_do_write_data_page, FI_ATOMIC_FILE flag selects the target inode
between the original inode and COW inode. When aborting atomic write and
writeback occur simultaneously, invalid data can be written to original
inode if the FI_ATOMIC_FILE flag is cleared meanwhile.

To prevent the problem, let's truncate all pages before clearing the flag

Atomic write thread              Writeback thread
  f2fs_abort_atomic_write
    clear_inode_flag(inode, FI_ATOMIC_FILE)
                                  __writeback_single_inode
                                    do_writepages
                                      f2fs_do_write_data_page
                                        - use dn of original inode
    truncate_inode_pages_final

Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Cc: stable@vger.kernel.org #v5.19+
Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>
---
 fs/f2fs/segment.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 7901ede58113..7e47b8054413 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -192,6 +192,9 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
 	if (!f2fs_is_atomic_file(inode))
 		return;
 
+	if (clean)
+		truncate_inode_pages_final(inode->i_mapping);
+
 	release_atomic_write_cnt(inode);
 	clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
 	clear_inode_flag(inode, FI_ATOMIC_REPLACE);
@@ -201,7 +204,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
 	F2FS_I(inode)->atomic_write_task = NULL;
 
 	if (clean) {
-		truncate_inode_pages_final(inode->i_mapping);
 		f2fs_i_size_write(inode, fi->original_i_size);
 		fi->original_i_size = 0;
 	}
-- 
2.25.1

Re: [f2fs-dev] [PATCH 1/2] f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag

[Daeho Jeong]

Reviewed-by: Daeho Jeong <daehojeong@google.com>

On Wed, Mar 13, 2024 at 4:37 AM Sunmin Jeong <s_min.jeong@samsung.com> wrote:
>
> In f2fs_update_inode, i_size of the atomic file isn't updated until
> FI_ATOMIC_COMMITTED flag is set. When committing atomic write right
> after the writeback of the inode, i_size of the raw inode will not be
> updated. It can cause the atomicity corruption due to a mismatch between
> old file size and new data.
>
> To prevent the problem, let's mark inode dirty for FI_ATOMIC_COMMITTED
>
> Atomic write thread                   Writeback thread
>                                         __writeback_single_inode
>                                           write_inode
>                                             f2fs_update_inode
>                                               - skip i_size update
>   f2fs_ioc_commit_atomic_write
>     f2fs_commit_atomic_write
>       set_inode_flag(inode, FI_ATOMIC_COMMITTED)
>     f2fs_do_sync_file
>       f2fs_fsync_node_pages
>         - skip f2fs_update_inode since the inode is clean
>
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Cc: stable@vger.kernel.org #v5.19+
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
> Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>
> ---
>  fs/f2fs/f2fs.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 543898482f8b..a000cb024dbe 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3039,6 +3039,7 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
>         case FI_INLINE_DOTS:
>         case FI_PIN_FILE:
>         case FI_COMPRESS_RELEASED:
> +       case FI_ATOMIC_COMMITTED:
>                 f2fs_mark_inode_dirty_sync(inode, true);
>         }
>  }
> --
> 2.25.1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: truncate page cache before clearing flags when aborting atomic write

[Daeho Jeong]

Reviewed-by: Daeho Jeong <daehojeong@google.com>

On Wed, Mar 13, 2024 at 4:29 AM Sunmin Jeong <s_min.jeong@samsung.com> wrote:
>
> In f2fs_do_write_data_page, FI_ATOMIC_FILE flag selects the target inode
> between the original inode and COW inode. When aborting atomic write and
> writeback occur simultaneously, invalid data can be written to original
> inode if the FI_ATOMIC_FILE flag is cleared meanwhile.
>
> To prevent the problem, let's truncate all pages before clearing the flag
>
> Atomic write thread              Writeback thread
>   f2fs_abort_atomic_write
>     clear_inode_flag(inode, FI_ATOMIC_FILE)
>                                   __writeback_single_inode
>                                     do_writepages
>                                       f2fs_do_write_data_page
>                                         - use dn of original inode
>     truncate_inode_pages_final
>
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Cc: stable@vger.kernel.org #v5.19+
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
> Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>
> ---
>  fs/f2fs/segment.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 7901ede58113..7e47b8054413 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -192,6 +192,9 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>         if (!f2fs_is_atomic_file(inode))
>                 return;
>
> +       if (clean)
> +               truncate_inode_pages_final(inode->i_mapping);
> +
>         release_atomic_write_cnt(inode);
>         clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>         clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> @@ -201,7 +204,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>         F2FS_I(inode)->atomic_write_task = NULL;
>
>         if (clean) {
> -               truncate_inode_pages_final(inode->i_mapping);
>                 f2fs_i_size_write(inode, fi->original_i_size);
>                 fi->original_i_size = 0;
>         }
> --
> 2.25.1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 1/2] f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag

[Chao Yu]

On 2024/3/13 19:26, Sunmin Jeong wrote:
> In f2fs_update_inode, i_size of the atomic file isn't updated until
> FI_ATOMIC_COMMITTED flag is set. When committing atomic write right
> after the writeback of the inode, i_size of the raw inode will not be
> updated. It can cause the atomicity corruption due to a mismatch between
> old file size and new data.
> 
> To prevent the problem, let's mark inode dirty for FI_ATOMIC_COMMITTED
> 
> Atomic write thread                   Writeback thread
>                                          __writeback_single_inode
>                                            write_inode
>                                              f2fs_update_inode
>                                                - skip i_size update
>    f2fs_ioc_commit_atomic_write
>      f2fs_commit_atomic_write
>        set_inode_flag(inode, FI_ATOMIC_COMMITTED)
>      f2fs_do_sync_file
>        f2fs_fsync_node_pages
>          - skip f2fs_update_inode since the inode is clean
> 
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Cc: stable@vger.kernel.org #v5.19+
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
> Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [PATCH 2/2] f2fs: truncate page cache before clearing flags when aborting atomic write

[Chao Yu]

On 2024/3/13 19:26, Sunmin Jeong wrote:
> In f2fs_do_write_data_page, FI_ATOMIC_FILE flag selects the target inode
> between the original inode and COW inode. When aborting atomic write and
> writeback occur simultaneously, invalid data can be written to original
> inode if the FI_ATOMIC_FILE flag is cleared meanwhile.
> 
> To prevent the problem, let's truncate all pages before clearing the flag
> 
> Atomic write thread              Writeback thread
>    f2fs_abort_atomic_write
>      clear_inode_flag(inode, FI_ATOMIC_FILE)
>                                    __writeback_single_inode
>                                      do_writepages
>                                        f2fs_do_write_data_page
>                                          - use dn of original inode
>      truncate_inode_pages_final
> 
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Cc: stable@vger.kernel.org #v5.19+
> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> Reviewed-by: Yeongjin Gil <youngjin.gil@samsung.com>
> Signed-off-by: Sunmin Jeong <s_min.jeong@samsung.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [f2fs-dev] [PATCH 1/2] f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag

[patchwork-bot+f2fs]

Hello:

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

On Wed, 13 Mar 2024 20:26:19 +0900 you wrote:
> In f2fs_update_inode, i_size of the atomic file isn't updated until
> FI_ATOMIC_COMMITTED flag is set. When committing atomic write right
> after the writeback of the inode, i_size of the raw inode will not be
> updated. It can cause the atomicity corruption due to a mismatch between
> old file size and new data.
> 
> To prevent the problem, let's mark inode dirty for FI_ATOMIC_COMMITTED
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,1/2] f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag
    https://git.kernel.org/jaegeuk/f2fs/c/4bf78322346f
  - [f2fs-dev,2/2] f2fs: truncate page cache before clearing flags when aborting atomic write
    https://git.kernel.org/jaegeuk/f2fs/c/74b0ebcbdde4

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