>On 07/05, Sunmin Jeong wrote:
>> In case of the COW file, new updates and GC writes are already
>> separated to page caches of the atomic file and COW file. As some
>> cases that use the meta inode for GC, there are some race issues
>> between a foreground thread and GC thread.
>>
>> To handle them, we need to take care when to invalidate and wait
>> writeback of GC pages in COW files as the case of using the meta inode.
>> Also, a pointer from the COW inode to the original inode is required
>> to check the state of original pages.
>>
>> For the former, we can solve the problem by using the meta inode for
>> GC of COW files. Then let's get a page from the original inode in
>> move_data_block when GCing the COW file to avoid race condition.
>>
>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>> Cc: stable@xxxxxxxxxxxxxxx #v5.19+
>> Reviewed-by: Sungjong Seo <sj1557.seo@xxxxxxxxxxx>
>> Reviewed-by: Yeongjin Gil <youngjin.gil@xxxxxxxxxxx>
>> Signed-off-by: Sunmin Jeong <s_min.jeong@xxxxxxxxxxx>
>> ---
>> v2:
>> - use union for cow inode to point to atomic inode
>> fs/f2fs/data.c | 2 +-
>> fs/f2fs/f2fs.h | 13 +++++++++++--
>> fs/f2fs/file.c | 3 +++
>> fs/f2fs/gc.c | 12 ++++++++++--
>> fs/f2fs/inline.c | 2 +-
>> fs/f2fs/inode.c | 3 ++-
>> 6 files changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>> 9a213d03005d..f6b1782f965a 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -2606,7 +2606,7 @@ bool f2fs_should_update_outplace(struct inode
>*inode, struct f2fs_io_info *fio)
>> return true;
>> if (IS_NOQUOTA(inode))
>> return true;
>> - if (f2fs_is_atomic_file(inode))
>> + if (f2fs_used_in_atomic_write(inode))
>> return true;
>> /* rewrite low ratio compress data w/ OPU mode to avoid
>fragmentation */
>> if (f2fs_compressed_file(inode) &&
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index
>> 796ae11c0fa3..4a8621e4a33a 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -843,7 +843,11 @@ struct f2fs_inode_info {
>> struct task_struct *atomic_write_task; /* store atomic write task
>*/
>> struct extent_tree *extent_tree[NR_EXTENT_CACHES];
>> /* cached extent_tree entry */
>> - struct inode *cow_inode; /* copy-on-write inode for atomic
write
>*/
>> + union {
>> + struct inode *cow_inode; /* copy-on-write inode for
atomic
>write */
>> + struct inode *atomic_inode;
>> + /* point to atomic_inode, available
only
>for cow_inode */
>> + };
>>
>> /* avoid racing between foreground op and gc */
>> struct f2fs_rwsem i_gc_rwsem[2];
>> @@ -4263,9 +4267,14 @@ static inline bool f2fs_post_read_required(struct
>inode *inode)
>> f2fs_compressed_file(inode);
>> }
>>
>> +static inline bool f2fs_used_in_atomic_write(struct inode *inode) {
>> + return f2fs_is_atomic_file(inode) || f2fs_is_cow_file(inode); }
>> +
>> static inline bool f2fs_meta_inode_gc_required(struct inode *inode)
>> {
>> - return f2fs_post_read_required(inode) || f2fs_is_atomic_file(inode);
>> + return f2fs_post_read_required(inode) ||
>> +f2fs_used_in_atomic_write(inode);
>> }
>>
>> /*
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index
>> e4a7cff00796..547e7ec32b1f 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -2183,6 +2183,9 @@ static int f2fs_ioc_start_atomic_write(struct
>> file *filp, bool truncate)
>>
>> set_inode_flag(fi->cow_inode, FI_COW_FILE);
>> clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
>> +
>> + /* Set the COW inode's atomic_inode to the atomic inode */
>> + F2FS_I(fi->cow_inode)->atomic_inode = inode;
>> } else {
>> /* Reuse the already created COW inode */
>> ret = f2fs_do_truncate_blocks(fi->cow_inode, 0, true); diff
-
>-git
>> a/fs/f2fs/gc.c b/fs/f2fs/gc.c index cb3006551ab5..61913fcefd9e 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -1186,7 +1186,11 @@ static int ra_data_block(struct inode *inode,
>pgoff_t index)
>> };
>> int err;
>
>How about giving the right mapping like this?
Thanks for your opinion. I'll send the patch v3.
>
> mapping = f2fs_is_cow_file() ?
> F2FS_I(inode)->actomic_inode->i_mapping : inode->i_mapping;
> page = f2fs_grab_cache_page(mapping, index, true);
>
>>
>> - page = f2fs_grab_cache_page(mapping, index, true);
>> + if (f2fs_is_cow_file(inode))
>> + page = f2fs_grab_cache_page(F2FS_I(inode)->atomic_inode-
>>i_mapping,
>> + index, true);
>> + else
>> + page = f2fs_grab_cache_page(mapping, index, true);
>> if (!page)
>> return -ENOMEM;
>>
>> @@ -1282,7 +1286,11 @@ static int move_data_block(struct inode *inode,
>block_t bidx,
>> CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA;
>>
>> /* do not read out */
>
>ditto?
>
>> - page = f2fs_grab_cache_page(inode->i_mapping, bidx, false);
>> + if (f2fs_is_cow_file(inode))
>> + page = f2fs_grab_cache_page(F2FS_I(inode)->atomic_inode-
>>i_mapping,
>> + bidx, false);
>> + else
>> + page = f2fs_grab_cache_page(inode->i_mapping, bidx, false);
>> if (!page)
>> return -ENOMEM;
>>
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index
>> 1fba5728be70..cca7d448e55c 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -16,7 +16,7 @@
>>
>> static bool support_inline_data(struct inode *inode) {
>> - if (f2fs_is_atomic_file(inode))
>> + if (f2fs_used_in_atomic_write(inode))
>> return false;
>> if (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))
>> return false;
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c index
>> 7a3e2458b2d9..18dea43e694b 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -804,8 +804,9 @@ void f2fs_evict_inode(struct inode *inode)
>>
>> f2fs_abort_atomic_write(inode, true);
>>
>> - if (fi->cow_inode) {
>> + if (fi->cow_inode && f2fs_is_cow_file(fi->cow_inode)) {
>> clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>> + F2FS_I(fi->cow_inode)->atomic_inode = NULL;
>> iput(fi->cow_inode);
>> fi->cow_inode = NULL;
>> }
>> --
>> 2.25.1
