On Fri 24-06-22 15:04:04, Zhang Yi wrote:
> When evicting an inode with default dioread_nolock, it could be raced by
> the unwritten extents converting kworker after writeback some new
> allocated dirty blocks. It convert unwritten extents to written, the
> extents could be merged to upper level and free extent blocks, so it
> could mark the inode dirty again even this inode has been marked
> I_FREEING. But the inode->i_io_list check and warning in
> ext4_evict_inode() missing this corner case. Fortunately,
> ext4_evict_inode() will wait all extents converting finished before this
> check, so it will not lead to inode use-after-free problem, so every
> thing is OK besides this warning, let the WARN_ON_ONCE know the
> dioread_nolock case to silence this warning is fine.
>
> ======
> WARNING: CPU: 7 PID: 1092 at fs/ext4/inode.c:227
> ext4_evict_inode+0x875/0xc60
> ...
> RIP: 0010:ext4_evict_inode+0x875/0xc60
> ...
> Call Trace:
> <TASK>
> evict+0x11c/0x2b0
> iput+0x236/0x3a0
> do_unlinkat+0x1b4/0x490
> __x64_sys_unlinkat+0x4c/0xb0
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x46/0xb0
> RIP: 0033:0x7fa933c1115b
> ======
>
> rm kworker
> ext4_end_io_end()
> vfs_unlink()
> ext4_unlink()
> ext4_convert_unwritten_io_end_vec()
> ext4_convert_unwritten_extents()
> ext4_map_blocks()
> ext4_ext_map_blocks()
> ext4_ext_try_to_merge_up()
> __mark_inode_dirty()
> check !I_FREEING
> locked_inode_to_wb_and_lock_list()
> iput()
> iput_final()
> evict()
> ext4_evict_inode()
> truncate_inode_pages_final() //wait release io_end
> inode_io_list_move_locked()
> ext4_release_io_end()
> trigger WARN_ON_ONCE()
>
> Fixes: ceff86fddae8 ("ext4: Avoid freeing inodes on dirty list")
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
Good catch! So for the i_nlink == 0 case below, I'd just remove the
WARN_ON_ONCE altogether. It isn't very useful after your change anyway. But
probably we should add:
WARN_ON_ONCE(!list_empty(&inode->i_io_list));
to the no_delete: case of ext4_evict_inode()? Race like you mention above
does not seem possible for that case but seeing the complicated
interactions I'd rather have the assertion in place.
Honza
> ---
> fs/ext4/inode.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3dce7d058985..3b64d72416b7 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -220,11 +220,14 @@ void ext4_evict_inode(struct inode *inode)
>
> /*
> * For inodes with journalled data, transaction commit could have
> - * dirtied the inode. Flush worker is ignoring it because of I_FREEING
> - * flag but we still need to remove the inode from the writeback lists.
> + * dirtied the inode. And for inodes with dioread_nolock, unwritten
> + * extents converting worker could merged extents and also have dirtied
> + * the inode. Flush worker is ignoring it because of I_FREEING flag but
> + * we still need to remove the inode from the writeback lists.
> */
> if (!list_empty_careful(&inode->i_io_list)) {
> - WARN_ON_ONCE(!ext4_should_journal_data(inode));
> + WARN_ON_ONCE(!ext4_should_journal_data(inode) &&
> + !ext4_should_dioread_nolock(inode));
> inode_io_list_del(inode);
> }
>
> --
> 2.31.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
