Commit 3db1de0e582c ("f2fs: change the current atomic write way")
removed old tracepoints, but it missed to add new one, this patch
fixes to introduce trace_f2fs_replace_atomic_write_block to trace
atomic_write commit flow.
Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/segment.c | 3 +++
include/trace/events/f2fs.h | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 8b773f3eef3b..c6f6d0618164 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -255,6 +255,9 @@ static int __replace_atomic_write_block(struct inode *inode, pgoff_t index,
}
f2fs_put_dnode(&dn);
+
+ trace_f2fs_replace_atomic_write_block(inode, F2FS_I(inode)->cow_inode,
+ index, *old_addr, new_addr, recover);
return 0;
}
diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
index 3852085198fb..fe6bcf5f917d 100644
--- a/include/trace/events/f2fs.h
+++ b/include/trace/events/f2fs.h
@@ -1290,6 +1290,43 @@ DEFINE_EVENT(f2fs__page, f2fs_vm_page_mkwrite,
TP_ARGS(page, type)
);
+TRACE_EVENT(f2fs_replace_atomic_write_block,
+
+ TP_PROTO(struct inode *inode, struct inode *cow_inode, pgoff_t index,
+ block_t old_addr, block_t new_addr, bool recovery),
+
+ TP_ARGS(inode, cow_inode, index, old_addr, new_addr, recovery),
+
+ TP_STRUCT__entry(
+ __field(dev_t, dev)
+ __field(ino_t, ino)
+ __field(ino_t, cow_ino)
+ __field(pgoff_t, index)
+ __field(block_t, old_addr)
+ __field(block_t, new_addr)
+ __field(bool, recovery)
+ ),
+
+ TP_fast_assign(
+ __entry->dev = inode->i_sb->s_dev;
+ __entry->ino = inode->i_ino;
+ __entry->cow_ino = cow_inode->i_ino;
+ __entry->index = index;
+ __entry->old_addr = old_addr;
+ __entry->new_addr = new_addr;
+ __entry->recovery = recovery;
+ ),
+
+ TP_printk("dev = (%d,%d), ino = %lu, cow_ino = %lu, index = %lu, "
+ "old_addr = 0x%llx, new_addr = 0x%llx, recovery = %d",
+ show_dev_ino(__entry),
+ __entry->cow_ino,
+ (unsigned long)__entry->index,
+ (unsigned long long)__entry->old_addr,
+ (unsigned long long)__entry->new_addr,
+ __entry->recovery)
+);
+
TRACE_EVENT(f2fs_filemap_fault,
TP_PROTO(struct inode *inode, pgoff_t index, unsigned long ret),
--
2.25.1
Syzbot reports a kernel panic as below:
kernel BUG at fs/inode.c:1763!
RIP: 0010:iput+0x68/0x80 fs/inode.c:1763
Call Trace:
f2fs_abort_atomic_write+0xea/0x4f0 fs/f2fs/segment.c:196
f2fs_ioc_commit_atomic_write+0x19f/0x260 fs/f2fs/file.c:2157
__f2fs_ioctl+0x26f0/0xaaf0 fs/f2fs/file.c:4154
f2fs_ioctl+0x18e/0x220 fs/f2fs/file.c:4242
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl fs/ioctl.c:856 [inline]
__x64_sys_ioctl+0x197/0x210 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
The root cause is there may be race case in between f2fs_abort_atomic_write()
called from f2fs_file_flush() and f2fs_ioc_commit_atomic_write().
Thread A Thread B
- close
- close_fd
- filp_close
- f2fs_file_flush
- f2fs_ioctl
- f2fs_ioc_commit_atomic_write
- f2fs_abort_atomic_write
- iput(cow_inode)
- f2fs_evict_inode
- clear_inode
- inode->i_state = I_FREEING | I_CLEAR;
- f2fs_abort_atomic_write
- iput(cow_inode)
- BUG_ON(inode->i_state & I_CLEAR)
- cow_inode = NULL
- cow_inode = NULL
So let's cover f2fs_abort_atomic_write() with f2fs_inode_info.i_atomic_sem
lock to avoid this race condition.
Reported-by: [email protected]
Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/f2fs.h | 1 +
fs/f2fs/segment.c | 6 +++++-
fs/f2fs/super.c | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 70ed01aca6f6..fa41c0dad308 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -854,6 +854,7 @@ struct f2fs_inode_info {
struct extent_tree *extent_tree[NR_EXTENT_CACHES];
/* cached extent_tree entry */
struct inode *cow_inode; /* copy-on-write inode for atomic write */
+ struct f2fs_rwsem i_atomic_sem; /* protect atomic write context */
/* avoid racing between foreground op and gc */
struct f2fs_rwsem i_gc_rwsem[2];
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 5f7e42b355eb..245d7adef6c6 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -189,8 +189,10 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
{
struct f2fs_inode_info *fi = F2FS_I(inode);
+ f2fs_down_write(&fi->i_atomic_sem);
+
if (!f2fs_is_atomic_file(inode))
- return;
+ goto out_unlock;
clear_inode_flag(fi->cow_inode, FI_COW_FILE);
iput(fi->cow_inode);
@@ -208,6 +210,8 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
f2fs_i_size_write(inode, fi->original_i_size);
fi->original_i_size = 0;
}
+out_unlock:
+ f2fs_up_write(&fi->i_atomic_sem);
}
static int __replace_atomic_write_block(struct inode *inode, pgoff_t index,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 0e8b3e27fa14..3edc8630eb4b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1411,6 +1411,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
init_f2fs_rwsem(&fi->i_gc_rwsem[READ]);
init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
init_f2fs_rwsem(&fi->i_xattr_sem);
+ init_f2fs_rwsem(&fi->i_atomic_sem);
/* Will be used by directory only */
fi->i_dir_level = F2FS_SB(sb)->dir_level;
--
2.25.1
Thread A Kworker
- application crashs
- do_exit
- close_files
- filp_close
- flush (f2fs_file_flush)
- writepages
- f2fs_write_cache_pages
- f2fs_write_single_data_page
- f2fs_do_write_data_page
- check f2fs_is_atomic_file
- f2fs_abort_atomic_write
- check f2fs_is_atomic_file
- iput(cow_inode)
- cow_inode = NULL
- set_new_dnode(cow_inode)
Fix this issue by covering f2fs_do_write_data_page() with i_atomic_sem.
Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/data.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index c940da1c540f..1645b8a1b904 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2637,13 +2637,24 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
struct dnode_of_data dn;
struct node_info ni;
bool ipu_force = false;
+ bool atomic_locked = false;
int err = 0;
/* Use COW inode to make dnode_of_data for atomic write */
- if (f2fs_is_atomic_file(inode))
+ if (f2fs_is_atomic_file(inode)) {
+ f2fs_down_write(&F2FS_I(inode)->i_atomic_sem);
+ atomic_locked = true;
+
+ if (!f2fs_is_atomic_file(inode)) {
+ /* atomic write is aborted */
+ err = -ENOENT;
+ goto out_err;
+ }
+
set_new_dnode(&dn, F2FS_I(inode)->cow_inode, NULL, NULL, 0);
- else
+ } else {
set_new_dnode(&dn, inode, NULL, NULL, 0);
+ }
if (need_inplace_update(fio) &&
f2fs_lookup_read_extent_cache_block(inode, page->index,
@@ -2652,7 +2663,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
DATA_GENERIC_ENHANCE)) {
f2fs_handle_error(fio->sbi,
ERROR_INVALID_BLKADDR);
- return -EFSCORRUPTED;
+ err = -EFSCORRUPTED;
+ goto out_err;
}
ipu_force = true;
@@ -2661,8 +2673,10 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
}
/* Deadlock due to between page->lock and f2fs_lock_op */
- if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
- return -EAGAIN;
+ if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi)) {
+ err = -EAGAIN;
+ goto out_err;
+ }
err = f2fs_get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
if (err)
@@ -2710,6 +2724,9 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
set_inode_flag(inode, FI_UPDATE_WRITE);
}
trace_f2fs_do_write_data_page(fio->page, IPU);
+
+ if (atomic_locked)
+ f2fs_up_write(&F2FS_I(inode)->i_atomic_sem);
return err;
}
@@ -2747,6 +2764,9 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
out:
if (fio->need_lock == LOCK_REQ)
f2fs_unlock_op(fio->sbi);
+out_err:
+ if (atomic_locked)
+ f2fs_up_write(&F2FS_I(inode)->i_atomic_sem);
return err;
}
--
2.25.1
Otherwise, last .atomic_write_task will be remained in structure
f2fs_inode_info, resulting in aborting atomic_write accidentally
in race case. Meanwhile, clear original_i_size as well.
Fixes: 7a10f0177e11 ("f2fs: don't give partially written atomic data from process crash")
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/segment.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c6f6d0618164..5f7e42b355eb 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -201,9 +201,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
clear_inode_flag(inode, FI_ATOMIC_FILE);
stat_dec_atomic_inode(inode);
+ F2FS_I(inode)->atomic_write_task = NULL;
+
if (clean) {
truncate_inode_pages_final(inode->i_mapping);
f2fs_i_size_write(inode, fi->original_i_size);
+ fi->original_i_size = 0;
}
}
--
2.25.1
Commit 7a10f0177e11 ("f2fs: don't give partially written atomic data
from process crash") attempted to drop atomic write data after process
crash, however, f2fs_abort_atomic_write() may be called from noncrash
case, fix it by adding missed PF_EXITING check condition
f2fs_file_flush().
- application crashs
- do_exit
- exit_signals -- sets PF_EXITING
- exit_files
- put_files_struct
- close_files
- filp_close
- flush (f2fs_file_flush)
- check atomic_write_task && PF_EXITING
- f2fs_abort_atomic_write
Fixes: 7a10f0177e11 ("f2fs: don't give partially written atomic data from process crash")
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 434c0d89c145..7b62c533f6d3 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1876,7 +1876,8 @@ 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 &&
+ (current->flags & PF_EXITING))
f2fs_abort_atomic_write(inode, true);
return 0;
}
--
2.25.1
Hello:
This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <[email protected]>:
On Mon, 9 Jan 2023 11:44:49 +0800 you wrote:
> Commit 3db1de0e582c ("f2fs: change the current atomic write way")
> removed old tracepoints, but it missed to add new one, this patch
> fixes to introduce trace_f2fs_replace_atomic_write_block to trace
> atomic_write commit flow.
>
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Chao Yu <[email protected]>
>
> [...]
Here is the summary with links:
- [f2fs-dev,1/5] f2fs: introduce trace_f2fs_replace_atomic_write_block
https://git.kernel.org/jaegeuk/f2fs/c/2f3a9ae990a7
- [f2fs-dev,2/5] f2fs: clear atomic_write_task in f2fs_abort_atomic_write()
https://git.kernel.org/jaegeuk/f2fs/c/0e8d040bfa4c
- [f2fs-dev,3/5] f2fs: fix to abort atomic write only during do_exist()
(no matching commit)
- [f2fs-dev,4/5] f2fs: fix to avoid race condition of f2fs_abort_atomic_write()
https://git.kernel.org/jaegeuk/f2fs/c/f4c49874a80b
- [f2fs-dev,5/5] f2fs: fix to avoid race condition of atomic write
https://git.kernel.org/jaegeuk/f2fs/c/f571253668a9
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <[email protected]>:
On Mon, 9 Jan 2023 11:44:49 +0800 you wrote:
> Commit 3db1de0e582c ("f2fs: change the current atomic write way")
> removed old tracepoints, but it missed to add new one, this patch
> fixes to introduce trace_f2fs_replace_atomic_write_block to trace
> atomic_write commit flow.
>
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Signed-off-by: Chao Yu <[email protected]>
>
> [...]
Here is the summary with links:
- [f2fs-dev,1/5] f2fs: introduce trace_f2fs_replace_atomic_write_block
(no matching commit)
- [f2fs-dev,2/5] f2fs: clear atomic_write_task in f2fs_abort_atomic_write()
(no matching commit)
- [f2fs-dev,3/5] f2fs: fix to abort atomic write only during do_exist()
https://git.kernel.org/jaegeuk/f2fs/c/ae267fc1cfe9
- [f2fs-dev,4/5] f2fs: fix to avoid race condition of f2fs_abort_atomic_write()
(no matching commit)
- [f2fs-dev,5/5] f2fs: fix to avoid race condition of atomic write
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html