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@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 --- 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;
- 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 */ - 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; }
On 2024/7/5 16:25, 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@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,
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@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
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;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?
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);@@ -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);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;-- 2.25.1
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@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
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;
-
-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);@@ -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);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;-- 2.25.1
linux-stable-mirror@lists.linaro.org
-
Chao Yu -
Jaegeuk Kim -
Sunmin Jeong