* [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-09 18:18 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-02-09 18:18 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
From: Daeho Jeong <daehojeong@google.com>
To fix a race condition between atomic write aborts, I use the inode
lock and make COW inode to be re-usable thoroughout the whole
atomic file inode lifetime.
Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
fs/f2fs/file.c | 44 +++++++++++++++++++++++++++++---------------
fs/f2fs/inode.c | 11 +++++++++--
fs/f2fs/segment.c | 3 ---
fs/f2fs/super.c | 2 --
4 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 300eae8b5415..6436c52e7913 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1863,7 +1863,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
atomic_read(&inode->i_writecount) != 1)
return 0;
+ inode_lock(inode);
f2fs_abort_atomic_write(inode, true);
+ inode_unlock(inode);
+
return 0;
}
@@ -1878,8 +1881,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
* before dropping file lock, it needs to do in ->flush.
*/
if (F2FS_I(inode)->atomic_write_task == current &&
- (current->flags & PF_EXITING))
+ (current->flags & PF_EXITING)) {
+ inode_lock(inode);
f2fs_abort_atomic_write(inode, true);
+ inode_unlock(inode);
+ }
+
return 0;
}
@@ -2085,19 +2092,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
goto out;
}
- /* Create a COW inode for atomic write */
- pinode = f2fs_iget(inode->i_sb, fi->i_pino);
- if (IS_ERR(pinode)) {
- f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
- ret = PTR_ERR(pinode);
- goto out;
- }
+ /* Check if the inode already has a COW inode */
+ if (fi->cow_inode == NULL) {
+ /* Create a COW inode for atomic write */
+ pinode = f2fs_iget(inode->i_sb, fi->i_pino);
+ if (IS_ERR(pinode)) {
+ f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
+ ret = PTR_ERR(pinode);
+ goto out;
+ }
- ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
- iput(pinode);
- if (ret) {
- f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
- goto out;
+ ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
+ iput(pinode);
+ if (ret) {
+ f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
+ goto out;
+ }
+
+ set_inode_flag(fi->cow_inode, FI_COW_FILE);
+ clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
+ } else {
+ /* Reuse the already created COW inode */
+ f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
}
f2fs_write_inode(inode, NULL);
@@ -2105,8 +2121,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);
isize = i_size_read(inode);
fi->original_i_size = isize;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 28c9c72dda2a..7bf660d4cad9 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
void f2fs_evict_inode(struct inode *inode)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
- nid_t xnid = F2FS_I(inode)->i_xattr_nid;
+ struct f2fs_inode_info *fi = F2FS_I(inode);
+ nid_t xnid = fi->i_xattr_nid;
int err = 0;
f2fs_abort_atomic_write(inode, true);
+ if (fi->cow_inode) {
+ clear_inode_flag(fi->cow_inode, FI_COW_FILE);
+ iput(fi->cow_inode);
+ fi->cow_inode = NULL;
+ }
+
trace_f2fs_evict_inode(inode);
truncate_inode_pages_final(&inode->i_data);
@@ -866,7 +873,7 @@ void f2fs_evict_inode(struct inode *inode)
stat_dec_inline_inode(inode);
stat_dec_compr_inode(inode);
stat_sub_compr_blocks(inode,
- atomic_read(&F2FS_I(inode)->i_compr_blocks));
+ atomic_read(&fi->i_compr_blocks));
if (likely(!f2fs_cp_error(sbi) &&
!is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ead3f35f501d..719329c1808c 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
if (!f2fs_is_atomic_file(inode))
return;
- clear_inode_flag(fi->cow_inode, FI_COW_FILE);
- iput(fi->cow_inode);
- fi->cow_inode = NULL;
release_atomic_write_cnt(inode);
clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
clear_inode_flag(inode, FI_ATOMIC_REPLACE);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index c11a161ba5be..aa55dc12aff2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1427,8 +1427,6 @@ static int f2fs_drop_inode(struct inode *inode)
atomic_inc(&inode->i_count);
spin_unlock(&inode->i_lock);
- f2fs_abort_atomic_write(inode, true);
-
/* should remain fi->extent_tree for writepage */
f2fs_destroy_extent_node(inode);
--
2.39.1.581.gbfd45094c4-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-09 18:18 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-02-09 18:18 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
From: Daeho Jeong <daehojeong@google.com>
To fix a race condition between atomic write aborts, I use the inode
lock and make COW inode to be re-usable thoroughout the whole
atomic file inode lifetime.
Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
fs/f2fs/file.c | 44 +++++++++++++++++++++++++++++---------------
fs/f2fs/inode.c | 11 +++++++++--
fs/f2fs/segment.c | 3 ---
fs/f2fs/super.c | 2 --
4 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 300eae8b5415..6436c52e7913 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1863,7 +1863,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
atomic_read(&inode->i_writecount) != 1)
return 0;
+ inode_lock(inode);
f2fs_abort_atomic_write(inode, true);
+ inode_unlock(inode);
+
return 0;
}
@@ -1878,8 +1881,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
* before dropping file lock, it needs to do in ->flush.
*/
if (F2FS_I(inode)->atomic_write_task == current &&
- (current->flags & PF_EXITING))
+ (current->flags & PF_EXITING)) {
+ inode_lock(inode);
f2fs_abort_atomic_write(inode, true);
+ inode_unlock(inode);
+ }
+
return 0;
}
@@ -2085,19 +2092,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
goto out;
}
- /* Create a COW inode for atomic write */
- pinode = f2fs_iget(inode->i_sb, fi->i_pino);
- if (IS_ERR(pinode)) {
- f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
- ret = PTR_ERR(pinode);
- goto out;
- }
+ /* Check if the inode already has a COW inode */
+ if (fi->cow_inode == NULL) {
+ /* Create a COW inode for atomic write */
+ pinode = f2fs_iget(inode->i_sb, fi->i_pino);
+ if (IS_ERR(pinode)) {
+ f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
+ ret = PTR_ERR(pinode);
+ goto out;
+ }
- ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
- iput(pinode);
- if (ret) {
- f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
- goto out;
+ ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
+ iput(pinode);
+ if (ret) {
+ f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
+ goto out;
+ }
+
+ set_inode_flag(fi->cow_inode, FI_COW_FILE);
+ clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
+ } else {
+ /* Reuse the already created COW inode */
+ f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
}
f2fs_write_inode(inode, NULL);
@@ -2105,8 +2121,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);
isize = i_size_read(inode);
fi->original_i_size = isize;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 28c9c72dda2a..7bf660d4cad9 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
void f2fs_evict_inode(struct inode *inode)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
- nid_t xnid = F2FS_I(inode)->i_xattr_nid;
+ struct f2fs_inode_info *fi = F2FS_I(inode);
+ nid_t xnid = fi->i_xattr_nid;
int err = 0;
f2fs_abort_atomic_write(inode, true);
+ if (fi->cow_inode) {
+ clear_inode_flag(fi->cow_inode, FI_COW_FILE);
+ iput(fi->cow_inode);
+ fi->cow_inode = NULL;
+ }
+
trace_f2fs_evict_inode(inode);
truncate_inode_pages_final(&inode->i_data);
@@ -866,7 +873,7 @@ void f2fs_evict_inode(struct inode *inode)
stat_dec_inline_inode(inode);
stat_dec_compr_inode(inode);
stat_sub_compr_blocks(inode,
- atomic_read(&F2FS_I(inode)->i_compr_blocks));
+ atomic_read(&fi->i_compr_blocks));
if (likely(!f2fs_cp_error(sbi) &&
!is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ead3f35f501d..719329c1808c 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
if (!f2fs_is_atomic_file(inode))
return;
- clear_inode_flag(fi->cow_inode, FI_COW_FILE);
- iput(fi->cow_inode);
- fi->cow_inode = NULL;
release_atomic_write_cnt(inode);
clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
clear_inode_flag(inode, FI_ATOMIC_REPLACE);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index c11a161ba5be..aa55dc12aff2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1427,8 +1427,6 @@ static int f2fs_drop_inode(struct inode *inode)
atomic_inc(&inode->i_count);
spin_unlock(&inode->i_lock);
- f2fs_abort_atomic_write(inode, true);
-
/* should remain fi->extent_tree for writepage */
f2fs_destroy_extent_node(inode);
--
2.39.1.581.gbfd45094c4-goog
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-02-09 18:18 ` [f2fs-dev] " Daeho Jeong
@ 2023-02-13 9:47 ` Chao Yu
-1 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-13 9:47 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
On 2023/2/10 2:18, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> fs/f2fs/file.c | 44 +++++++++++++++++++++++++++++---------------
> fs/f2fs/inode.c | 11 +++++++++--
> fs/f2fs/segment.c | 3 ---
> fs/f2fs/super.c | 2 --
> 4 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 300eae8b5415..6436c52e7913 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1863,7 +1863,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> atomic_read(&inode->i_writecount) != 1)
> return 0;
>
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> +
> return 0;
> }
>
> @@ -1878,8 +1881,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> * before dropping file lock, it needs to do in ->flush.
> */
> if (F2FS_I(inode)->atomic_write_task == current &&
> - (current->flags & PF_EXITING))
> + (current->flags & PF_EXITING)) {
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> + }
> +
> return 0;
> }
>
> @@ -2085,19 +2092,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> goto out;
> }
>
> - /* Create a COW inode for atomic write */
> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> - if (IS_ERR(pinode)) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - ret = PTR_ERR(pinode);
> - goto out;
> - }
> + /* Check if the inode already has a COW inode */
> + if (fi->cow_inode == NULL) {
> + /* Create a COW inode for atomic write */
> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> + if (IS_ERR(pinode)) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + ret = PTR_ERR(pinode);
> + goto out;
> + }
>
> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> - iput(pinode);
> - if (ret) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - goto out;
> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> + iput(pinode);
> + if (ret) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + goto out;
> + }
> +
> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> + } else {
> + /* Reuse the already created COW inode */
> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> }
>
> f2fs_write_inode(inode, NULL);
> @@ -2105,8 +2121,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);
>
> isize = i_size_read(inode);
> fi->original_i_size = isize;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 28c9c72dda2a..7bf660d4cad9 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> void f2fs_evict_inode(struct inode *inode)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> + struct f2fs_inode_info *fi = F2FS_I(inode);
> + nid_t xnid = fi->i_xattr_nid;
> int err = 0;
>
> f2fs_abort_atomic_write(inode, true);
>
> + if (fi->cow_inode) {
> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> + iput(fi->cow_inode);
> + fi->cow_inode = NULL;
> + }
It looks "fi->cow_inode = NULL" here may race w/ cow_inode allocation in
f2fs_ioc_start_atomic_write due to f2fs_write_inode() has not been covered
by inode_lock()? IIUC.
Thanks,
> +
> trace_f2fs_evict_inode(inode);
> truncate_inode_pages_final(&inode->i_data);
>
> @@ -866,7 +873,7 @@ void f2fs_evict_inode(struct inode *inode)
> stat_dec_inline_inode(inode);
> stat_dec_compr_inode(inode);
> stat_sub_compr_blocks(inode,
> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> + atomic_read(&fi->i_compr_blocks));
>
> if (likely(!f2fs_cp_error(sbi) &&
> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index ead3f35f501d..719329c1808c 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> if (!f2fs_is_atomic_file(inode))
> return;
>
> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> - iput(fi->cow_inode);
> - fi->cow_inode = NULL;
> release_atomic_write_cnt(inode);
> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index c11a161ba5be..aa55dc12aff2 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1427,8 +1427,6 @@ static int f2fs_drop_inode(struct inode *inode)
> atomic_inc(&inode->i_count);
> spin_unlock(&inode->i_lock);
>
> - f2fs_abort_atomic_write(inode, true);
> -
> /* should remain fi->extent_tree for writepage */
> f2fs_destroy_extent_node(inode);
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-13 9:47 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-13 9:47 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
On 2023/2/10 2:18, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> fs/f2fs/file.c | 44 +++++++++++++++++++++++++++++---------------
> fs/f2fs/inode.c | 11 +++++++++--
> fs/f2fs/segment.c | 3 ---
> fs/f2fs/super.c | 2 --
> 4 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 300eae8b5415..6436c52e7913 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1863,7 +1863,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> atomic_read(&inode->i_writecount) != 1)
> return 0;
>
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> +
> return 0;
> }
>
> @@ -1878,8 +1881,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> * before dropping file lock, it needs to do in ->flush.
> */
> if (F2FS_I(inode)->atomic_write_task == current &&
> - (current->flags & PF_EXITING))
> + (current->flags & PF_EXITING)) {
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> + }
> +
> return 0;
> }
>
> @@ -2085,19 +2092,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> goto out;
> }
>
> - /* Create a COW inode for atomic write */
> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> - if (IS_ERR(pinode)) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - ret = PTR_ERR(pinode);
> - goto out;
> - }
> + /* Check if the inode already has a COW inode */
> + if (fi->cow_inode == NULL) {
> + /* Create a COW inode for atomic write */
> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> + if (IS_ERR(pinode)) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + ret = PTR_ERR(pinode);
> + goto out;
> + }
>
> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> - iput(pinode);
> - if (ret) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - goto out;
> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> + iput(pinode);
> + if (ret) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + goto out;
> + }
> +
> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> + } else {
> + /* Reuse the already created COW inode */
> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> }
>
> f2fs_write_inode(inode, NULL);
> @@ -2105,8 +2121,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);
>
> isize = i_size_read(inode);
> fi->original_i_size = isize;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 28c9c72dda2a..7bf660d4cad9 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> void f2fs_evict_inode(struct inode *inode)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> + struct f2fs_inode_info *fi = F2FS_I(inode);
> + nid_t xnid = fi->i_xattr_nid;
> int err = 0;
>
> f2fs_abort_atomic_write(inode, true);
>
> + if (fi->cow_inode) {
> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> + iput(fi->cow_inode);
> + fi->cow_inode = NULL;
> + }
It looks "fi->cow_inode = NULL" here may race w/ cow_inode allocation in
f2fs_ioc_start_atomic_write due to f2fs_write_inode() has not been covered
by inode_lock()? IIUC.
Thanks,
> +
> trace_f2fs_evict_inode(inode);
> truncate_inode_pages_final(&inode->i_data);
>
> @@ -866,7 +873,7 @@ void f2fs_evict_inode(struct inode *inode)
> stat_dec_inline_inode(inode);
> stat_dec_compr_inode(inode);
> stat_sub_compr_blocks(inode,
> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> + atomic_read(&fi->i_compr_blocks));
>
> if (likely(!f2fs_cp_error(sbi) &&
> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index ead3f35f501d..719329c1808c 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> if (!f2fs_is_atomic_file(inode))
> return;
>
> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> - iput(fi->cow_inode);
> - fi->cow_inode = NULL;
> release_atomic_write_cnt(inode);
> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index c11a161ba5be..aa55dc12aff2 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1427,8 +1427,6 @@ static int f2fs_drop_inode(struct inode *inode)
> atomic_inc(&inode->i_count);
> spin_unlock(&inode->i_lock);
>
> - f2fs_abort_atomic_write(inode, true);
> -
> /* should remain fi->extent_tree for writepage */
> f2fs_destroy_extent_node(inode);
>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-02-13 9:47 ` Chao Yu
@ 2023-02-13 20:14 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-02-13 20:14 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
On Mon, Feb 13, 2023 at 1:47 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2023/2/10 2:18, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > To fix a race condition between atomic write aborts, I use the inode
> > lock and make COW inode to be re-usable thoroughout the whole
> > atomic file inode lifetime.
> >
> > Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > fs/f2fs/file.c | 44 +++++++++++++++++++++++++++++---------------
> > fs/f2fs/inode.c | 11 +++++++++--
> > fs/f2fs/segment.c | 3 ---
> > fs/f2fs/super.c | 2 --
> > 4 files changed, 38 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 300eae8b5415..6436c52e7913 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1863,7 +1863,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > atomic_read(&inode->i_writecount) != 1)
> > return 0;
> >
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > +
> > return 0;
> > }
> >
> > @@ -1878,8 +1881,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > * before dropping file lock, it needs to do in ->flush.
> > */
> > if (F2FS_I(inode)->atomic_write_task == current &&
> > - (current->flags & PF_EXITING))
> > + (current->flags & PF_EXITING)) {
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -2085,19 +2092,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > goto out;
> > }
> >
> > - /* Create a COW inode for atomic write */
> > - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > - if (IS_ERR(pinode)) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - ret = PTR_ERR(pinode);
> > - goto out;
> > - }
> > + /* Check if the inode already has a COW inode */
> > + if (fi->cow_inode == NULL) {
> > + /* Create a COW inode for atomic write */
> > + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > + if (IS_ERR(pinode)) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + ret = PTR_ERR(pinode);
> > + goto out;
> > + }
> >
> > - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > - iput(pinode);
> > - if (ret) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - goto out;
> > + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > + iput(pinode);
> > + if (ret) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + goto out;
> > + }
> > +
> > + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > + } else {
> > + /* Reuse the already created COW inode */
> > + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > }
> >
> > f2fs_write_inode(inode, NULL);
> > @@ -2105,8 +2121,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);
> >
> > isize = i_size_read(inode);
> > fi->original_i_size = isize;
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 28c9c72dda2a..7bf660d4cad9 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > void f2fs_evict_inode(struct inode *inode)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > + struct f2fs_inode_info *fi = F2FS_I(inode);
> > + nid_t xnid = fi->i_xattr_nid;
> > int err = 0;
> >
> > f2fs_abort_atomic_write(inode, true);
> >
> > + if (fi->cow_inode) {
> > + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + iput(fi->cow_inode);
> > + fi->cow_inode = NULL;
> > + }
>
> It looks "fi->cow_inode = NULL" here may race w/ cow_inode allocation in
> f2fs_ioc_start_atomic_write due to f2fs_write_inode() has not been covered
> by inode_lock()? IIUC.
Sorry, I couldn't understand it, since I couldn't find any relation
between f2fs_ioc_start_atomic_write and f2fs_write_inode. Could you
elaborate more on this? I thought the code might be safe, since it
happens in the inode eviction phase.
>
> Thanks,
>
> > +
> > trace_f2fs_evict_inode(inode);
> > truncate_inode_pages_final(&inode->i_data);
> >
> > @@ -866,7 +873,7 @@ void f2fs_evict_inode(struct inode *inode)
> > stat_dec_inline_inode(inode);
> > stat_dec_compr_inode(inode);
> > stat_sub_compr_blocks(inode,
> > - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > + atomic_read(&fi->i_compr_blocks));
> >
> > if (likely(!f2fs_cp_error(sbi) &&
> > !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index ead3f35f501d..719329c1808c 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > if (!f2fs_is_atomic_file(inode))
> > return;
> >
> > - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > - iput(fi->cow_inode);
> > - fi->cow_inode = NULL;
> > release_atomic_write_cnt(inode);
> > clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index c11a161ba5be..aa55dc12aff2 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -1427,8 +1427,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > atomic_inc(&inode->i_count);
> > spin_unlock(&inode->i_lock);
> >
> > - f2fs_abort_atomic_write(inode, true);
> > -
> > /* should remain fi->extent_tree for writepage */
> > f2fs_destroy_extent_node(inode);
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-13 20:14 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-02-13 20:14 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
On Mon, Feb 13, 2023 at 1:47 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2023/2/10 2:18, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > To fix a race condition between atomic write aborts, I use the inode
> > lock and make COW inode to be re-usable thoroughout the whole
> > atomic file inode lifetime.
> >
> > Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > fs/f2fs/file.c | 44 +++++++++++++++++++++++++++++---------------
> > fs/f2fs/inode.c | 11 +++++++++--
> > fs/f2fs/segment.c | 3 ---
> > fs/f2fs/super.c | 2 --
> > 4 files changed, 38 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 300eae8b5415..6436c52e7913 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1863,7 +1863,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > atomic_read(&inode->i_writecount) != 1)
> > return 0;
> >
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > +
> > return 0;
> > }
> >
> > @@ -1878,8 +1881,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > * before dropping file lock, it needs to do in ->flush.
> > */
> > if (F2FS_I(inode)->atomic_write_task == current &&
> > - (current->flags & PF_EXITING))
> > + (current->flags & PF_EXITING)) {
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -2085,19 +2092,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > goto out;
> > }
> >
> > - /* Create a COW inode for atomic write */
> > - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > - if (IS_ERR(pinode)) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - ret = PTR_ERR(pinode);
> > - goto out;
> > - }
> > + /* Check if the inode already has a COW inode */
> > + if (fi->cow_inode == NULL) {
> > + /* Create a COW inode for atomic write */
> > + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > + if (IS_ERR(pinode)) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + ret = PTR_ERR(pinode);
> > + goto out;
> > + }
> >
> > - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > - iput(pinode);
> > - if (ret) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - goto out;
> > + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > + iput(pinode);
> > + if (ret) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + goto out;
> > + }
> > +
> > + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > + } else {
> > + /* Reuse the already created COW inode */
> > + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > }
> >
> > f2fs_write_inode(inode, NULL);
> > @@ -2105,8 +2121,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);
> >
> > isize = i_size_read(inode);
> > fi->original_i_size = isize;
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 28c9c72dda2a..7bf660d4cad9 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > void f2fs_evict_inode(struct inode *inode)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > + struct f2fs_inode_info *fi = F2FS_I(inode);
> > + nid_t xnid = fi->i_xattr_nid;
> > int err = 0;
> >
> > f2fs_abort_atomic_write(inode, true);
> >
> > + if (fi->cow_inode) {
> > + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + iput(fi->cow_inode);
> > + fi->cow_inode = NULL;
> > + }
>
> It looks "fi->cow_inode = NULL" here may race w/ cow_inode allocation in
> f2fs_ioc_start_atomic_write due to f2fs_write_inode() has not been covered
> by inode_lock()? IIUC.
Sorry, I couldn't understand it, since I couldn't find any relation
between f2fs_ioc_start_atomic_write and f2fs_write_inode. Could you
elaborate more on this? I thought the code might be safe, since it
happens in the inode eviction phase.
>
> Thanks,
>
> > +
> > trace_f2fs_evict_inode(inode);
> > truncate_inode_pages_final(&inode->i_data);
> >
> > @@ -866,7 +873,7 @@ void f2fs_evict_inode(struct inode *inode)
> > stat_dec_inline_inode(inode);
> > stat_dec_compr_inode(inode);
> > stat_sub_compr_blocks(inode,
> > - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > + atomic_read(&fi->i_compr_blocks));
> >
> > if (likely(!f2fs_cp_error(sbi) &&
> > !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index ead3f35f501d..719329c1808c 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > if (!f2fs_is_atomic_file(inode))
> > return;
> >
> > - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > - iput(fi->cow_inode);
> > - fi->cow_inode = NULL;
> > release_atomic_write_cnt(inode);
> > clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index c11a161ba5be..aa55dc12aff2 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -1427,8 +1427,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > atomic_inc(&inode->i_count);
> > spin_unlock(&inode->i_lock);
> >
> > - f2fs_abort_atomic_write(inode, true);
> > -
> > /* should remain fi->extent_tree for writepage */
> > f2fs_destroy_extent_node(inode);
> >
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-02-13 20:14 ` Daeho Jeong
@ 2023-02-14 1:46 ` Chao Yu
-1 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-14 1:46 UTC (permalink / raw)
To: Daeho Jeong
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
On 2023/2/14 4:14, Daeho Jeong wrote:
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 28c9c72dda2a..7bf660d4cad9 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>>> void f2fs_evict_inode(struct inode *inode)
>>> {
>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
>>> + nid_t xnid = fi->i_xattr_nid;
>>> int err = 0;
>>>
>>> f2fs_abort_atomic_write(inode, true);
>>>
>>> + if (fi->cow_inode) {
>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> + iput(fi->cow_inode);
>>> + fi->cow_inode = NULL;
>>> + }
>>
>> It looks "fi->cow_inode = NULL" here may race w/ cow_inode allocation in
>> f2fs_ioc_start_atomic_write due to f2fs_write_inode() has not been covered
>> by inode_lock()? IIUC.
>
> Sorry, I couldn't understand it, since I couldn't find any relation
> between f2fs_ioc_start_atomic_write and f2fs_write_inode. Could you
> elaborate more on this? I thought the code might be safe, since it
> happens in the inode eviction phase.
int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
void f2fs_evict_inode(struct inode *inode)
Oops, it looks I was misled by f2fs_write_inode() above f2fs_evict_inode(),
please ignore my comments, sorry. :(
Thanks,
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-14 1:46 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-14 1:46 UTC (permalink / raw)
To: Daeho Jeong
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
On 2023/2/14 4:14, Daeho Jeong wrote:
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 28c9c72dda2a..7bf660d4cad9 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -777,11 +777,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>>> void f2fs_evict_inode(struct inode *inode)
>>> {
>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
>>> + nid_t xnid = fi->i_xattr_nid;
>>> int err = 0;
>>>
>>> f2fs_abort_atomic_write(inode, true);
>>>
>>> + if (fi->cow_inode) {
>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> + iput(fi->cow_inode);
>>> + fi->cow_inode = NULL;
>>> + }
>>
>> It looks "fi->cow_inode = NULL" here may race w/ cow_inode allocation in
>> f2fs_ioc_start_atomic_write due to f2fs_write_inode() has not been covered
>> by inode_lock()? IIUC.
>
> Sorry, I couldn't understand it, since I couldn't find any relation
> between f2fs_ioc_start_atomic_write and f2fs_write_inode. Could you
> elaborate more on this? I thought the code might be safe, since it
> happens in the inode eviction phase.
int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
void f2fs_evict_inode(struct inode *inode)
Oops, it looks I was misled by f2fs_write_inode() above f2fs_evict_inode(),
please ignore my comments, sorry. :(
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] 31+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-02-09 18:18 ` [f2fs-dev] " Daeho Jeong
@ 2023-02-14 1:47 ` Chao Yu
-1 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-14 1:47 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
On 2023/2/10 2:18, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
Reviewed-by: Chao Yu <chao@kernel.org>
Thanks,
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-14 1:47 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-14 1:47 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
On 2023/2/10 2:18, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
Reviewed-by: Chao Yu <chao@kernel.org>
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] 31+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-02-09 18:18 ` [f2fs-dev] " Daeho Jeong
@ 2023-02-14 18:10 ` patchwork-bot+f2fs
-1 siblings, 0 replies; 31+ messages in thread
From: patchwork-bot+f2fs @ 2023-02-14 18:10 UTC (permalink / raw)
To: Daeho Jeong
Cc: linux-kernel, linux-f2fs-devel, kernel-team, daehojeong,
syzbot+823000d23b3400619f7c
Hello:
This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:
On Thu, 9 Feb 2023 10:18:19 -0800 you wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>
> [...]
Here is the summary with links:
- [f2fs-dev] f2fs: synchronize atomic write aborts
https://git.kernel.org/jaegeuk/f2fs/c/a46bebd502fe
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-14 18:10 ` patchwork-bot+f2fs
0 siblings, 0 replies; 31+ messages in thread
From: patchwork-bot+f2fs @ 2023-02-14 18:10 UTC (permalink / raw)
To: Daeho Jeong
Cc: daehojeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
Hello:
This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:
On Thu, 9 Feb 2023 10:18:19 -0800 you wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>
> [...]
Here is the summary with links:
- [f2fs-dev] f2fs: synchronize atomic write aborts
https://git.kernel.org/jaegeuk/f2fs/c/a46bebd502fe
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
_______________________________________________
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] 31+ messages in thread
* [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-13 0:49 Daeho Jeong
2023-01-28 2:07 ` Chao Yu
0 siblings, 1 reply; 31+ messages in thread
From: Daeho Jeong @ 2023-01-13 0:49 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
From: Daeho Jeong <daehojeong@google.com>
To fix a race condition between atomic write aborts, I use the inode
lock and make COW inode to be re-usable thoroughout the whole
atomic file inode lifetime.
Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
fs/f2fs/inode.c | 11 +++++++++--
fs/f2fs/segment.c | 3 ---
fs/f2fs/super.c | 2 --
4 files changed, 37 insertions(+), 22 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ecbc8c135b49..ff072a9ed258 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
atomic_read(&inode->i_writecount) != 1)
return 0;
+ inode_lock(inode);
f2fs_abort_atomic_write(inode, true);
+ inode_unlock(inode);
+
return 0;
}
@@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
* until all the writers close its file. Since this should be done
* before dropping file lock, it needs to do in ->flush.
*/
- if (F2FS_I(inode)->atomic_write_task == current)
+ if (F2FS_I(inode)->atomic_write_task == current) {
+ inode_lock(inode);
f2fs_abort_atomic_write(inode, true);
+ inode_unlock(inode);
+ }
return 0;
}
@@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
goto out;
}
- /* Create a COW inode for atomic write */
- pinode = f2fs_iget(inode->i_sb, fi->i_pino);
- if (IS_ERR(pinode)) {
- f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
- ret = PTR_ERR(pinode);
- goto out;
- }
+ /* Check if the inode already has a COW inode */
+ if (fi->cow_inode == NULL) {
+ /* Create a COW inode for atomic write */
+ pinode = f2fs_iget(inode->i_sb, fi->i_pino);
+ if (IS_ERR(pinode)) {
+ f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
+ ret = PTR_ERR(pinode);
+ goto out;
+ }
- ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
- iput(pinode);
- if (ret) {
- f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
- goto out;
+ ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
+ iput(pinode);
+ if (ret) {
+ f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
+ goto out;
+ }
+
+ set_inode_flag(fi->cow_inode, FI_COW_FILE);
+ clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
+ } else {
+ /* Reuse the already created COW inode */
+ f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
}
f2fs_write_inode(inode, NULL);
@@ -2107,8 +2122,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);
isize = i_size_read(inode);
fi->original_i_size = isize;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index ff6cf66ed46b..4921f7209e28 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
void f2fs_evict_inode(struct inode *inode)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
- nid_t xnid = F2FS_I(inode)->i_xattr_nid;
+ struct f2fs_inode_info *fi = F2FS_I(inode);
+ nid_t xnid = fi->i_xattr_nid;
int err = 0;
f2fs_abort_atomic_write(inode, true);
+ if (fi->cow_inode) {
+ clear_inode_flag(fi->cow_inode, FI_COW_FILE);
+ iput(fi->cow_inode);
+ fi->cow_inode = NULL;
+ }
+
trace_f2fs_evict_inode(inode);
truncate_inode_pages_final(&inode->i_data);
@@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
stat_dec_inline_inode(inode);
stat_dec_compr_inode(inode);
stat_sub_compr_blocks(inode,
- atomic_read(&F2FS_I(inode)->i_compr_blocks));
+ atomic_read(&fi->i_compr_blocks));
if (likely(!f2fs_cp_error(sbi) &&
!is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ae3c4e5474ef..536d7c674b04 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
if (!f2fs_is_atomic_file(inode))
return;
- clear_inode_flag(fi->cow_inode, FI_COW_FILE);
- iput(fi->cow_inode);
- fi->cow_inode = NULL;
release_atomic_write_cnt(inode);
clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
clear_inode_flag(inode, FI_ATOMIC_REPLACE);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1f812b9ce985..10463f084d30 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
atomic_inc(&inode->i_count);
spin_unlock(&inode->i_lock);
- f2fs_abort_atomic_write(inode, true);
-
/* should remain fi->extent_tree for writepage */
f2fs_destroy_extent_node(inode);
--
2.39.0.246.g2a6d74b583-goog
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-13 0:49 Daeho Jeong
@ 2023-01-28 2:07 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-01-28 2:07 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
Hi Daeho, Jaegeuk,
Please take a look at patchset in below link:
https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
know your preference. :)
One comment as below.
On 2023/1/13 8:49, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> fs/f2fs/inode.c | 11 +++++++++--
> fs/f2fs/segment.c | 3 ---
> fs/f2fs/super.c | 2 --
> 4 files changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index ecbc8c135b49..ff072a9ed258 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> atomic_read(&inode->i_writecount) != 1)
> return 0;
>
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> +
> return 0;
> }
>
> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> * until all the writers close its file. Since this should be done
> * before dropping file lock, it needs to do in ->flush.
> */
> - if (F2FS_I(inode)->atomic_write_task == current)
> + if (F2FS_I(inode)->atomic_write_task == current) {
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> + }
> return 0;
> }
>
> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> goto out;
> }
>
> - /* Create a COW inode for atomic write */
> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> - if (IS_ERR(pinode)) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - ret = PTR_ERR(pinode);
> - goto out;
> - }
> + /* Check if the inode already has a COW inode */
> + if (fi->cow_inode == NULL) {
> + /* Create a COW inode for atomic write */
> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> + if (IS_ERR(pinode)) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + ret = PTR_ERR(pinode);
> + goto out;
> + }
>
> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> - iput(pinode);
> - if (ret) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - goto out;
> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> + iput(pinode);
> + if (ret) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + goto out;
> + }
> +
> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> + } else {
> + /* Reuse the already created COW inode */
> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> }
>
> f2fs_write_inode(inode, NULL);
> @@ -2107,8 +2122,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);
>
> isize = i_size_read(inode);
> fi->original_i_size = isize;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index ff6cf66ed46b..4921f7209e28 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> void f2fs_evict_inode(struct inode *inode)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> + struct f2fs_inode_info *fi = F2FS_I(inode);
> + nid_t xnid = fi->i_xattr_nid;
> int err = 0;
>
> f2fs_abort_atomic_write(inode, true);
>
> + if (fi->cow_inode) {
> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> + iput(fi->cow_inode);
> + fi->cow_inode = NULL;
> + }
> +
> trace_f2fs_evict_inode(inode);
> truncate_inode_pages_final(&inode->i_data);
>
> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> stat_dec_inline_inode(inode);
> stat_dec_compr_inode(inode);
> stat_sub_compr_blocks(inode,
> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> + atomic_read(&fi->i_compr_blocks));
>
> if (likely(!f2fs_cp_error(sbi) &&
> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index ae3c4e5474ef..536d7c674b04 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> if (!f2fs_is_atomic_file(inode))
> return;
>
> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> - iput(fi->cow_inode);
> - fi->cow_inode = NULL;
> release_atomic_write_cnt(inode);
> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1f812b9ce985..10463f084d30 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> atomic_inc(&inode->i_count);
> spin_unlock(&inode->i_lock);
>
> - f2fs_abort_atomic_write(inode, true);
In order to avoid caching obsolete page of cow_inode, how about truncating
them here?
if (f2fs_is_atomic_file() && cow_inode)
truncate_inode_pages_final(&cow_inode->i_data);
Thanks,
> -
> /* should remain fi->extent_tree for writepage */
> f2fs_destroy_extent_node(inode);
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-28 2:07 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-01-28 2:07 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team
Cc: Daeho Jeong, syzbot+823000d23b3400619f7c
Hi Daeho, Jaegeuk,
Please take a look at patchset in below link:
https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
know your preference. :)
One comment as below.
On 2023/1/13 8:49, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
>
> To fix a race condition between atomic write aborts, I use the inode
> lock and make COW inode to be re-usable thoroughout the whole
> atomic file inode lifetime.
>
> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> fs/f2fs/inode.c | 11 +++++++++--
> fs/f2fs/segment.c | 3 ---
> fs/f2fs/super.c | 2 --
> 4 files changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index ecbc8c135b49..ff072a9ed258 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> atomic_read(&inode->i_writecount) != 1)
> return 0;
>
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> +
> return 0;
> }
>
> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> * until all the writers close its file. Since this should be done
> * before dropping file lock, it needs to do in ->flush.
> */
> - if (F2FS_I(inode)->atomic_write_task == current)
> + if (F2FS_I(inode)->atomic_write_task == current) {
> + inode_lock(inode);
> f2fs_abort_atomic_write(inode, true);
> + inode_unlock(inode);
> + }
> return 0;
> }
>
> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> goto out;
> }
>
> - /* Create a COW inode for atomic write */
> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> - if (IS_ERR(pinode)) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - ret = PTR_ERR(pinode);
> - goto out;
> - }
> + /* Check if the inode already has a COW inode */
> + if (fi->cow_inode == NULL) {
> + /* Create a COW inode for atomic write */
> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> + if (IS_ERR(pinode)) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + ret = PTR_ERR(pinode);
> + goto out;
> + }
>
> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> - iput(pinode);
> - if (ret) {
> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> - goto out;
> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> + iput(pinode);
> + if (ret) {
> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> + goto out;
> + }
> +
> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> + } else {
> + /* Reuse the already created COW inode */
> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> }
>
> f2fs_write_inode(inode, NULL);
> @@ -2107,8 +2122,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);
>
> isize = i_size_read(inode);
> fi->original_i_size = isize;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index ff6cf66ed46b..4921f7209e28 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> void f2fs_evict_inode(struct inode *inode)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> + struct f2fs_inode_info *fi = F2FS_I(inode);
> + nid_t xnid = fi->i_xattr_nid;
> int err = 0;
>
> f2fs_abort_atomic_write(inode, true);
>
> + if (fi->cow_inode) {
> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> + iput(fi->cow_inode);
> + fi->cow_inode = NULL;
> + }
> +
> trace_f2fs_evict_inode(inode);
> truncate_inode_pages_final(&inode->i_data);
>
> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> stat_dec_inline_inode(inode);
> stat_dec_compr_inode(inode);
> stat_sub_compr_blocks(inode,
> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> + atomic_read(&fi->i_compr_blocks));
>
> if (likely(!f2fs_cp_error(sbi) &&
> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index ae3c4e5474ef..536d7c674b04 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> if (!f2fs_is_atomic_file(inode))
> return;
>
> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> - iput(fi->cow_inode);
> - fi->cow_inode = NULL;
> release_atomic_write_cnt(inode);
> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1f812b9ce985..10463f084d30 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> atomic_inc(&inode->i_count);
> spin_unlock(&inode->i_lock);
>
> - f2fs_abort_atomic_write(inode, true);
In order to avoid caching obsolete page of cow_inode, how about truncating
them here?
if (f2fs_is_atomic_file() && cow_inode)
truncate_inode_pages_final(&cow_inode->i_data);
Thanks,
> -
> /* should remain fi->extent_tree for writepage */
> f2fs_destroy_extent_node(inode);
>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-28 2:07 ` Chao Yu
@ 2023-01-30 16:34 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-30 16:34 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
Hi Chao,
I read your patch series now and I like it.
However, how about a race condition between start_atomic_write and
abort_atomic_write?
abort_atomic_write is called without inode_lock in closing filp scenarios.
What do you think about this?
Thanks,
On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
>
> Hi Daeho, Jaegeuk,
>
> Please take a look at patchset in below link:
>
> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
>
> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> know your preference. :)
>
> One comment as below.
>
> On 2023/1/13 8:49, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > To fix a race condition between atomic write aborts, I use the inode
> > lock and make COW inode to be re-usable thoroughout the whole
> > atomic file inode lifetime.
> >
> > Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > fs/f2fs/inode.c | 11 +++++++++--
> > fs/f2fs/segment.c | 3 ---
> > fs/f2fs/super.c | 2 --
> > 4 files changed, 37 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index ecbc8c135b49..ff072a9ed258 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > atomic_read(&inode->i_writecount) != 1)
> > return 0;
> >
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > +
> > return 0;
> > }
> >
> > @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > * until all the writers close its file. Since this should be done
> > * before dropping file lock, it needs to do in ->flush.
> > */
> > - if (F2FS_I(inode)->atomic_write_task == current)
> > + if (F2FS_I(inode)->atomic_write_task == current) {
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > + }
> > return 0;
> > }
> >
> > @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > goto out;
> > }
> >
> > - /* Create a COW inode for atomic write */
> > - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > - if (IS_ERR(pinode)) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - ret = PTR_ERR(pinode);
> > - goto out;
> > - }
> > + /* Check if the inode already has a COW inode */
> > + if (fi->cow_inode == NULL) {
> > + /* Create a COW inode for atomic write */
> > + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > + if (IS_ERR(pinode)) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + ret = PTR_ERR(pinode);
> > + goto out;
> > + }
> >
> > - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > - iput(pinode);
> > - if (ret) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - goto out;
> > + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > + iput(pinode);
> > + if (ret) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + goto out;
> > + }
> > +
> > + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > + } else {
> > + /* Reuse the already created COW inode */
> > + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > }
> >
> > f2fs_write_inode(inode, NULL);
> > @@ -2107,8 +2122,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);
> >
> > isize = i_size_read(inode);
> > fi->original_i_size = isize;
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index ff6cf66ed46b..4921f7209e28 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > void f2fs_evict_inode(struct inode *inode)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > + struct f2fs_inode_info *fi = F2FS_I(inode);
> > + nid_t xnid = fi->i_xattr_nid;
> > int err = 0;
> >
> > f2fs_abort_atomic_write(inode, true);
> >
> > + if (fi->cow_inode) {
> > + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + iput(fi->cow_inode);
> > + fi->cow_inode = NULL;
> > + }
> > +
> > trace_f2fs_evict_inode(inode);
> > truncate_inode_pages_final(&inode->i_data);
> >
> > @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > stat_dec_inline_inode(inode);
> > stat_dec_compr_inode(inode);
> > stat_sub_compr_blocks(inode,
> > - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > + atomic_read(&fi->i_compr_blocks));
> >
> > if (likely(!f2fs_cp_error(sbi) &&
> > !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index ae3c4e5474ef..536d7c674b04 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > if (!f2fs_is_atomic_file(inode))
> > return;
> >
> > - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > - iput(fi->cow_inode);
> > - fi->cow_inode = NULL;
> > release_atomic_write_cnt(inode);
> > clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 1f812b9ce985..10463f084d30 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > atomic_inc(&inode->i_count);
> > spin_unlock(&inode->i_lock);
> >
> > - f2fs_abort_atomic_write(inode, true);
>
> In order to avoid caching obsolete page of cow_inode, how about truncating
> them here?
>
> if (f2fs_is_atomic_file() && cow_inode)
> truncate_inode_pages_final(&cow_inode->i_data);
>
> Thanks,
>
> > -
> > /* should remain fi->extent_tree for writepage */
> > f2fs_destroy_extent_node(inode);
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-30 16:34 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-30 16:34 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
Hi Chao,
I read your patch series now and I like it.
However, how about a race condition between start_atomic_write and
abort_atomic_write?
abort_atomic_write is called without inode_lock in closing filp scenarios.
What do you think about this?
Thanks,
On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
>
> Hi Daeho, Jaegeuk,
>
> Please take a look at patchset in below link:
>
> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
>
> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> know your preference. :)
>
> One comment as below.
>
> On 2023/1/13 8:49, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > To fix a race condition between atomic write aborts, I use the inode
> > lock and make COW inode to be re-usable thoroughout the whole
> > atomic file inode lifetime.
> >
> > Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > fs/f2fs/inode.c | 11 +++++++++--
> > fs/f2fs/segment.c | 3 ---
> > fs/f2fs/super.c | 2 --
> > 4 files changed, 37 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index ecbc8c135b49..ff072a9ed258 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > atomic_read(&inode->i_writecount) != 1)
> > return 0;
> >
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > +
> > return 0;
> > }
> >
> > @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > * until all the writers close its file. Since this should be done
> > * before dropping file lock, it needs to do in ->flush.
> > */
> > - if (F2FS_I(inode)->atomic_write_task == current)
> > + if (F2FS_I(inode)->atomic_write_task == current) {
> > + inode_lock(inode);
> > f2fs_abort_atomic_write(inode, true);
> > + inode_unlock(inode);
> > + }
> > return 0;
> > }
> >
> > @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > goto out;
> > }
> >
> > - /* Create a COW inode for atomic write */
> > - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > - if (IS_ERR(pinode)) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - ret = PTR_ERR(pinode);
> > - goto out;
> > - }
> > + /* Check if the inode already has a COW inode */
> > + if (fi->cow_inode == NULL) {
> > + /* Create a COW inode for atomic write */
> > + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > + if (IS_ERR(pinode)) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + ret = PTR_ERR(pinode);
> > + goto out;
> > + }
> >
> > - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > - iput(pinode);
> > - if (ret) {
> > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > - goto out;
> > + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > + iput(pinode);
> > + if (ret) {
> > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > + goto out;
> > + }
> > +
> > + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > + } else {
> > + /* Reuse the already created COW inode */
> > + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > }
> >
> > f2fs_write_inode(inode, NULL);
> > @@ -2107,8 +2122,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);
> >
> > isize = i_size_read(inode);
> > fi->original_i_size = isize;
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index ff6cf66ed46b..4921f7209e28 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > void f2fs_evict_inode(struct inode *inode)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > + struct f2fs_inode_info *fi = F2FS_I(inode);
> > + nid_t xnid = fi->i_xattr_nid;
> > int err = 0;
> >
> > f2fs_abort_atomic_write(inode, true);
> >
> > + if (fi->cow_inode) {
> > + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > + iput(fi->cow_inode);
> > + fi->cow_inode = NULL;
> > + }
> > +
> > trace_f2fs_evict_inode(inode);
> > truncate_inode_pages_final(&inode->i_data);
> >
> > @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > stat_dec_inline_inode(inode);
> > stat_dec_compr_inode(inode);
> > stat_sub_compr_blocks(inode,
> > - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > + atomic_read(&fi->i_compr_blocks));
> >
> > if (likely(!f2fs_cp_error(sbi) &&
> > !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index ae3c4e5474ef..536d7c674b04 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > if (!f2fs_is_atomic_file(inode))
> > return;
> >
> > - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > - iput(fi->cow_inode);
> > - fi->cow_inode = NULL;
> > release_atomic_write_cnt(inode);
> > clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 1f812b9ce985..10463f084d30 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > atomic_inc(&inode->i_count);
> > spin_unlock(&inode->i_lock);
> >
> > - f2fs_abort_atomic_write(inode, true);
>
> In order to avoid caching obsolete page of cow_inode, how about truncating
> them here?
>
> if (f2fs_is_atomic_file() && cow_inode)
> truncate_inode_pages_final(&cow_inode->i_data);
>
> Thanks,
>
> > -
> > /* should remain fi->extent_tree for writepage */
> > f2fs_destroy_extent_node(inode);
> >
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-30 16:34 ` Daeho Jeong
@ 2023-01-31 11:37 ` Chao Yu
-1 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-01-31 11:37 UTC (permalink / raw)
To: Daeho Jeong
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
Hi Daeho,
On 2023/1/31 0:34, Daeho Jeong wrote:
> Hi Chao,
>
> I read your patch series now and I like it.
Thank you for checking the patches. :)
> However, how about a race condition between start_atomic_write and
> abort_atomic_write?
Yup, I noticed that issue, I guess we can avoid this race condition by
covering these two flows w/ i_atomic_sem.
> abort_atomic_write is called without inode_lock in closing filp scenarios.
> What do you think about this?
I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
page cache if atomic_write is committed or aborted to avoid caching obsolete page?
Thanks,
>
> Thanks,
>
>
> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
>>
>> Hi Daeho, Jaegeuk,
>>
>> Please take a look at patchset in below link:
>>
>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
>>
>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
>> know your preference. :)
>>
>> One comment as below.
>>
>> On 2023/1/13 8:49, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> To fix a race condition between atomic write aborts, I use the inode
>>> lock and make COW inode to be re-usable thoroughout the whole
>>> atomic file inode lifetime.
>>>
>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
>>> fs/f2fs/inode.c | 11 +++++++++--
>>> fs/f2fs/segment.c | 3 ---
>>> fs/f2fs/super.c | 2 --
>>> 4 files changed, 37 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index ecbc8c135b49..ff072a9ed258 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
>>> atomic_read(&inode->i_writecount) != 1)
>>> return 0;
>>>
>>> + inode_lock(inode);
>>> f2fs_abort_atomic_write(inode, true);
>>> + inode_unlock(inode);
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
>>> * until all the writers close its file. Since this should be done
>>> * before dropping file lock, it needs to do in ->flush.
>>> */
>>> - if (F2FS_I(inode)->atomic_write_task == current)
>>> + if (F2FS_I(inode)->atomic_write_task == current) {
>>> + inode_lock(inode);
>>> f2fs_abort_atomic_write(inode, true);
>>> + inode_unlock(inode);
>>> + }
>>> return 0;
>>> }
>>>
>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>>> goto out;
>>> }
>>>
>>> - /* Create a COW inode for atomic write */
>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>> - if (IS_ERR(pinode)) {
>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> - ret = PTR_ERR(pinode);
>>> - goto out;
>>> - }
>>> + /* Check if the inode already has a COW inode */
>>> + if (fi->cow_inode == NULL) {
>>> + /* Create a COW inode for atomic write */
>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>> + if (IS_ERR(pinode)) {
>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> + ret = PTR_ERR(pinode);
>>> + goto out;
>>> + }
>>>
>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>> - iput(pinode);
>>> - if (ret) {
>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> - goto out;
>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>> + iput(pinode);
>>> + if (ret) {
>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> + goto out;
>>> + }
>>> +
>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
>>> + } else {
>>> + /* Reuse the already created COW inode */
>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
>>> }
>>>
>>> f2fs_write_inode(inode, NULL);
>>> @@ -2107,8 +2122,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);
>>>
>>> isize = i_size_read(inode);
>>> fi->original_i_size = isize;
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index ff6cf66ed46b..4921f7209e28 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>>> void f2fs_evict_inode(struct inode *inode)
>>> {
>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
>>> + nid_t xnid = fi->i_xattr_nid;
>>> int err = 0;
>>>
>>> f2fs_abort_atomic_write(inode, true);
>>>
>>> + if (fi->cow_inode) {
>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> + iput(fi->cow_inode);
>>> + fi->cow_inode = NULL;
>>> + }
>>> +
>>> trace_f2fs_evict_inode(inode);
>>> truncate_inode_pages_final(&inode->i_data);
>>>
>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
>>> stat_dec_inline_inode(inode);
>>> stat_dec_compr_inode(inode);
>>> stat_sub_compr_blocks(inode,
>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
>>> + atomic_read(&fi->i_compr_blocks));
>>>
>>> if (likely(!f2fs_cp_error(sbi) &&
>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index ae3c4e5474ef..536d7c674b04 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>> if (!f2fs_is_atomic_file(inode))
>>> return;
>>>
>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> - iput(fi->cow_inode);
>>> - fi->cow_inode = NULL;
>>> release_atomic_write_cnt(inode);
>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 1f812b9ce985..10463f084d30 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
>>> atomic_inc(&inode->i_count);
>>> spin_unlock(&inode->i_lock);
>>>
>>> - f2fs_abort_atomic_write(inode, true);
>>
>> In order to avoid caching obsolete page of cow_inode, how about truncating
>> them here?
>>
>> if (f2fs_is_atomic_file() && cow_inode)
>> truncate_inode_pages_final(&cow_inode->i_data);
>>
>> Thanks,
>>
>>> -
>>> /* should remain fi->extent_tree for writepage */
>>> f2fs_destroy_extent_node(inode);
>>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-31 11:37 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-01-31 11:37 UTC (permalink / raw)
To: Daeho Jeong
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
Hi Daeho,
On 2023/1/31 0:34, Daeho Jeong wrote:
> Hi Chao,
>
> I read your patch series now and I like it.
Thank you for checking the patches. :)
> However, how about a race condition between start_atomic_write and
> abort_atomic_write?
Yup, I noticed that issue, I guess we can avoid this race condition by
covering these two flows w/ i_atomic_sem.
> abort_atomic_write is called without inode_lock in closing filp scenarios.
> What do you think about this?
I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
page cache if atomic_write is committed or aborted to avoid caching obsolete page?
Thanks,
>
> Thanks,
>
>
> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
>>
>> Hi Daeho, Jaegeuk,
>>
>> Please take a look at patchset in below link:
>>
>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
>>
>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
>> know your preference. :)
>>
>> One comment as below.
>>
>> On 2023/1/13 8:49, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> To fix a race condition between atomic write aborts, I use the inode
>>> lock and make COW inode to be re-usable thoroughout the whole
>>> atomic file inode lifetime.
>>>
>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
>>> fs/f2fs/inode.c | 11 +++++++++--
>>> fs/f2fs/segment.c | 3 ---
>>> fs/f2fs/super.c | 2 --
>>> 4 files changed, 37 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index ecbc8c135b49..ff072a9ed258 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
>>> atomic_read(&inode->i_writecount) != 1)
>>> return 0;
>>>
>>> + inode_lock(inode);
>>> f2fs_abort_atomic_write(inode, true);
>>> + inode_unlock(inode);
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
>>> * until all the writers close its file. Since this should be done
>>> * before dropping file lock, it needs to do in ->flush.
>>> */
>>> - if (F2FS_I(inode)->atomic_write_task == current)
>>> + if (F2FS_I(inode)->atomic_write_task == current) {
>>> + inode_lock(inode);
>>> f2fs_abort_atomic_write(inode, true);
>>> + inode_unlock(inode);
>>> + }
>>> return 0;
>>> }
>>>
>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>>> goto out;
>>> }
>>>
>>> - /* Create a COW inode for atomic write */
>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>> - if (IS_ERR(pinode)) {
>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> - ret = PTR_ERR(pinode);
>>> - goto out;
>>> - }
>>> + /* Check if the inode already has a COW inode */
>>> + if (fi->cow_inode == NULL) {
>>> + /* Create a COW inode for atomic write */
>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>> + if (IS_ERR(pinode)) {
>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> + ret = PTR_ERR(pinode);
>>> + goto out;
>>> + }
>>>
>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>> - iput(pinode);
>>> - if (ret) {
>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> - goto out;
>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>> + iput(pinode);
>>> + if (ret) {
>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>> + goto out;
>>> + }
>>> +
>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
>>> + } else {
>>> + /* Reuse the already created COW inode */
>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
>>> }
>>>
>>> f2fs_write_inode(inode, NULL);
>>> @@ -2107,8 +2122,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);
>>>
>>> isize = i_size_read(inode);
>>> fi->original_i_size = isize;
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index ff6cf66ed46b..4921f7209e28 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>>> void f2fs_evict_inode(struct inode *inode)
>>> {
>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
>>> + nid_t xnid = fi->i_xattr_nid;
>>> int err = 0;
>>>
>>> f2fs_abort_atomic_write(inode, true);
>>>
>>> + if (fi->cow_inode) {
>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> + iput(fi->cow_inode);
>>> + fi->cow_inode = NULL;
>>> + }
>>> +
>>> trace_f2fs_evict_inode(inode);
>>> truncate_inode_pages_final(&inode->i_data);
>>>
>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
>>> stat_dec_inline_inode(inode);
>>> stat_dec_compr_inode(inode);
>>> stat_sub_compr_blocks(inode,
>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
>>> + atomic_read(&fi->i_compr_blocks));
>>>
>>> if (likely(!f2fs_cp_error(sbi) &&
>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index ae3c4e5474ef..536d7c674b04 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>> if (!f2fs_is_atomic_file(inode))
>>> return;
>>>
>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>> - iput(fi->cow_inode);
>>> - fi->cow_inode = NULL;
>>> release_atomic_write_cnt(inode);
>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 1f812b9ce985..10463f084d30 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
>>> atomic_inc(&inode->i_count);
>>> spin_unlock(&inode->i_lock);
>>>
>>> - f2fs_abort_atomic_write(inode, true);
>>
>> In order to avoid caching obsolete page of cow_inode, how about truncating
>> them here?
>>
>> if (f2fs_is_atomic_file() && cow_inode)
>> truncate_inode_pages_final(&cow_inode->i_data);
>>
>> Thanks,
>>
>>> -
>>> /* should remain fi->extent_tree for writepage */
>>> f2fs_destroy_extent_node(inode);
>>>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-31 11:37 ` Chao Yu
@ 2023-01-31 19:13 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 19:13 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
Hi Chao,
On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
>
> Hi Daeho,
>
> On 2023/1/31 0:34, Daeho Jeong wrote:
> > Hi Chao,
> >
> > I read your patch series now and I like it.
>
> Thank you for checking the patches. :)
>
> > However, how about a race condition between start_atomic_write and
> > abort_atomic_write?
>
> Yup, I noticed that issue, I guess we can avoid this race condition by
> covering these two flows w/ i_atomic_sem.
>
> > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > What do you think about this?
>
> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> page cache if atomic_write is committed or aborted to avoid caching obsolete page?
It's better to put that part in f2fs_abort_atomic_write().
On top of that, maybe, we should move
f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
Thanks,
>
> Thanks,
>
> >
> > Thanks,
> >
> >
> > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> >>
> >> Hi Daeho, Jaegeuk,
> >>
> >> Please take a look at patchset in below link:
> >>
> >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> >>
> >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> >> know your preference. :)
> >>
> >> One comment as below.
> >>
> >> On 2023/1/13 8:49, Daeho Jeong wrote:
> >>> From: Daeho Jeong <daehojeong@google.com>
> >>>
> >>> To fix a race condition between atomic write aborts, I use the inode
> >>> lock and make COW inode to be re-usable thoroughout the whole
> >>> atomic file inode lifetime.
> >>>
> >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>> ---
> >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> >>> fs/f2fs/inode.c | 11 +++++++++--
> >>> fs/f2fs/segment.c | 3 ---
> >>> fs/f2fs/super.c | 2 --
> >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>> index ecbc8c135b49..ff072a9ed258 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> >>> atomic_read(&inode->i_writecount) != 1)
> >>> return 0;
> >>>
> >>> + inode_lock(inode);
> >>> f2fs_abort_atomic_write(inode, true);
> >>> + inode_unlock(inode);
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> >>> * until all the writers close its file. Since this should be done
> >>> * before dropping file lock, it needs to do in ->flush.
> >>> */
> >>> - if (F2FS_I(inode)->atomic_write_task == current)
> >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> >>> + inode_lock(inode);
> >>> f2fs_abort_atomic_write(inode, true);
> >>> + inode_unlock(inode);
> >>> + }
> >>> return 0;
> >>> }
> >>>
> >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> >>> goto out;
> >>> }
> >>>
> >>> - /* Create a COW inode for atomic write */
> >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>> - if (IS_ERR(pinode)) {
> >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> - ret = PTR_ERR(pinode);
> >>> - goto out;
> >>> - }
> >>> + /* Check if the inode already has a COW inode */
> >>> + if (fi->cow_inode == NULL) {
> >>> + /* Create a COW inode for atomic write */
> >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>> + if (IS_ERR(pinode)) {
> >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> + ret = PTR_ERR(pinode);
> >>> + goto out;
> >>> + }
> >>>
> >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>> - iput(pinode);
> >>> - if (ret) {
> >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> - goto out;
> >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>> + iput(pinode);
> >>> + if (ret) {
> >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> + goto out;
> >>> + }
> >>> +
> >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> >>> + } else {
> >>> + /* Reuse the already created COW inode */
> >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> >>> }
> >>>
> >>> f2fs_write_inode(inode, NULL);
> >>> @@ -2107,8 +2122,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);
> >>>
> >>> isize = i_size_read(inode);
> >>> fi->original_i_size = isize;
> >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >>> index ff6cf66ed46b..4921f7209e28 100644
> >>> --- a/fs/f2fs/inode.c
> >>> +++ b/fs/f2fs/inode.c
> >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >>> void f2fs_evict_inode(struct inode *inode)
> >>> {
> >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> >>> + nid_t xnid = fi->i_xattr_nid;
> >>> int err = 0;
> >>>
> >>> f2fs_abort_atomic_write(inode, true);
> >>>
> >>> + if (fi->cow_inode) {
> >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>> + iput(fi->cow_inode);
> >>> + fi->cow_inode = NULL;
> >>> + }
> >>> +
> >>> trace_f2fs_evict_inode(inode);
> >>> truncate_inode_pages_final(&inode->i_data);
> >>>
> >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> >>> stat_dec_inline_inode(inode);
> >>> stat_dec_compr_inode(inode);
> >>> stat_sub_compr_blocks(inode,
> >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> >>> + atomic_read(&fi->i_compr_blocks));
> >>>
> >>> if (likely(!f2fs_cp_error(sbi) &&
> >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index ae3c4e5474ef..536d7c674b04 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >>> if (!f2fs_is_atomic_file(inode))
> >>> return;
> >>>
> >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>> - iput(fi->cow_inode);
> >>> - fi->cow_inode = NULL;
> >>> release_atomic_write_cnt(inode);
> >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index 1f812b9ce985..10463f084d30 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> >>> atomic_inc(&inode->i_count);
> >>> spin_unlock(&inode->i_lock);
> >>>
> >>> - f2fs_abort_atomic_write(inode, true);
> >>
> >> In order to avoid caching obsolete page of cow_inode, how about truncating
> >> them here?
> >>
> >> if (f2fs_is_atomic_file() && cow_inode)
> >> truncate_inode_pages_final(&cow_inode->i_data);
> >>
> >> Thanks,
> >>
> >>> -
> >>> /* should remain fi->extent_tree for writepage */
> >>> f2fs_destroy_extent_node(inode);
> >>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-31 19:13 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 19:13 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
Hi Chao,
On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
>
> Hi Daeho,
>
> On 2023/1/31 0:34, Daeho Jeong wrote:
> > Hi Chao,
> >
> > I read your patch series now and I like it.
>
> Thank you for checking the patches. :)
>
> > However, how about a race condition between start_atomic_write and
> > abort_atomic_write?
>
> Yup, I noticed that issue, I guess we can avoid this race condition by
> covering these two flows w/ i_atomic_sem.
>
> > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > What do you think about this?
>
> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> page cache if atomic_write is committed or aborted to avoid caching obsolete page?
It's better to put that part in f2fs_abort_atomic_write().
On top of that, maybe, we should move
f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
Thanks,
>
> Thanks,
>
> >
> > Thanks,
> >
> >
> > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> >>
> >> Hi Daeho, Jaegeuk,
> >>
> >> Please take a look at patchset in below link:
> >>
> >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> >>
> >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> >> know your preference. :)
> >>
> >> One comment as below.
> >>
> >> On 2023/1/13 8:49, Daeho Jeong wrote:
> >>> From: Daeho Jeong <daehojeong@google.com>
> >>>
> >>> To fix a race condition between atomic write aborts, I use the inode
> >>> lock and make COW inode to be re-usable thoroughout the whole
> >>> atomic file inode lifetime.
> >>>
> >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>> ---
> >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> >>> fs/f2fs/inode.c | 11 +++++++++--
> >>> fs/f2fs/segment.c | 3 ---
> >>> fs/f2fs/super.c | 2 --
> >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>> index ecbc8c135b49..ff072a9ed258 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> >>> atomic_read(&inode->i_writecount) != 1)
> >>> return 0;
> >>>
> >>> + inode_lock(inode);
> >>> f2fs_abort_atomic_write(inode, true);
> >>> + inode_unlock(inode);
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> >>> * until all the writers close its file. Since this should be done
> >>> * before dropping file lock, it needs to do in ->flush.
> >>> */
> >>> - if (F2FS_I(inode)->atomic_write_task == current)
> >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> >>> + inode_lock(inode);
> >>> f2fs_abort_atomic_write(inode, true);
> >>> + inode_unlock(inode);
> >>> + }
> >>> return 0;
> >>> }
> >>>
> >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> >>> goto out;
> >>> }
> >>>
> >>> - /* Create a COW inode for atomic write */
> >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>> - if (IS_ERR(pinode)) {
> >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> - ret = PTR_ERR(pinode);
> >>> - goto out;
> >>> - }
> >>> + /* Check if the inode already has a COW inode */
> >>> + if (fi->cow_inode == NULL) {
> >>> + /* Create a COW inode for atomic write */
> >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>> + if (IS_ERR(pinode)) {
> >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> + ret = PTR_ERR(pinode);
> >>> + goto out;
> >>> + }
> >>>
> >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>> - iput(pinode);
> >>> - if (ret) {
> >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> - goto out;
> >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>> + iput(pinode);
> >>> + if (ret) {
> >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>> + goto out;
> >>> + }
> >>> +
> >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> >>> + } else {
> >>> + /* Reuse the already created COW inode */
> >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> >>> }
> >>>
> >>> f2fs_write_inode(inode, NULL);
> >>> @@ -2107,8 +2122,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);
> >>>
> >>> isize = i_size_read(inode);
> >>> fi->original_i_size = isize;
> >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >>> index ff6cf66ed46b..4921f7209e28 100644
> >>> --- a/fs/f2fs/inode.c
> >>> +++ b/fs/f2fs/inode.c
> >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >>> void f2fs_evict_inode(struct inode *inode)
> >>> {
> >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> >>> + nid_t xnid = fi->i_xattr_nid;
> >>> int err = 0;
> >>>
> >>> f2fs_abort_atomic_write(inode, true);
> >>>
> >>> + if (fi->cow_inode) {
> >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>> + iput(fi->cow_inode);
> >>> + fi->cow_inode = NULL;
> >>> + }
> >>> +
> >>> trace_f2fs_evict_inode(inode);
> >>> truncate_inode_pages_final(&inode->i_data);
> >>>
> >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> >>> stat_dec_inline_inode(inode);
> >>> stat_dec_compr_inode(inode);
> >>> stat_sub_compr_blocks(inode,
> >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> >>> + atomic_read(&fi->i_compr_blocks));
> >>>
> >>> if (likely(!f2fs_cp_error(sbi) &&
> >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index ae3c4e5474ef..536d7c674b04 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >>> if (!f2fs_is_atomic_file(inode))
> >>> return;
> >>>
> >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>> - iput(fi->cow_inode);
> >>> - fi->cow_inode = NULL;
> >>> release_atomic_write_cnt(inode);
> >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index 1f812b9ce985..10463f084d30 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> >>> atomic_inc(&inode->i_count);
> >>> spin_unlock(&inode->i_lock);
> >>>
> >>> - f2fs_abort_atomic_write(inode, true);
> >>
> >> In order to avoid caching obsolete page of cow_inode, how about truncating
> >> them here?
> >>
> >> if (f2fs_is_atomic_file() && cow_inode)
> >> truncate_inode_pages_final(&cow_inode->i_data);
> >>
> >> Thanks,
> >>
> >>> -
> >>> /* should remain fi->extent_tree for writepage */
> >>> f2fs_destroy_extent_node(inode);
> >>>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-31 19:13 ` Daeho Jeong
@ 2023-01-31 21:38 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 21:38 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
>
> Hi Chao,
>
> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> >
> > Hi Daeho,
> >
> > On 2023/1/31 0:34, Daeho Jeong wrote:
> > > Hi Chao,
> > >
> > > I read your patch series now and I like it.
> >
> > Thank you for checking the patches. :)
> >
> > > However, how about a race condition between start_atomic_write and
> > > abort_atomic_write?
> >
> > Yup, I noticed that issue, I guess we can avoid this race condition by
> > covering these two flows w/ i_atomic_sem.
> >
> > > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > > What do you think about this?
> >
> > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> > page cache if atomic_write is committed or aborted to avoid caching obsolete page?
>
> It's better to put that part in f2fs_abort_atomic_write().
> On top of that, maybe, we should move
> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
might be some left writeback after aborting atomic write.
Plz. review it related to the timing of calling truncate_inode_pages_final().
>
> Thanks,
>
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > >
> > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> > >>
> > >> Hi Daeho, Jaegeuk,
> > >>
> > >> Please take a look at patchset in below link:
> > >>
> > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> > >>
> > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> > >> know your preference. :)
> > >>
> > >> One comment as below.
> > >>
> > >> On 2023/1/13 8:49, Daeho Jeong wrote:
> > >>> From: Daeho Jeong <daehojeong@google.com>
> > >>>
> > >>> To fix a race condition between atomic write aborts, I use the inode
> > >>> lock and make COW inode to be re-usable thoroughout the whole
> > >>> atomic file inode lifetime.
> > >>>
> > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > >>> ---
> > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > >>> fs/f2fs/inode.c | 11 +++++++++--
> > >>> fs/f2fs/segment.c | 3 ---
> > >>> fs/f2fs/super.c | 2 --
> > >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > >>> index ecbc8c135b49..ff072a9ed258 100644
> > >>> --- a/fs/f2fs/file.c
> > >>> +++ b/fs/f2fs/file.c
> > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > >>> atomic_read(&inode->i_writecount) != 1)
> > >>> return 0;
> > >>>
> > >>> + inode_lock(inode);
> > >>> f2fs_abort_atomic_write(inode, true);
> > >>> + inode_unlock(inode);
> > >>> +
> > >>> return 0;
> > >>> }
> > >>>
> > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > >>> * until all the writers close its file. Since this should be done
> > >>> * before dropping file lock, it needs to do in ->flush.
> > >>> */
> > >>> - if (F2FS_I(inode)->atomic_write_task == current)
> > >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> > >>> + inode_lock(inode);
> > >>> f2fs_abort_atomic_write(inode, true);
> > >>> + inode_unlock(inode);
> > >>> + }
> > >>> return 0;
> > >>> }
> > >>>
> > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > >>> goto out;
> > >>> }
> > >>>
> > >>> - /* Create a COW inode for atomic write */
> > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > >>> - if (IS_ERR(pinode)) {
> > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> - ret = PTR_ERR(pinode);
> > >>> - goto out;
> > >>> - }
> > >>> + /* Check if the inode already has a COW inode */
> > >>> + if (fi->cow_inode == NULL) {
> > >>> + /* Create a COW inode for atomic write */
> > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > >>> + if (IS_ERR(pinode)) {
> > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> + ret = PTR_ERR(pinode);
> > >>> + goto out;
> > >>> + }
> > >>>
> > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > >>> - iput(pinode);
> > >>> - if (ret) {
> > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> - goto out;
> > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > >>> + iput(pinode);
> > >>> + if (ret) {
> > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> + goto out;
> > >>> + }
> > >>> +
> > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > >>> + } else {
> > >>> + /* Reuse the already created COW inode */
> > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > >>> }
> > >>>
> > >>> f2fs_write_inode(inode, NULL);
> > >>> @@ -2107,8 +2122,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);
> > >>>
> > >>> isize = i_size_read(inode);
> > >>> fi->original_i_size = isize;
> > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > >>> index ff6cf66ed46b..4921f7209e28 100644
> > >>> --- a/fs/f2fs/inode.c
> > >>> +++ b/fs/f2fs/inode.c
> > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > >>> void f2fs_evict_inode(struct inode *inode)
> > >>> {
> > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> > >>> + nid_t xnid = fi->i_xattr_nid;
> > >>> int err = 0;
> > >>>
> > >>> f2fs_abort_atomic_write(inode, true);
> > >>>
> > >>> + if (fi->cow_inode) {
> > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > >>> + iput(fi->cow_inode);
> > >>> + fi->cow_inode = NULL;
> > >>> + }
> > >>> +
> > >>> trace_f2fs_evict_inode(inode);
> > >>> truncate_inode_pages_final(&inode->i_data);
> > >>>
> > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > >>> stat_dec_inline_inode(inode);
> > >>> stat_dec_compr_inode(inode);
> > >>> stat_sub_compr_blocks(inode,
> > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > >>> + atomic_read(&fi->i_compr_blocks));
> > >>>
> > >>> if (likely(!f2fs_cp_error(sbi) &&
> > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > >>> index ae3c4e5474ef..536d7c674b04 100644
> > >>> --- a/fs/f2fs/segment.c
> > >>> +++ b/fs/f2fs/segment.c
> > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > >>> if (!f2fs_is_atomic_file(inode))
> > >>> return;
> > >>>
> > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > >>> - iput(fi->cow_inode);
> > >>> - fi->cow_inode = NULL;
> > >>> release_atomic_write_cnt(inode);
> > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > >>> index 1f812b9ce985..10463f084d30 100644
> > >>> --- a/fs/f2fs/super.c
> > >>> +++ b/fs/f2fs/super.c
> > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > >>> atomic_inc(&inode->i_count);
> > >>> spin_unlock(&inode->i_lock);
> > >>>
> > >>> - f2fs_abort_atomic_write(inode, true);
> > >>
> > >> In order to avoid caching obsolete page of cow_inode, how about truncating
> > >> them here?
> > >>
> > >> if (f2fs_is_atomic_file() && cow_inode)
> > >> truncate_inode_pages_final(&cow_inode->i_data);
> > >>
> > >> Thanks,
> > >>
> > >>> -
> > >>> /* should remain fi->extent_tree for writepage */
> > >>> f2fs_destroy_extent_node(inode);
> > >>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-31 21:38 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 21:38 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
>
> Hi Chao,
>
> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> >
> > Hi Daeho,
> >
> > On 2023/1/31 0:34, Daeho Jeong wrote:
> > > Hi Chao,
> > >
> > > I read your patch series now and I like it.
> >
> > Thank you for checking the patches. :)
> >
> > > However, how about a race condition between start_atomic_write and
> > > abort_atomic_write?
> >
> > Yup, I noticed that issue, I guess we can avoid this race condition by
> > covering these two flows w/ i_atomic_sem.
> >
> > > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > > What do you think about this?
> >
> > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> > page cache if atomic_write is committed or aborted to avoid caching obsolete page?
>
> It's better to put that part in f2fs_abort_atomic_write().
> On top of that, maybe, we should move
> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
might be some left writeback after aborting atomic write.
Plz. review it related to the timing of calling truncate_inode_pages_final().
>
> Thanks,
>
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > >
> > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> > >>
> > >> Hi Daeho, Jaegeuk,
> > >>
> > >> Please take a look at patchset in below link:
> > >>
> > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> > >>
> > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> > >> know your preference. :)
> > >>
> > >> One comment as below.
> > >>
> > >> On 2023/1/13 8:49, Daeho Jeong wrote:
> > >>> From: Daeho Jeong <daehojeong@google.com>
> > >>>
> > >>> To fix a race condition between atomic write aborts, I use the inode
> > >>> lock and make COW inode to be re-usable thoroughout the whole
> > >>> atomic file inode lifetime.
> > >>>
> > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > >>> ---
> > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > >>> fs/f2fs/inode.c | 11 +++++++++--
> > >>> fs/f2fs/segment.c | 3 ---
> > >>> fs/f2fs/super.c | 2 --
> > >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > >>> index ecbc8c135b49..ff072a9ed258 100644
> > >>> --- a/fs/f2fs/file.c
> > >>> +++ b/fs/f2fs/file.c
> > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > >>> atomic_read(&inode->i_writecount) != 1)
> > >>> return 0;
> > >>>
> > >>> + inode_lock(inode);
> > >>> f2fs_abort_atomic_write(inode, true);
> > >>> + inode_unlock(inode);
> > >>> +
> > >>> return 0;
> > >>> }
> > >>>
> > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > >>> * until all the writers close its file. Since this should be done
> > >>> * before dropping file lock, it needs to do in ->flush.
> > >>> */
> > >>> - if (F2FS_I(inode)->atomic_write_task == current)
> > >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> > >>> + inode_lock(inode);
> > >>> f2fs_abort_atomic_write(inode, true);
> > >>> + inode_unlock(inode);
> > >>> + }
> > >>> return 0;
> > >>> }
> > >>>
> > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > >>> goto out;
> > >>> }
> > >>>
> > >>> - /* Create a COW inode for atomic write */
> > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > >>> - if (IS_ERR(pinode)) {
> > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> - ret = PTR_ERR(pinode);
> > >>> - goto out;
> > >>> - }
> > >>> + /* Check if the inode already has a COW inode */
> > >>> + if (fi->cow_inode == NULL) {
> > >>> + /* Create a COW inode for atomic write */
> > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > >>> + if (IS_ERR(pinode)) {
> > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> + ret = PTR_ERR(pinode);
> > >>> + goto out;
> > >>> + }
> > >>>
> > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > >>> - iput(pinode);
> > >>> - if (ret) {
> > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> - goto out;
> > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > >>> + iput(pinode);
> > >>> + if (ret) {
> > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >>> + goto out;
> > >>> + }
> > >>> +
> > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > >>> + } else {
> > >>> + /* Reuse the already created COW inode */
> > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > >>> }
> > >>>
> > >>> f2fs_write_inode(inode, NULL);
> > >>> @@ -2107,8 +2122,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);
> > >>>
> > >>> isize = i_size_read(inode);
> > >>> fi->original_i_size = isize;
> > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > >>> index ff6cf66ed46b..4921f7209e28 100644
> > >>> --- a/fs/f2fs/inode.c
> > >>> +++ b/fs/f2fs/inode.c
> > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > >>> void f2fs_evict_inode(struct inode *inode)
> > >>> {
> > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> > >>> + nid_t xnid = fi->i_xattr_nid;
> > >>> int err = 0;
> > >>>
> > >>> f2fs_abort_atomic_write(inode, true);
> > >>>
> > >>> + if (fi->cow_inode) {
> > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > >>> + iput(fi->cow_inode);
> > >>> + fi->cow_inode = NULL;
> > >>> + }
> > >>> +
> > >>> trace_f2fs_evict_inode(inode);
> > >>> truncate_inode_pages_final(&inode->i_data);
> > >>>
> > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > >>> stat_dec_inline_inode(inode);
> > >>> stat_dec_compr_inode(inode);
> > >>> stat_sub_compr_blocks(inode,
> > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > >>> + atomic_read(&fi->i_compr_blocks));
> > >>>
> > >>> if (likely(!f2fs_cp_error(sbi) &&
> > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > >>> index ae3c4e5474ef..536d7c674b04 100644
> > >>> --- a/fs/f2fs/segment.c
> > >>> +++ b/fs/f2fs/segment.c
> > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > >>> if (!f2fs_is_atomic_file(inode))
> > >>> return;
> > >>>
> > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > >>> - iput(fi->cow_inode);
> > >>> - fi->cow_inode = NULL;
> > >>> release_atomic_write_cnt(inode);
> > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > >>> index 1f812b9ce985..10463f084d30 100644
> > >>> --- a/fs/f2fs/super.c
> > >>> +++ b/fs/f2fs/super.c
> > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > >>> atomic_inc(&inode->i_count);
> > >>> spin_unlock(&inode->i_lock);
> > >>>
> > >>> - f2fs_abort_atomic_write(inode, true);
> > >>
> > >> In order to avoid caching obsolete page of cow_inode, how about truncating
> > >> them here?
> > >>
> > >> if (f2fs_is_atomic_file() && cow_inode)
> > >> truncate_inode_pages_final(&cow_inode->i_data);
> > >>
> > >> Thanks,
> > >>
> > >>> -
> > >>> /* should remain fi->extent_tree for writepage */
> > >>> f2fs_destroy_extent_node(inode);
> > >>>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-31 21:38 ` Daeho Jeong
@ 2023-01-31 21:57 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 21:57 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
> >
> > Hi Chao,
> >
> > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> > >
> > > Hi Daeho,
> > >
> > > On 2023/1/31 0:34, Daeho Jeong wrote:
> > > > Hi Chao,
> > > >
> > > > I read your patch series now and I like it.
> > >
> > > Thank you for checking the patches. :)
> > >
> > > > However, how about a race condition between start_atomic_write and
> > > > abort_atomic_write?
> > >
> > > Yup, I noticed that issue, I guess we can avoid this race condition by
> > > covering these two flows w/ i_atomic_sem.
> > >
> > > > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > > > What do you think about this?
> > >
> > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> > > page cache if atomic_write is committed or aborted to avoid caching obsolete page?
> >
> > It's better to put that part in f2fs_abort_atomic_write().
> > On top of that, maybe, we should move
> > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
>
> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
> might be some left writeback after aborting atomic write.
> Plz. review it related to the timing of calling truncate_inode_pages_final().
Looks like the scenario becomes too complicated if I think about more
than one writer's scenario.
How about we check writecount in commit_atomic_write ioctl and return
EBUSY when it's not only one writer?
In that case, we can make the scenario simple and effective, and we
can release all the resources in abort_atomic_write().
>
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > >
> > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> > > >>
> > > >> Hi Daeho, Jaegeuk,
> > > >>
> > > >> Please take a look at patchset in below link:
> > > >>
> > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> > > >>
> > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> > > >> know your preference. :)
> > > >>
> > > >> One comment as below.
> > > >>
> > > >> On 2023/1/13 8:49, Daeho Jeong wrote:
> > > >>> From: Daeho Jeong <daehojeong@google.com>
> > > >>>
> > > >>> To fix a race condition between atomic write aborts, I use the inode
> > > >>> lock and make COW inode to be re-usable thoroughout the whole
> > > >>> atomic file inode lifetime.
> > > >>>
> > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > >>> ---
> > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > > >>> fs/f2fs/inode.c | 11 +++++++++--
> > > >>> fs/f2fs/segment.c | 3 ---
> > > >>> fs/f2fs/super.c | 2 --
> > > >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> > > >>>
> > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > >>> index ecbc8c135b49..ff072a9ed258 100644
> > > >>> --- a/fs/f2fs/file.c
> > > >>> +++ b/fs/f2fs/file.c
> > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > > >>> atomic_read(&inode->i_writecount) != 1)
> > > >>> return 0;
> > > >>>
> > > >>> + inode_lock(inode);
> > > >>> f2fs_abort_atomic_write(inode, true);
> > > >>> + inode_unlock(inode);
> > > >>> +
> > > >>> return 0;
> > > >>> }
> > > >>>
> > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > > >>> * until all the writers close its file. Since this should be done
> > > >>> * before dropping file lock, it needs to do in ->flush.
> > > >>> */
> > > >>> - if (F2FS_I(inode)->atomic_write_task == current)
> > > >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> > > >>> + inode_lock(inode);
> > > >>> f2fs_abort_atomic_write(inode, true);
> > > >>> + inode_unlock(inode);
> > > >>> + }
> > > >>> return 0;
> > > >>> }
> > > >>>
> > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > > >>> goto out;
> > > >>> }
> > > >>>
> > > >>> - /* Create a COW inode for atomic write */
> > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > >>> - if (IS_ERR(pinode)) {
> > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> - ret = PTR_ERR(pinode);
> > > >>> - goto out;
> > > >>> - }
> > > >>> + /* Check if the inode already has a COW inode */
> > > >>> + if (fi->cow_inode == NULL) {
> > > >>> + /* Create a COW inode for atomic write */
> > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > >>> + if (IS_ERR(pinode)) {
> > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> + ret = PTR_ERR(pinode);
> > > >>> + goto out;
> > > >>> + }
> > > >>>
> > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > >>> - iput(pinode);
> > > >>> - if (ret) {
> > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> - goto out;
> > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > >>> + iput(pinode);
> > > >>> + if (ret) {
> > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> + goto out;
> > > >>> + }
> > > >>> +
> > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > > >>> + } else {
> > > >>> + /* Reuse the already created COW inode */
> > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > > >>> }
> > > >>>
> > > >>> f2fs_write_inode(inode, NULL);
> > > >>> @@ -2107,8 +2122,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);
> > > >>>
> > > >>> isize = i_size_read(inode);
> > > >>> fi->original_i_size = isize;
> > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > >>> index ff6cf66ed46b..4921f7209e28 100644
> > > >>> --- a/fs/f2fs/inode.c
> > > >>> +++ b/fs/f2fs/inode.c
> > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > > >>> void f2fs_evict_inode(struct inode *inode)
> > > >>> {
> > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> > > >>> + nid_t xnid = fi->i_xattr_nid;
> > > >>> int err = 0;
> > > >>>
> > > >>> f2fs_abort_atomic_write(inode, true);
> > > >>>
> > > >>> + if (fi->cow_inode) {
> > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > >>> + iput(fi->cow_inode);
> > > >>> + fi->cow_inode = NULL;
> > > >>> + }
> > > >>> +
> > > >>> trace_f2fs_evict_inode(inode);
> > > >>> truncate_inode_pages_final(&inode->i_data);
> > > >>>
> > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > > >>> stat_dec_inline_inode(inode);
> > > >>> stat_dec_compr_inode(inode);
> > > >>> stat_sub_compr_blocks(inode,
> > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > > >>> + atomic_read(&fi->i_compr_blocks));
> > > >>>
> > > >>> if (likely(!f2fs_cp_error(sbi) &&
> > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > >>> index ae3c4e5474ef..536d7c674b04 100644
> > > >>> --- a/fs/f2fs/segment.c
> > > >>> +++ b/fs/f2fs/segment.c
> > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > > >>> if (!f2fs_is_atomic_file(inode))
> > > >>> return;
> > > >>>
> > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > >>> - iput(fi->cow_inode);
> > > >>> - fi->cow_inode = NULL;
> > > >>> release_atomic_write_cnt(inode);
> > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > >>> index 1f812b9ce985..10463f084d30 100644
> > > >>> --- a/fs/f2fs/super.c
> > > >>> +++ b/fs/f2fs/super.c
> > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > > >>> atomic_inc(&inode->i_count);
> > > >>> spin_unlock(&inode->i_lock);
> > > >>>
> > > >>> - f2fs_abort_atomic_write(inode, true);
> > > >>
> > > >> In order to avoid caching obsolete page of cow_inode, how about truncating
> > > >> them here?
> > > >>
> > > >> if (f2fs_is_atomic_file() && cow_inode)
> > > >> truncate_inode_pages_final(&cow_inode->i_data);
> > > >>
> > > >> Thanks,
> > > >>
> > > >>> -
> > > >>> /* should remain fi->extent_tree for writepage */
> > > >>> f2fs_destroy_extent_node(inode);
> > > >>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-31 21:57 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 21:57 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
> >
> > Hi Chao,
> >
> > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> > >
> > > Hi Daeho,
> > >
> > > On 2023/1/31 0:34, Daeho Jeong wrote:
> > > > Hi Chao,
> > > >
> > > > I read your patch series now and I like it.
> > >
> > > Thank you for checking the patches. :)
> > >
> > > > However, how about a race condition between start_atomic_write and
> > > > abort_atomic_write?
> > >
> > > Yup, I noticed that issue, I guess we can avoid this race condition by
> > > covering these two flows w/ i_atomic_sem.
> > >
> > > > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > > > What do you think about this?
> > >
> > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> > > page cache if atomic_write is committed or aborted to avoid caching obsolete page?
> >
> > It's better to put that part in f2fs_abort_atomic_write().
> > On top of that, maybe, we should move
> > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
>
> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
> might be some left writeback after aborting atomic write.
> Plz. review it related to the timing of calling truncate_inode_pages_final().
Looks like the scenario becomes too complicated if I think about more
than one writer's scenario.
How about we check writecount in commit_atomic_write ioctl and return
EBUSY when it's not only one writer?
In that case, we can make the scenario simple and effective, and we
can release all the resources in abort_atomic_write().
>
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > >
> > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> > > >>
> > > >> Hi Daeho, Jaegeuk,
> > > >>
> > > >> Please take a look at patchset in below link:
> > > >>
> > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> > > >>
> > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> > > >> know your preference. :)
> > > >>
> > > >> One comment as below.
> > > >>
> > > >> On 2023/1/13 8:49, Daeho Jeong wrote:
> > > >>> From: Daeho Jeong <daehojeong@google.com>
> > > >>>
> > > >>> To fix a race condition between atomic write aborts, I use the inode
> > > >>> lock and make COW inode to be re-usable thoroughout the whole
> > > >>> atomic file inode lifetime.
> > > >>>
> > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > >>> ---
> > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > > >>> fs/f2fs/inode.c | 11 +++++++++--
> > > >>> fs/f2fs/segment.c | 3 ---
> > > >>> fs/f2fs/super.c | 2 --
> > > >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> > > >>>
> > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > >>> index ecbc8c135b49..ff072a9ed258 100644
> > > >>> --- a/fs/f2fs/file.c
> > > >>> +++ b/fs/f2fs/file.c
> > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > > >>> atomic_read(&inode->i_writecount) != 1)
> > > >>> return 0;
> > > >>>
> > > >>> + inode_lock(inode);
> > > >>> f2fs_abort_atomic_write(inode, true);
> > > >>> + inode_unlock(inode);
> > > >>> +
> > > >>> return 0;
> > > >>> }
> > > >>>
> > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > > >>> * until all the writers close its file. Since this should be done
> > > >>> * before dropping file lock, it needs to do in ->flush.
> > > >>> */
> > > >>> - if (F2FS_I(inode)->atomic_write_task == current)
> > > >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> > > >>> + inode_lock(inode);
> > > >>> f2fs_abort_atomic_write(inode, true);
> > > >>> + inode_unlock(inode);
> > > >>> + }
> > > >>> return 0;
> > > >>> }
> > > >>>
> > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > > >>> goto out;
> > > >>> }
> > > >>>
> > > >>> - /* Create a COW inode for atomic write */
> > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > >>> - if (IS_ERR(pinode)) {
> > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> - ret = PTR_ERR(pinode);
> > > >>> - goto out;
> > > >>> - }
> > > >>> + /* Check if the inode already has a COW inode */
> > > >>> + if (fi->cow_inode == NULL) {
> > > >>> + /* Create a COW inode for atomic write */
> > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > >>> + if (IS_ERR(pinode)) {
> > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> + ret = PTR_ERR(pinode);
> > > >>> + goto out;
> > > >>> + }
> > > >>>
> > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > >>> - iput(pinode);
> > > >>> - if (ret) {
> > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> - goto out;
> > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > >>> + iput(pinode);
> > > >>> + if (ret) {
> > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > >>> + goto out;
> > > >>> + }
> > > >>> +
> > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > > >>> + } else {
> > > >>> + /* Reuse the already created COW inode */
> > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > > >>> }
> > > >>>
> > > >>> f2fs_write_inode(inode, NULL);
> > > >>> @@ -2107,8 +2122,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);
> > > >>>
> > > >>> isize = i_size_read(inode);
> > > >>> fi->original_i_size = isize;
> > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > >>> index ff6cf66ed46b..4921f7209e28 100644
> > > >>> --- a/fs/f2fs/inode.c
> > > >>> +++ b/fs/f2fs/inode.c
> > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > > >>> void f2fs_evict_inode(struct inode *inode)
> > > >>> {
> > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> > > >>> + nid_t xnid = fi->i_xattr_nid;
> > > >>> int err = 0;
> > > >>>
> > > >>> f2fs_abort_atomic_write(inode, true);
> > > >>>
> > > >>> + if (fi->cow_inode) {
> > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > >>> + iput(fi->cow_inode);
> > > >>> + fi->cow_inode = NULL;
> > > >>> + }
> > > >>> +
> > > >>> trace_f2fs_evict_inode(inode);
> > > >>> truncate_inode_pages_final(&inode->i_data);
> > > >>>
> > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > > >>> stat_dec_inline_inode(inode);
> > > >>> stat_dec_compr_inode(inode);
> > > >>> stat_sub_compr_blocks(inode,
> > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > > >>> + atomic_read(&fi->i_compr_blocks));
> > > >>>
> > > >>> if (likely(!f2fs_cp_error(sbi) &&
> > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > >>> index ae3c4e5474ef..536d7c674b04 100644
> > > >>> --- a/fs/f2fs/segment.c
> > > >>> +++ b/fs/f2fs/segment.c
> > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > > >>> if (!f2fs_is_atomic_file(inode))
> > > >>> return;
> > > >>>
> > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > >>> - iput(fi->cow_inode);
> > > >>> - fi->cow_inode = NULL;
> > > >>> release_atomic_write_cnt(inode);
> > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > >>> index 1f812b9ce985..10463f084d30 100644
> > > >>> --- a/fs/f2fs/super.c
> > > >>> +++ b/fs/f2fs/super.c
> > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > > >>> atomic_inc(&inode->i_count);
> > > >>> spin_unlock(&inode->i_lock);
> > > >>>
> > > >>> - f2fs_abort_atomic_write(inode, true);
> > > >>
> > > >> In order to avoid caching obsolete page of cow_inode, how about truncating
> > > >> them here?
> > > >>
> > > >> if (f2fs_is_atomic_file() && cow_inode)
> > > >> truncate_inode_pages_final(&cow_inode->i_data);
> > > >>
> > > >> Thanks,
> > > >>
> > > >>> -
> > > >>> /* should remain fi->extent_tree for writepage */
> > > >>> f2fs_destroy_extent_node(inode);
> > > >>>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-31 21:57 ` Daeho Jeong
@ 2023-01-31 22:34 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 22:34 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
> > >
> > > Hi Chao,
> > >
> > > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> > > >
> > > > Hi Daeho,
> > > >
> > > > On 2023/1/31 0:34, Daeho Jeong wrote:
> > > > > Hi Chao,
> > > > >
> > > > > I read your patch series now and I like it.
> > > >
> > > > Thank you for checking the patches. :)
> > > >
> > > > > However, how about a race condition between start_atomic_write and
> > > > > abort_atomic_write?
> > > >
> > > > Yup, I noticed that issue, I guess we can avoid this race condition by
> > > > covering these two flows w/ i_atomic_sem.
> > > >
> > > > > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > > > > What do you think about this?
> > > >
> > > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> > > > page cache if atomic_write is committed or aborted to avoid caching obsolete page?
> > >
> > > It's better to put that part in f2fs_abort_atomic_write().
> > > On top of that, maybe, we should move
> > > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> > > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
> >
> > Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
> > might be some left writeback after aborting atomic write.
> > Plz. review it related to the timing of calling truncate_inode_pages_final().
>
> Looks like the scenario becomes too complicated if I think about more
> than one writer's scenario.
> How about we check writecount in commit_atomic_write ioctl and return
> EBUSY when it's not only one writer?
> In that case, we can make the scenario simple and effective, and we
> can release all the resources in abort_atomic_write().
Oh, I totally forgot this. We don't use pages of COW inode. So, we
don't need to clean them up.
>
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > >
> > > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> > > > >>
> > > > >> Hi Daeho, Jaegeuk,
> > > > >>
> > > > >> Please take a look at patchset in below link:
> > > > >>
> > > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> > > > >>
> > > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> > > > >> know your preference. :)
> > > > >>
> > > > >> One comment as below.
> > > > >>
> > > > >> On 2023/1/13 8:49, Daeho Jeong wrote:
> > > > >>> From: Daeho Jeong <daehojeong@google.com>
> > > > >>>
> > > > >>> To fix a race condition between atomic write aborts, I use the inode
> > > > >>> lock and make COW inode to be re-usable thoroughout the whole
> > > > >>> atomic file inode lifetime.
> > > > >>>
> > > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > >>> ---
> > > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > > > >>> fs/f2fs/inode.c | 11 +++++++++--
> > > > >>> fs/f2fs/segment.c | 3 ---
> > > > >>> fs/f2fs/super.c | 2 --
> > > > >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> > > > >>>
> > > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > >>> index ecbc8c135b49..ff072a9ed258 100644
> > > > >>> --- a/fs/f2fs/file.c
> > > > >>> +++ b/fs/f2fs/file.c
> > > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > > > >>> atomic_read(&inode->i_writecount) != 1)
> > > > >>> return 0;
> > > > >>>
> > > > >>> + inode_lock(inode);
> > > > >>> f2fs_abort_atomic_write(inode, true);
> > > > >>> + inode_unlock(inode);
> > > > >>> +
> > > > >>> return 0;
> > > > >>> }
> > > > >>>
> > > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > > > >>> * until all the writers close its file. Since this should be done
> > > > >>> * before dropping file lock, it needs to do in ->flush.
> > > > >>> */
> > > > >>> - if (F2FS_I(inode)->atomic_write_task == current)
> > > > >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> > > > >>> + inode_lock(inode);
> > > > >>> f2fs_abort_atomic_write(inode, true);
> > > > >>> + inode_unlock(inode);
> > > > >>> + }
> > > > >>> return 0;
> > > > >>> }
> > > > >>>
> > > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > > > >>> goto out;
> > > > >>> }
> > > > >>>
> > > > >>> - /* Create a COW inode for atomic write */
> > > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > > >>> - if (IS_ERR(pinode)) {
> > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> - ret = PTR_ERR(pinode);
> > > > >>> - goto out;
> > > > >>> - }
> > > > >>> + /* Check if the inode already has a COW inode */
> > > > >>> + if (fi->cow_inode == NULL) {
> > > > >>> + /* Create a COW inode for atomic write */
> > > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > > >>> + if (IS_ERR(pinode)) {
> > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> + ret = PTR_ERR(pinode);
> > > > >>> + goto out;
> > > > >>> + }
> > > > >>>
> > > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > > >>> - iput(pinode);
> > > > >>> - if (ret) {
> > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> - goto out;
> > > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > > >>> + iput(pinode);
> > > > >>> + if (ret) {
> > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> + goto out;
> > > > >>> + }
> > > > >>> +
> > > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > > > >>> + } else {
> > > > >>> + /* Reuse the already created COW inode */
> > > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > > > >>> }
> > > > >>>
> > > > >>> f2fs_write_inode(inode, NULL);
> > > > >>> @@ -2107,8 +2122,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);
> > > > >>>
> > > > >>> isize = i_size_read(inode);
> > > > >>> fi->original_i_size = isize;
> > > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > >>> index ff6cf66ed46b..4921f7209e28 100644
> > > > >>> --- a/fs/f2fs/inode.c
> > > > >>> +++ b/fs/f2fs/inode.c
> > > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > > > >>> void f2fs_evict_inode(struct inode *inode)
> > > > >>> {
> > > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> > > > >>> + nid_t xnid = fi->i_xattr_nid;
> > > > >>> int err = 0;
> > > > >>>
> > > > >>> f2fs_abort_atomic_write(inode, true);
> > > > >>>
> > > > >>> + if (fi->cow_inode) {
> > > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > > >>> + iput(fi->cow_inode);
> > > > >>> + fi->cow_inode = NULL;
> > > > >>> + }
> > > > >>> +
> > > > >>> trace_f2fs_evict_inode(inode);
> > > > >>> truncate_inode_pages_final(&inode->i_data);
> > > > >>>
> > > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > > > >>> stat_dec_inline_inode(inode);
> > > > >>> stat_dec_compr_inode(inode);
> > > > >>> stat_sub_compr_blocks(inode,
> > > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > > > >>> + atomic_read(&fi->i_compr_blocks));
> > > > >>>
> > > > >>> if (likely(!f2fs_cp_error(sbi) &&
> > > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > >>> index ae3c4e5474ef..536d7c674b04 100644
> > > > >>> --- a/fs/f2fs/segment.c
> > > > >>> +++ b/fs/f2fs/segment.c
> > > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > > > >>> if (!f2fs_is_atomic_file(inode))
> > > > >>> return;
> > > > >>>
> > > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > > >>> - iput(fi->cow_inode);
> > > > >>> - fi->cow_inode = NULL;
> > > > >>> release_atomic_write_cnt(inode);
> > > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > >>> index 1f812b9ce985..10463f084d30 100644
> > > > >>> --- a/fs/f2fs/super.c
> > > > >>> +++ b/fs/f2fs/super.c
> > > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > > > >>> atomic_inc(&inode->i_count);
> > > > >>> spin_unlock(&inode->i_lock);
> > > > >>>
> > > > >>> - f2fs_abort_atomic_write(inode, true);
> > > > >>
> > > > >> In order to avoid caching obsolete page of cow_inode, how about truncating
> > > > >> them here?
> > > > >>
> > > > >> if (f2fs_is_atomic_file() && cow_inode)
> > > > >> truncate_inode_pages_final(&cow_inode->i_data);
> > > > >>
> > > > >> Thanks,
> > > > >>
> > > > >>> -
> > > > >>> /* should remain fi->extent_tree for writepage */
> > > > >>> f2fs_destroy_extent_node(inode);
> > > > >>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-01-31 22:34 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-01-31 22:34 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
> > >
> > > Hi Chao,
> > >
> > > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> > > >
> > > > Hi Daeho,
> > > >
> > > > On 2023/1/31 0:34, Daeho Jeong wrote:
> > > > > Hi Chao,
> > > > >
> > > > > I read your patch series now and I like it.
> > > >
> > > > Thank you for checking the patches. :)
> > > >
> > > > > However, how about a race condition between start_atomic_write and
> > > > > abort_atomic_write?
> > > >
> > > > Yup, I noticed that issue, I guess we can avoid this race condition by
> > > > covering these two flows w/ i_atomic_sem.
> > > >
> > > > > abort_atomic_write is called without inode_lock in closing filp scenarios.
> > > > > What do you think about this?
> > > >
> > > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> > > > page cache if atomic_write is committed or aborted to avoid caching obsolete page?
> > >
> > > It's better to put that part in f2fs_abort_atomic_write().
> > > On top of that, maybe, we should move
> > > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> > > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
> >
> > Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
> > might be some left writeback after aborting atomic write.
> > Plz. review it related to the timing of calling truncate_inode_pages_final().
>
> Looks like the scenario becomes too complicated if I think about more
> than one writer's scenario.
> How about we check writecount in commit_atomic_write ioctl and return
> EBUSY when it's not only one writer?
> In that case, we can make the scenario simple and effective, and we
> can release all the resources in abort_atomic_write().
Oh, I totally forgot this. We don't use pages of COW inode. So, we
don't need to clean them up.
>
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > >
> > > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> > > > >>
> > > > >> Hi Daeho, Jaegeuk,
> > > > >>
> > > > >> Please take a look at patchset in below link:
> > > > >>
> > > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> > > > >>
> > > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> > > > >> know your preference. :)
> > > > >>
> > > > >> One comment as below.
> > > > >>
> > > > >> On 2023/1/13 8:49, Daeho Jeong wrote:
> > > > >>> From: Daeho Jeong <daehojeong@google.com>
> > > > >>>
> > > > >>> To fix a race condition between atomic write aborts, I use the inode
> > > > >>> lock and make COW inode to be re-usable thoroughout the whole
> > > > >>> atomic file inode lifetime.
> > > > >>>
> > > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> > > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> > > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > >>> ---
> > > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> > > > >>> fs/f2fs/inode.c | 11 +++++++++--
> > > > >>> fs/f2fs/segment.c | 3 ---
> > > > >>> fs/f2fs/super.c | 2 --
> > > > >>> 4 files changed, 37 insertions(+), 22 deletions(-)
> > > > >>>
> > > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > >>> index ecbc8c135b49..ff072a9ed258 100644
> > > > >>> --- a/fs/f2fs/file.c
> > > > >>> +++ b/fs/f2fs/file.c
> > > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> > > > >>> atomic_read(&inode->i_writecount) != 1)
> > > > >>> return 0;
> > > > >>>
> > > > >>> + inode_lock(inode);
> > > > >>> f2fs_abort_atomic_write(inode, true);
> > > > >>> + inode_unlock(inode);
> > > > >>> +
> > > > >>> return 0;
> > > > >>> }
> > > > >>>
> > > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> > > > >>> * until all the writers close its file. Since this should be done
> > > > >>> * before dropping file lock, it needs to do in ->flush.
> > > > >>> */
> > > > >>> - if (F2FS_I(inode)->atomic_write_task == current)
> > > > >>> + if (F2FS_I(inode)->atomic_write_task == current) {
> > > > >>> + inode_lock(inode);
> > > > >>> f2fs_abort_atomic_write(inode, true);
> > > > >>> + inode_unlock(inode);
> > > > >>> + }
> > > > >>> return 0;
> > > > >>> }
> > > > >>>
> > > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> > > > >>> goto out;
> > > > >>> }
> > > > >>>
> > > > >>> - /* Create a COW inode for atomic write */
> > > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > > >>> - if (IS_ERR(pinode)) {
> > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> - ret = PTR_ERR(pinode);
> > > > >>> - goto out;
> > > > >>> - }
> > > > >>> + /* Check if the inode already has a COW inode */
> > > > >>> + if (fi->cow_inode == NULL) {
> > > > >>> + /* Create a COW inode for atomic write */
> > > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> > > > >>> + if (IS_ERR(pinode)) {
> > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> + ret = PTR_ERR(pinode);
> > > > >>> + goto out;
> > > > >>> + }
> > > > >>>
> > > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > > >>> - iput(pinode);
> > > > >>> - if (ret) {
> > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> - goto out;
> > > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> > > > >>> + iput(pinode);
> > > > >>> + if (ret) {
> > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > > > >>> + goto out;
> > > > >>> + }
> > > > >>> +
> > > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> > > > >>> + } else {
> > > > >>> + /* Reuse the already created COW inode */
> > > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> > > > >>> }
> > > > >>>
> > > > >>> f2fs_write_inode(inode, NULL);
> > > > >>> @@ -2107,8 +2122,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);
> > > > >>>
> > > > >>> isize = i_size_read(inode);
> > > > >>> fi->original_i_size = isize;
> > > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > >>> index ff6cf66ed46b..4921f7209e28 100644
> > > > >>> --- a/fs/f2fs/inode.c
> > > > >>> +++ b/fs/f2fs/inode.c
> > > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> > > > >>> void f2fs_evict_inode(struct inode *inode)
> > > > >>> {
> > > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> > > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> > > > >>> + nid_t xnid = fi->i_xattr_nid;
> > > > >>> int err = 0;
> > > > >>>
> > > > >>> f2fs_abort_atomic_write(inode, true);
> > > > >>>
> > > > >>> + if (fi->cow_inode) {
> > > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > > >>> + iput(fi->cow_inode);
> > > > >>> + fi->cow_inode = NULL;
> > > > >>> + }
> > > > >>> +
> > > > >>> trace_f2fs_evict_inode(inode);
> > > > >>> truncate_inode_pages_final(&inode->i_data);
> > > > >>>
> > > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> > > > >>> stat_dec_inline_inode(inode);
> > > > >>> stat_dec_compr_inode(inode);
> > > > >>> stat_sub_compr_blocks(inode,
> > > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> > > > >>> + atomic_read(&fi->i_compr_blocks));
> > > > >>>
> > > > >>> if (likely(!f2fs_cp_error(sbi) &&
> > > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> > > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > >>> index ae3c4e5474ef..536d7c674b04 100644
> > > > >>> --- a/fs/f2fs/segment.c
> > > > >>> +++ b/fs/f2fs/segment.c
> > > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > > > >>> if (!f2fs_is_atomic_file(inode))
> > > > >>> return;
> > > > >>>
> > > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> > > > >>> - iput(fi->cow_inode);
> > > > >>> - fi->cow_inode = NULL;
> > > > >>> release_atomic_write_cnt(inode);
> > > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > >>> index 1f812b9ce985..10463f084d30 100644
> > > > >>> --- a/fs/f2fs/super.c
> > > > >>> +++ b/fs/f2fs/super.c
> > > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> > > > >>> atomic_inc(&inode->i_count);
> > > > >>> spin_unlock(&inode->i_lock);
> > > > >>>
> > > > >>> - f2fs_abort_atomic_write(inode, true);
> > > > >>
> > > > >> In order to avoid caching obsolete page of cow_inode, how about truncating
> > > > >> them here?
> > > > >>
> > > > >> if (f2fs_is_atomic_file() && cow_inode)
> > > > >> truncate_inode_pages_final(&cow_inode->i_data);
> > > > >>
> > > > >> Thanks,
> > > > >>
> > > > >>> -
> > > > >>> /* should remain fi->extent_tree for writepage */
> > > > >>> f2fs_destroy_extent_node(inode);
> > > > >>>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-01-31 22:34 ` Daeho Jeong
@ 2023-02-01 1:40 ` Chao Yu
-1 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-01 1:40 UTC (permalink / raw)
To: Daeho Jeong
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
Hi Daeho,
On 2023/2/1 6:34, Daeho Jeong wrote:
> On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote:
>>
>> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
>>>
>>> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
>>>>
>>>> Hi Chao,
>>>>
>>>> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
>>>>>
>>>>> Hi Daeho,
>>>>>
>>>>> On 2023/1/31 0:34, Daeho Jeong wrote:
>>>>>> Hi Chao,
>>>>>>
>>>>>> I read your patch series now and I like it.
>>>>>
>>>>> Thank you for checking the patches. :)
>>>>>
>>>>>> However, how about a race condition between start_atomic_write and
>>>>>> abort_atomic_write?
>>>>>
>>>>> Yup, I noticed that issue, I guess we can avoid this race condition by
>>>>> covering these two flows w/ i_atomic_sem.
>>>>>
>>>>>> abort_atomic_write is called without inode_lock in closing filp scenarios.
>>>>>> What do you think about this?
>>>>>
>>>>> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
>>>>> page cache if atomic_write is committed or aborted to avoid caching obsolete page?
>>>>
>>>> It's better to put that part in f2fs_abort_atomic_write().
>>>> On top of that, maybe, we should move
>>>> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
>>>> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
>>>
>>> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
>>> might be some left writeback after aborting atomic write.
>>> Plz. review it related to the timing of calling truncate_inode_pages_final().
>>
>> Looks like the scenario becomes too complicated if I think about more
>> than one writer's scenario.
>> How about we check writecount in commit_atomic_write ioctl and return
>> EBUSY when it's not only one writer?
>> In that case, we can make the scenario simple and effective, and we
>> can release all the resources in abort_atomic_write().
>
> Oh, I totally forgot this. We don't use pages of COW inode. So, we
Yes, after atomic_write is committed or aborted, we don't use pages of
cow_inode, so they are obsolete, IMO, we'd better to reclaim them
immediately to avoid unnecessary memory use.
The implementation may be:
- f2fs_ioc_abort_atomic_write
- f2fs_abort_atomic_write
- truncate_inode_pages_final(cow_inode)
- f2fs_ioc_commit_atomic_write
- f2fs_abort_atomic_write
- truncate_inode_pages_final(cow_inode)
Thanks,
> don't need to clean them up.
>
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>
>>>>>> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
>>>>>>>
>>>>>>> Hi Daeho, Jaegeuk,
>>>>>>>
>>>>>>> Please take a look at patchset in below link:
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
>>>>>>>
>>>>>>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
>>>>>>> know your preference. :)
>>>>>>>
>>>>>>> One comment as below.
>>>>>>>
>>>>>>> On 2023/1/13 8:49, Daeho Jeong wrote:
>>>>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>>>>
>>>>>>>> To fix a race condition between atomic write aborts, I use the inode
>>>>>>>> lock and make COW inode to be re-usable thoroughout the whole
>>>>>>>> atomic file inode lifetime.
>>>>>>>>
>>>>>>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
>>>>>>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>>>>> ---
>>>>>>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
>>>>>>>> fs/f2fs/inode.c | 11 +++++++++--
>>>>>>>> fs/f2fs/segment.c | 3 ---
>>>>>>>> fs/f2fs/super.c | 2 --
>>>>>>>> 4 files changed, 37 insertions(+), 22 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>>> index ecbc8c135b49..ff072a9ed258 100644
>>>>>>>> --- a/fs/f2fs/file.c
>>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
>>>>>>>> atomic_read(&inode->i_writecount) != 1)
>>>>>>>> return 0;
>>>>>>>>
>>>>>>>> + inode_lock(inode);
>>>>>>>> f2fs_abort_atomic_write(inode, true);
>>>>>>>> + inode_unlock(inode);
>>>>>>>> +
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
>>>>>>>> * until all the writers close its file. Since this should be done
>>>>>>>> * before dropping file lock, it needs to do in ->flush.
>>>>>>>> */
>>>>>>>> - if (F2FS_I(inode)->atomic_write_task == current)
>>>>>>>> + if (F2FS_I(inode)->atomic_write_task == current) {
>>>>>>>> + inode_lock(inode);
>>>>>>>> f2fs_abort_atomic_write(inode, true);
>>>>>>>> + inode_unlock(inode);
>>>>>>>> + }
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>>>>>>>> goto out;
>>>>>>>> }
>>>>>>>>
>>>>>>>> - /* Create a COW inode for atomic write */
>>>>>>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>>>>>>> - if (IS_ERR(pinode)) {
>>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> - ret = PTR_ERR(pinode);
>>>>>>>> - goto out;
>>>>>>>> - }
>>>>>>>> + /* Check if the inode already has a COW inode */
>>>>>>>> + if (fi->cow_inode == NULL) {
>>>>>>>> + /* Create a COW inode for atomic write */
>>>>>>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>>>>>>> + if (IS_ERR(pinode)) {
>>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> + ret = PTR_ERR(pinode);
>>>>>>>> + goto out;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>>>>>>> - iput(pinode);
>>>>>>>> - if (ret) {
>>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> - goto out;
>>>>>>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>>>>>>> + iput(pinode);
>>>>>>>> + if (ret) {
>>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> + goto out;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>>>>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
>>>>>>>> + } else {
>>>>>>>> + /* Reuse the already created COW inode */
>>>>>>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
>>>>>>>> }
>>>>>>>>
>>>>>>>> f2fs_write_inode(inode, NULL);
>>>>>>>> @@ -2107,8 +2122,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);
>>>>>>>>
>>>>>>>> isize = i_size_read(inode);
>>>>>>>> fi->original_i_size = isize;
>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>>> index ff6cf66ed46b..4921f7209e28 100644
>>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>>>>>>>> void f2fs_evict_inode(struct inode *inode)
>>>>>>>> {
>>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>>>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>>>>>>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
>>>>>>>> + nid_t xnid = fi->i_xattr_nid;
>>>>>>>> int err = 0;
>>>>>>>>
>>>>>>>> f2fs_abort_atomic_write(inode, true);
>>>>>>>>
>>>>>>>> + if (fi->cow_inode) {
>>>>>>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>>>>>> + iput(fi->cow_inode);
>>>>>>>> + fi->cow_inode = NULL;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> trace_f2fs_evict_inode(inode);
>>>>>>>> truncate_inode_pages_final(&inode->i_data);
>>>>>>>>
>>>>>>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
>>>>>>>> stat_dec_inline_inode(inode);
>>>>>>>> stat_dec_compr_inode(inode);
>>>>>>>> stat_sub_compr_blocks(inode,
>>>>>>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
>>>>>>>> + atomic_read(&fi->i_compr_blocks));
>>>>>>>>
>>>>>>>> if (likely(!f2fs_cp_error(sbi) &&
>>>>>>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
>>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>>>> index ae3c4e5474ef..536d7c674b04 100644
>>>>>>>> --- a/fs/f2fs/segment.c
>>>>>>>> +++ b/fs/f2fs/segment.c
>>>>>>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>>>>>>> if (!f2fs_is_atomic_file(inode))
>>>>>>>> return;
>>>>>>>>
>>>>>>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>>>>>> - iput(fi->cow_inode);
>>>>>>>> - fi->cow_inode = NULL;
>>>>>>>> release_atomic_write_cnt(inode);
>>>>>>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>>>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>>>> index 1f812b9ce985..10463f084d30 100644
>>>>>>>> --- a/fs/f2fs/super.c
>>>>>>>> +++ b/fs/f2fs/super.c
>>>>>>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
>>>>>>>> atomic_inc(&inode->i_count);
>>>>>>>> spin_unlock(&inode->i_lock);
>>>>>>>>
>>>>>>>> - f2fs_abort_atomic_write(inode, true);
>>>>>>>
>>>>>>> In order to avoid caching obsolete page of cow_inode, how about truncating
>>>>>>> them here?
>>>>>>>
>>>>>>> if (f2fs_is_atomic_file() && cow_inode)
>>>>>>> truncate_inode_pages_final(&cow_inode->i_data);
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>> -
>>>>>>>> /* should remain fi->extent_tree for writepage */
>>>>>>>> f2fs_destroy_extent_node(inode);
>>>>>>>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-01 1:40 ` Chao Yu
0 siblings, 0 replies; 31+ messages in thread
From: Chao Yu @ 2023-02-01 1:40 UTC (permalink / raw)
To: Daeho Jeong
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
Hi Daeho,
On 2023/2/1 6:34, Daeho Jeong wrote:
> On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote:
>>
>> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
>>>
>>> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
>>>>
>>>> Hi Chao,
>>>>
>>>> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
>>>>>
>>>>> Hi Daeho,
>>>>>
>>>>> On 2023/1/31 0:34, Daeho Jeong wrote:
>>>>>> Hi Chao,
>>>>>>
>>>>>> I read your patch series now and I like it.
>>>>>
>>>>> Thank you for checking the patches. :)
>>>>>
>>>>>> However, how about a race condition between start_atomic_write and
>>>>>> abort_atomic_write?
>>>>>
>>>>> Yup, I noticed that issue, I guess we can avoid this race condition by
>>>>> covering these two flows w/ i_atomic_sem.
>>>>>
>>>>>> abort_atomic_write is called without inode_lock in closing filp scenarios.
>>>>>> What do you think about this?
>>>>>
>>>>> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
>>>>> page cache if atomic_write is committed or aborted to avoid caching obsolete page?
>>>>
>>>> It's better to put that part in f2fs_abort_atomic_write().
>>>> On top of that, maybe, we should move
>>>> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
>>>> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
>>>
>>> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
>>> might be some left writeback after aborting atomic write.
>>> Plz. review it related to the timing of calling truncate_inode_pages_final().
>>
>> Looks like the scenario becomes too complicated if I think about more
>> than one writer's scenario.
>> How about we check writecount in commit_atomic_write ioctl and return
>> EBUSY when it's not only one writer?
>> In that case, we can make the scenario simple and effective, and we
>> can release all the resources in abort_atomic_write().
>
> Oh, I totally forgot this. We don't use pages of COW inode. So, we
Yes, after atomic_write is committed or aborted, we don't use pages of
cow_inode, so they are obsolete, IMO, we'd better to reclaim them
immediately to avoid unnecessary memory use.
The implementation may be:
- f2fs_ioc_abort_atomic_write
- f2fs_abort_atomic_write
- truncate_inode_pages_final(cow_inode)
- f2fs_ioc_commit_atomic_write
- f2fs_abort_atomic_write
- truncate_inode_pages_final(cow_inode)
Thanks,
> don't need to clean them up.
>
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>
>>>>>> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
>>>>>>>
>>>>>>> Hi Daeho, Jaegeuk,
>>>>>>>
>>>>>>> Please take a look at patchset in below link:
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
>>>>>>>
>>>>>>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
>>>>>>> know your preference. :)
>>>>>>>
>>>>>>> One comment as below.
>>>>>>>
>>>>>>> On 2023/1/13 8:49, Daeho Jeong wrote:
>>>>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>>>>
>>>>>>>> To fix a race condition between atomic write aborts, I use the inode
>>>>>>>> lock and make COW inode to be re-usable thoroughout the whole
>>>>>>>> atomic file inode lifetime.
>>>>>>>>
>>>>>>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
>>>>>>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>>>>> ---
>>>>>>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
>>>>>>>> fs/f2fs/inode.c | 11 +++++++++--
>>>>>>>> fs/f2fs/segment.c | 3 ---
>>>>>>>> fs/f2fs/super.c | 2 --
>>>>>>>> 4 files changed, 37 insertions(+), 22 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>>> index ecbc8c135b49..ff072a9ed258 100644
>>>>>>>> --- a/fs/f2fs/file.c
>>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
>>>>>>>> atomic_read(&inode->i_writecount) != 1)
>>>>>>>> return 0;
>>>>>>>>
>>>>>>>> + inode_lock(inode);
>>>>>>>> f2fs_abort_atomic_write(inode, true);
>>>>>>>> + inode_unlock(inode);
>>>>>>>> +
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
>>>>>>>> * until all the writers close its file. Since this should be done
>>>>>>>> * before dropping file lock, it needs to do in ->flush.
>>>>>>>> */
>>>>>>>> - if (F2FS_I(inode)->atomic_write_task == current)
>>>>>>>> + if (F2FS_I(inode)->atomic_write_task == current) {
>>>>>>>> + inode_lock(inode);
>>>>>>>> f2fs_abort_atomic_write(inode, true);
>>>>>>>> + inode_unlock(inode);
>>>>>>>> + }
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>>>>>>>> goto out;
>>>>>>>> }
>>>>>>>>
>>>>>>>> - /* Create a COW inode for atomic write */
>>>>>>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>>>>>>> - if (IS_ERR(pinode)) {
>>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> - ret = PTR_ERR(pinode);
>>>>>>>> - goto out;
>>>>>>>> - }
>>>>>>>> + /* Check if the inode already has a COW inode */
>>>>>>>> + if (fi->cow_inode == NULL) {
>>>>>>>> + /* Create a COW inode for atomic write */
>>>>>>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
>>>>>>>> + if (IS_ERR(pinode)) {
>>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> + ret = PTR_ERR(pinode);
>>>>>>>> + goto out;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>>>>>>> - iput(pinode);
>>>>>>>> - if (ret) {
>>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> - goto out;
>>>>>>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
>>>>>>>> + iput(pinode);
>>>>>>>> + if (ret) {
>>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>>> + goto out;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>>>>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
>>>>>>>> + } else {
>>>>>>>> + /* Reuse the already created COW inode */
>>>>>>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
>>>>>>>> }
>>>>>>>>
>>>>>>>> f2fs_write_inode(inode, NULL);
>>>>>>>> @@ -2107,8 +2122,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);
>>>>>>>>
>>>>>>>> isize = i_size_read(inode);
>>>>>>>> fi->original_i_size = isize;
>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>>> index ff6cf66ed46b..4921f7209e28 100644
>>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>>>>>>>> void f2fs_evict_inode(struct inode *inode)
>>>>>>>> {
>>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>>>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>>>>>>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
>>>>>>>> + nid_t xnid = fi->i_xattr_nid;
>>>>>>>> int err = 0;
>>>>>>>>
>>>>>>>> f2fs_abort_atomic_write(inode, true);
>>>>>>>>
>>>>>>>> + if (fi->cow_inode) {
>>>>>>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>>>>>> + iput(fi->cow_inode);
>>>>>>>> + fi->cow_inode = NULL;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> trace_f2fs_evict_inode(inode);
>>>>>>>> truncate_inode_pages_final(&inode->i_data);
>>>>>>>>
>>>>>>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
>>>>>>>> stat_dec_inline_inode(inode);
>>>>>>>> stat_dec_compr_inode(inode);
>>>>>>>> stat_sub_compr_blocks(inode,
>>>>>>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
>>>>>>>> + atomic_read(&fi->i_compr_blocks));
>>>>>>>>
>>>>>>>> if (likely(!f2fs_cp_error(sbi) &&
>>>>>>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
>>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>>>> index ae3c4e5474ef..536d7c674b04 100644
>>>>>>>> --- a/fs/f2fs/segment.c
>>>>>>>> +++ b/fs/f2fs/segment.c
>>>>>>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>>>>>>> if (!f2fs_is_atomic_file(inode))
>>>>>>>> return;
>>>>>>>>
>>>>>>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>>>>>> - iput(fi->cow_inode);
>>>>>>>> - fi->cow_inode = NULL;
>>>>>>>> release_atomic_write_cnt(inode);
>>>>>>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>>>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>>>> index 1f812b9ce985..10463f084d30 100644
>>>>>>>> --- a/fs/f2fs/super.c
>>>>>>>> +++ b/fs/f2fs/super.c
>>>>>>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
>>>>>>>> atomic_inc(&inode->i_count);
>>>>>>>> spin_unlock(&inode->i_lock);
>>>>>>>>
>>>>>>>> - f2fs_abort_atomic_write(inode, true);
>>>>>>>
>>>>>>> In order to avoid caching obsolete page of cow_inode, how about truncating
>>>>>>> them here?
>>>>>>>
>>>>>>> if (f2fs_is_atomic_file() && cow_inode)
>>>>>>> truncate_inode_pages_final(&cow_inode->i_data);
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>> -
>>>>>>>> /* should remain fi->extent_tree for writepage */
>>>>>>>> f2fs_destroy_extent_node(inode);
>>>>>>>>
_______________________________________________
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] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
2023-02-01 1:40 ` Chao Yu
@ 2023-02-01 4:33 ` Daeho Jeong
-1 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-02-01 4:33 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong,
syzbot+823000d23b3400619f7c
On Tue, Jan 31, 2023 at 5:40 PM Chao Yu <chao@kernel.org> wrote:
>
> Hi Daeho,
>
> On 2023/2/1 6:34, Daeho Jeong wrote:
> > On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote:
> >>
> >> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
> >>>
> >>> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
> >>>>
> >>>> Hi Chao,
> >>>>
> >>>> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> >>>>>
> >>>>> Hi Daeho,
> >>>>>
> >>>>> On 2023/1/31 0:34, Daeho Jeong wrote:
> >>>>>> Hi Chao,
> >>>>>>
> >>>>>> I read your patch series now and I like it.
> >>>>>
> >>>>> Thank you for checking the patches. :)
> >>>>>
> >>>>>> However, how about a race condition between start_atomic_write and
> >>>>>> abort_atomic_write?
> >>>>>
> >>>>> Yup, I noticed that issue, I guess we can avoid this race condition by
> >>>>> covering these two flows w/ i_atomic_sem.
> >>>>>
> >>>>>> abort_atomic_write is called without inode_lock in closing filp scenarios.
> >>>>>> What do you think about this?
> >>>>>
> >>>>> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> >>>>> page cache if atomic_write is committed or aborted to avoid caching obsolete page?
> >>>>
> >>>> It's better to put that part in f2fs_abort_atomic_write().
> >>>> On top of that, maybe, we should move
> >>>> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> >>>> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
> >>>
> >>> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
> >>> might be some left writeback after aborting atomic write.
> >>> Plz. review it related to the timing of calling truncate_inode_pages_final().
> >>
> >> Looks like the scenario becomes too complicated if I think about more
> >> than one writer's scenario.
> >> How about we check writecount in commit_atomic_write ioctl and return
> >> EBUSY when it's not only one writer?
> >> In that case, we can make the scenario simple and effective, and we
> >> can release all the resources in abort_atomic_write().
> >
> > Oh, I totally forgot this. We don't use pages of COW inode. So, we
>
> Yes, after atomic_write is committed or aborted, we don't use pages of
> cow_inode, so they are obsolete, IMO, we'd better to reclaim them
> immediately to avoid unnecessary memory use.
I mean we don't use page cache of the COW inode at all during the
whole atomic write procedure.
We only use the page cache of the original file.
>
> The implementation may be:
>
> - f2fs_ioc_abort_atomic_write
> - f2fs_abort_atomic_write
> - truncate_inode_pages_final(cow_inode)
>
> - f2fs_ioc_commit_atomic_write
> - f2fs_abort_atomic_write
> - truncate_inode_pages_final(cow_inode)
>
> Thanks,
>
> > don't need to clean them up.
> >
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>>
> >>>>>> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> >>>>>>>
> >>>>>>> Hi Daeho, Jaegeuk,
> >>>>>>>
> >>>>>>> Please take a look at patchset in below link:
> >>>>>>>
> >>>>>>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> >>>>>>>
> >>>>>>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> >>>>>>> know your preference. :)
> >>>>>>>
> >>>>>>> One comment as below.
> >>>>>>>
> >>>>>>> On 2023/1/13 8:49, Daeho Jeong wrote:
> >>>>>>>> From: Daeho Jeong <daehojeong@google.com>
> >>>>>>>>
> >>>>>>>> To fix a race condition between atomic write aborts, I use the inode
> >>>>>>>> lock and make COW inode to be re-usable thoroughout the whole
> >>>>>>>> atomic file inode lifetime.
> >>>>>>>>
> >>>>>>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> >>>>>>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> >>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>>>>>>> ---
> >>>>>>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> >>>>>>>> fs/f2fs/inode.c | 11 +++++++++--
> >>>>>>>> fs/f2fs/segment.c | 3 ---
> >>>>>>>> fs/f2fs/super.c | 2 --
> >>>>>>>> 4 files changed, 37 insertions(+), 22 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>>>>>>> index ecbc8c135b49..ff072a9ed258 100644
> >>>>>>>> --- a/fs/f2fs/file.c
> >>>>>>>> +++ b/fs/f2fs/file.c
> >>>>>>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> >>>>>>>> atomic_read(&inode->i_writecount) != 1)
> >>>>>>>> return 0;
> >>>>>>>>
> >>>>>>>> + inode_lock(inode);
> >>>>>>>> f2fs_abort_atomic_write(inode, true);
> >>>>>>>> + inode_unlock(inode);
> >>>>>>>> +
> >>>>>>>> return 0;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> >>>>>>>> * until all the writers close its file. Since this should be done
> >>>>>>>> * before dropping file lock, it needs to do in ->flush.
> >>>>>>>> */
> >>>>>>>> - if (F2FS_I(inode)->atomic_write_task == current)
> >>>>>>>> + if (F2FS_I(inode)->atomic_write_task == current) {
> >>>>>>>> + inode_lock(inode);
> >>>>>>>> f2fs_abort_atomic_write(inode, true);
> >>>>>>>> + inode_unlock(inode);
> >>>>>>>> + }
> >>>>>>>> return 0;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> >>>>>>>> goto out;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - /* Create a COW inode for atomic write */
> >>>>>>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>>>>>>> - if (IS_ERR(pinode)) {
> >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> - ret = PTR_ERR(pinode);
> >>>>>>>> - goto out;
> >>>>>>>> - }
> >>>>>>>> + /* Check if the inode already has a COW inode */
> >>>>>>>> + if (fi->cow_inode == NULL) {
> >>>>>>>> + /* Create a COW inode for atomic write */
> >>>>>>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>>>>>>> + if (IS_ERR(pinode)) {
> >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> + ret = PTR_ERR(pinode);
> >>>>>>>> + goto out;
> >>>>>>>> + }
> >>>>>>>>
> >>>>>>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>>>>>>> - iput(pinode);
> >>>>>>>> - if (ret) {
> >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> - goto out;
> >>>>>>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>>>>>>> + iput(pinode);
> >>>>>>>> + if (ret) {
> >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> + goto out;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> >>>>>>>> + } else {
> >>>>>>>> + /* Reuse the already created COW inode */
> >>>>>>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> f2fs_write_inode(inode, NULL);
> >>>>>>>> @@ -2107,8 +2122,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);
> >>>>>>>>
> >>>>>>>> isize = i_size_read(inode);
> >>>>>>>> fi->original_i_size = isize;
> >>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >>>>>>>> index ff6cf66ed46b..4921f7209e28 100644
> >>>>>>>> --- a/fs/f2fs/inode.c
> >>>>>>>> +++ b/fs/f2fs/inode.c
> >>>>>>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >>>>>>>> void f2fs_evict_inode(struct inode *inode)
> >>>>>>>> {
> >>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >>>>>>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> >>>>>>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> >>>>>>>> + nid_t xnid = fi->i_xattr_nid;
> >>>>>>>> int err = 0;
> >>>>>>>>
> >>>>>>>> f2fs_abort_atomic_write(inode, true);
> >>>>>>>>
> >>>>>>>> + if (fi->cow_inode) {
> >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>>>>>>> + iput(fi->cow_inode);
> >>>>>>>> + fi->cow_inode = NULL;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> trace_f2fs_evict_inode(inode);
> >>>>>>>> truncate_inode_pages_final(&inode->i_data);
> >>>>>>>>
> >>>>>>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> >>>>>>>> stat_dec_inline_inode(inode);
> >>>>>>>> stat_dec_compr_inode(inode);
> >>>>>>>> stat_sub_compr_blocks(inode,
> >>>>>>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> >>>>>>>> + atomic_read(&fi->i_compr_blocks));
> >>>>>>>>
> >>>>>>>> if (likely(!f2fs_cp_error(sbi) &&
> >>>>>>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> >>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>>>>>>> index ae3c4e5474ef..536d7c674b04 100644
> >>>>>>>> --- a/fs/f2fs/segment.c
> >>>>>>>> +++ b/fs/f2fs/segment.c
> >>>>>>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >>>>>>>> if (!f2fs_is_atomic_file(inode))
> >>>>>>>> return;
> >>>>>>>>
> >>>>>>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>>>>>>> - iput(fi->cow_inode);
> >>>>>>>> - fi->cow_inode = NULL;
> >>>>>>>> release_atomic_write_cnt(inode);
> >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>>>>>>> index 1f812b9ce985..10463f084d30 100644
> >>>>>>>> --- a/fs/f2fs/super.c
> >>>>>>>> +++ b/fs/f2fs/super.c
> >>>>>>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> >>>>>>>> atomic_inc(&inode->i_count);
> >>>>>>>> spin_unlock(&inode->i_lock);
> >>>>>>>>
> >>>>>>>> - f2fs_abort_atomic_write(inode, true);
> >>>>>>>
> >>>>>>> In order to avoid caching obsolete page of cow_inode, how about truncating
> >>>>>>> them here?
> >>>>>>>
> >>>>>>> if (f2fs_is_atomic_file() && cow_inode)
> >>>>>>> truncate_inode_pages_final(&cow_inode->i_data);
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>>> -
> >>>>>>>> /* should remain fi->extent_tree for writepage */
> >>>>>>>> f2fs_destroy_extent_node(inode);
> >>>>>>>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts
@ 2023-02-01 4:33 ` Daeho Jeong
0 siblings, 0 replies; 31+ messages in thread
From: Daeho Jeong @ 2023-02-01 4:33 UTC (permalink / raw)
To: Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel,
syzbot+823000d23b3400619f7c, linux-f2fs-devel
On Tue, Jan 31, 2023 at 5:40 PM Chao Yu <chao@kernel.org> wrote:
>
> Hi Daeho,
>
> On 2023/2/1 6:34, Daeho Jeong wrote:
> > On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote:
> >>
> >> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote:
> >>>
> >>> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote:
> >>>>
> >>>> Hi Chao,
> >>>>
> >>>> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote:
> >>>>>
> >>>>> Hi Daeho,
> >>>>>
> >>>>> On 2023/1/31 0:34, Daeho Jeong wrote:
> >>>>>> Hi Chao,
> >>>>>>
> >>>>>> I read your patch series now and I like it.
> >>>>>
> >>>>> Thank you for checking the patches. :)
> >>>>>
> >>>>>> However, how about a race condition between start_atomic_write and
> >>>>>> abort_atomic_write?
> >>>>>
> >>>>> Yup, I noticed that issue, I guess we can avoid this race condition by
> >>>>> covering these two flows w/ i_atomic_sem.
> >>>>>
> >>>>>> abort_atomic_write is called without inode_lock in closing filp scenarios.
> >>>>>> What do you think about this?
> >>>>>
> >>>>> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's
> >>>>> page cache if atomic_write is committed or aborted to avoid caching obsolete page?
> >>>>
> >>>> It's better to put that part in f2fs_abort_atomic_write().
> >>>> On top of that, maybe, we should move
> >>>> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from
> >>>> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too.
> >>>
> >>> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there
> >>> might be some left writeback after aborting atomic write.
> >>> Plz. review it related to the timing of calling truncate_inode_pages_final().
> >>
> >> Looks like the scenario becomes too complicated if I think about more
> >> than one writer's scenario.
> >> How about we check writecount in commit_atomic_write ioctl and return
> >> EBUSY when it's not only one writer?
> >> In that case, we can make the scenario simple and effective, and we
> >> can release all the resources in abort_atomic_write().
> >
> > Oh, I totally forgot this. We don't use pages of COW inode. So, we
>
> Yes, after atomic_write is committed or aborted, we don't use pages of
> cow_inode, so they are obsolete, IMO, we'd better to reclaim them
> immediately to avoid unnecessary memory use.
I mean we don't use page cache of the COW inode at all during the
whole atomic write procedure.
We only use the page cache of the original file.
>
> The implementation may be:
>
> - f2fs_ioc_abort_atomic_write
> - f2fs_abort_atomic_write
> - truncate_inode_pages_final(cow_inode)
>
> - f2fs_ioc_commit_atomic_write
> - f2fs_abort_atomic_write
> - truncate_inode_pages_final(cow_inode)
>
> Thanks,
>
> > don't need to clean them up.
> >
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>>
> >>>>>> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote:
> >>>>>>>
> >>>>>>> Hi Daeho, Jaegeuk,
> >>>>>>>
> >>>>>>> Please take a look at patchset in below link:
> >>>>>>>
> >>>>>>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t
> >>>>>>>
> >>>>>>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me
> >>>>>>> know your preference. :)
> >>>>>>>
> >>>>>>> One comment as below.
> >>>>>>>
> >>>>>>> On 2023/1/13 8:49, Daeho Jeong wrote:
> >>>>>>>> From: Daeho Jeong <daehojeong@google.com>
> >>>>>>>>
> >>>>>>>> To fix a race condition between atomic write aborts, I use the inode
> >>>>>>>> lock and make COW inode to be re-usable thoroughout the whole
> >>>>>>>> atomic file inode lifetime.
> >>>>>>>>
> >>>>>>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
> >>>>>>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> >>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>>>>>>> ---
> >>>>>>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++---------------
> >>>>>>>> fs/f2fs/inode.c | 11 +++++++++--
> >>>>>>>> fs/f2fs/segment.c | 3 ---
> >>>>>>>> fs/f2fs/super.c | 2 --
> >>>>>>>> 4 files changed, 37 insertions(+), 22 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>>>>>>> index ecbc8c135b49..ff072a9ed258 100644
> >>>>>>>> --- a/fs/f2fs/file.c
> >>>>>>>> +++ b/fs/f2fs/file.c
> >>>>>>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
> >>>>>>>> atomic_read(&inode->i_writecount) != 1)
> >>>>>>>> return 0;
> >>>>>>>>
> >>>>>>>> + inode_lock(inode);
> >>>>>>>> f2fs_abort_atomic_write(inode, true);
> >>>>>>>> + inode_unlock(inode);
> >>>>>>>> +
> >>>>>>>> return 0;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id)
> >>>>>>>> * until all the writers close its file. Since this should be done
> >>>>>>>> * before dropping file lock, it needs to do in ->flush.
> >>>>>>>> */
> >>>>>>>> - if (F2FS_I(inode)->atomic_write_task == current)
> >>>>>>>> + if (F2FS_I(inode)->atomic_write_task == current) {
> >>>>>>>> + inode_lock(inode);
> >>>>>>>> f2fs_abort_atomic_write(inode, true);
> >>>>>>>> + inode_unlock(inode);
> >>>>>>>> + }
> >>>>>>>> return 0;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
> >>>>>>>> goto out;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - /* Create a COW inode for atomic write */
> >>>>>>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>>>>>>> - if (IS_ERR(pinode)) {
> >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> - ret = PTR_ERR(pinode);
> >>>>>>>> - goto out;
> >>>>>>>> - }
> >>>>>>>> + /* Check if the inode already has a COW inode */
> >>>>>>>> + if (fi->cow_inode == NULL) {
> >>>>>>>> + /* Create a COW inode for atomic write */
> >>>>>>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino);
> >>>>>>>> + if (IS_ERR(pinode)) {
> >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> + ret = PTR_ERR(pinode);
> >>>>>>>> + goto out;
> >>>>>>>> + }
> >>>>>>>>
> >>>>>>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>>>>>>> - iput(pinode);
> >>>>>>>> - if (ret) {
> >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> - goto out;
> >>>>>>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode);
> >>>>>>>> + iput(pinode);
> >>>>>>>> + if (ret) {
> >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >>>>>>>> + goto out;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> >>>>>>>> + } else {
> >>>>>>>> + /* Reuse the already created COW inode */
> >>>>>>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> f2fs_write_inode(inode, NULL);
> >>>>>>>> @@ -2107,8 +2122,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);
> >>>>>>>>
> >>>>>>>> isize = i_size_read(inode);
> >>>>>>>> fi->original_i_size = isize;
> >>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >>>>>>>> index ff6cf66ed46b..4921f7209e28 100644
> >>>>>>>> --- a/fs/f2fs/inode.c
> >>>>>>>> +++ b/fs/f2fs/inode.c
> >>>>>>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >>>>>>>> void f2fs_evict_inode(struct inode *inode)
> >>>>>>>> {
> >>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >>>>>>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> >>>>>>>> + struct f2fs_inode_info *fi = F2FS_I(inode);
> >>>>>>>> + nid_t xnid = fi->i_xattr_nid;
> >>>>>>>> int err = 0;
> >>>>>>>>
> >>>>>>>> f2fs_abort_atomic_write(inode, true);
> >>>>>>>>
> >>>>>>>> + if (fi->cow_inode) {
> >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>>>>>>> + iput(fi->cow_inode);
> >>>>>>>> + fi->cow_inode = NULL;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> trace_f2fs_evict_inode(inode);
> >>>>>>>> truncate_inode_pages_final(&inode->i_data);
> >>>>>>>>
> >>>>>>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode)
> >>>>>>>> stat_dec_inline_inode(inode);
> >>>>>>>> stat_dec_compr_inode(inode);
> >>>>>>>> stat_sub_compr_blocks(inode,
> >>>>>>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks));
> >>>>>>>> + atomic_read(&fi->i_compr_blocks));
> >>>>>>>>
> >>>>>>>> if (likely(!f2fs_cp_error(sbi) &&
> >>>>>>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> >>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>>>>>>> index ae3c4e5474ef..536d7c674b04 100644
> >>>>>>>> --- a/fs/f2fs/segment.c
> >>>>>>>> +++ b/fs/f2fs/segment.c
> >>>>>>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >>>>>>>> if (!f2fs_is_atomic_file(inode))
> >>>>>>>> return;
> >>>>>>>>
> >>>>>>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >>>>>>>> - iput(fi->cow_inode);
> >>>>>>>> - fi->cow_inode = NULL;
> >>>>>>>> release_atomic_write_cnt(inode);
> >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>>>>>>> index 1f812b9ce985..10463f084d30 100644
> >>>>>>>> --- a/fs/f2fs/super.c
> >>>>>>>> +++ b/fs/f2fs/super.c
> >>>>>>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode)
> >>>>>>>> atomic_inc(&inode->i_count);
> >>>>>>>> spin_unlock(&inode->i_lock);
> >>>>>>>>
> >>>>>>>> - f2fs_abort_atomic_write(inode, true);
> >>>>>>>
> >>>>>>> In order to avoid caching obsolete page of cow_inode, how about truncating
> >>>>>>> them here?
> >>>>>>>
> >>>>>>> if (f2fs_is_atomic_file() && cow_inode)
> >>>>>>> truncate_inode_pages_final(&cow_inode->i_data);
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>>> -
> >>>>>>>> /* should remain fi->extent_tree for writepage */
> >>>>>>>> f2fs_destroy_extent_node(inode);
> >>>>>>>>
_______________________________________________
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] 31+ messages in thread
end of thread, other threads:[~2023-02-14 18:10 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-09 18:18 [PATCH] f2fs: synchronize atomic write aborts Daeho Jeong
2023-02-09 18:18 ` [f2fs-dev] " Daeho Jeong
2023-02-13 9:47 ` Chao Yu
2023-02-13 9:47 ` Chao Yu
2023-02-13 20:14 ` Daeho Jeong
2023-02-13 20:14 ` Daeho Jeong
2023-02-14 1:46 ` Chao Yu
2023-02-14 1:46 ` Chao Yu
2023-02-14 1:47 ` Chao Yu
2023-02-14 1:47 ` Chao Yu
2023-02-14 18:10 ` patchwork-bot+f2fs
2023-02-14 18:10 ` patchwork-bot+f2fs
-- strict thread matches above, loose matches on Subject: below --
2023-01-13 0:49 Daeho Jeong
2023-01-28 2:07 ` Chao Yu
2023-01-28 2:07 ` Chao Yu
2023-01-30 16:34 ` Daeho Jeong
2023-01-30 16:34 ` Daeho Jeong
2023-01-31 11:37 ` Chao Yu
2023-01-31 11:37 ` Chao Yu
2023-01-31 19:13 ` Daeho Jeong
2023-01-31 19:13 ` Daeho Jeong
2023-01-31 21:38 ` Daeho Jeong
2023-01-31 21:38 ` Daeho Jeong
2023-01-31 21:57 ` Daeho Jeong
2023-01-31 21:57 ` Daeho Jeong
2023-01-31 22:34 ` Daeho Jeong
2023-01-31 22:34 ` Daeho Jeong
2023-02-01 1:40 ` Chao Yu
2023-02-01 1:40 ` Chao Yu
2023-02-01 4:33 ` Daeho Jeong
2023-02-01 4:33 ` Daeho Jeong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.