Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Jun 2022 12:17:49 +0200
From:   Jan Kara <jack@suse.cz>
To:     Zhang Yi <yi.zhang@huawei.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jack@suse.cz, yukuai3@huawei.com
Subject: Re: [PATCH v2 1/2] ext4: silence the warning when evicting inode
 with dioread_nolock
Message-ID: <20220629101749.zljoazab33mb2xqm@quack3>
References: <20220629022940.2855538-1-yi.zhang@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220629022940.2855538-1-yi.zhang@huawei.com>
Precedence: bulk

On Wed 29-06-22 10:29:39, 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, every thing
> is OK besides this warning. The WARN_ON_ONCE was originally designed
> for finding inode use-after-free issues in advance, but if we add
> current dioread_nolock case in, it will become not quite useful, so fix
> this warning by just remove this check.
> 
>  ======
>  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@huawei.com>

Thanks! The patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 84c0eb55071d..702cc208689a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -220,13 +220,13 @@ 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 merge 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));
> +	if (!list_empty_careful(&inode->i_io_list))
>  		inode_io_list_del(inode);
> -	}
>  
>  	/*
>  	 * Protect us against freezing - iput() caller didn't have to have any
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR