Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE

[Chao Yu <chao@kernel.org>]

On 2022/10/1 4:01, Daeho Jeong wrote:
> On Fri, Sep 30, 2022 at 9:04 AM Daeho Jeong <daeho43@gmail.com> wrote:
>>
>>>>>>>
>>>>>>> Hi Daeho,
>>>>>>>
>>>>>>> isize should be updated after tagging inode as atomic_write one?
>>>>>>> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
>>>>>>> latter checkpoint may persist that change? IIUC...
>>>>>>>
>>>>>>> Thanks,
>>>>>>
>>>>>> Hi Chao,
>>>>>>
>>>>>> The first patch of this patchset prevents the inode page from being
>>>>>> updated as dirty for atomic file cases.
>>>>>> Is there any other chances for the inode page to be marked as dirty?
>>>>>
>>>>> I mean:
>>>>>
>>>>> Thread A                                Thread B
>>>>> - f2fs_ioc_start_atomic_write
>>>>>     - f2fs_i_size_write(inode, 0)
>>>>>      - f2fs_mark_inode_dirty_sync
>>>>>                                           - checkpoint
>>>>>                                            - persist inode with incorrect zero isize
>>>>>
>>>>>     - set_inode_flag(inode, FI_ATOMIC_FILE)
>>>>>
>>>>> Am I missing something?
>>>>>
>>>>
>>>> So, f2fs_mark_inode_dirty_sync() will not work for atomic files
>>>> anymore, which means it doesn't make the inode dirty.
>>>> Plz, refer to the first patch of this patchset. Or I might be confused
>>>> with something. :(
>>>
>>> I mean FI_ATOMIC_FILE was set after f2fs_i_size_write(), so inode will be set
>>> as dirty.
>>>
>>> Thanks,
>>>
>>
>> Oh, I was confused that f2fs_update_inode() is called in
>> f2fs_mark_inode_dirty_sync().
>> That is called in f2fs_write_inode(). Let me fix this.
> 
> Hmm, I think the issue was already there before my patch.
> So, how about making the inode flushed and clean if the inode is
> already dirty when starting atomic write?

What I mean is:

---
  fs/f2fs/file.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e4b6e51086a3..31b229678b1d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2053,6 +2053,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)

  	isize = i_size_read(inode);
  	fi->original_i_size = isize;
+
+	set_inode_flag(inode, FI_ATOMIC_FILE);
+
  	if (truncate) {
  		set_inode_flag(inode, FI_ATOMIC_REPLACE);
  		truncate_inode_pages_final(inode->i_mapping);
@@ -2063,7 +2066,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)

  	stat_inc_atomic_inode(inode);

-	set_inode_flag(inode, FI_ATOMIC_FILE);
  	set_inode_flag(fi->cow_inode, FI_COW_FILE);
  	clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
  	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
-- 


Let's set FI_ATOMIC_FILE flag before f2fs_i_size_write(inode, 0), so
- f2fs_ioc_start_atomic_write
  - f2fs_i_size_write(, 0)
   - f2fs_mark_inode_dirty_sync
    check f2fs_is_atomic_file() and return correctly.

And for the case the inode is dirty before f2fs_i_size_write(, 0), we
can call f2fs_write_inode() to flush dirty feilds into inode page, and
make inode clean.

> 
>>
>> Thanks,