Re: [PATCH] ext4: Fix deadlock with fs freezing and EA inodes

From: Andreas Dilger <adilger@dilger.ca>
To: Jan Kara <jack@suse.cz>
Cc: Ted Tso <tytso@mit.edu>,
	linux-ext4@vger.kernel.org, stable@vger.kernel.org,
	Tahsin Erdogan <tahsin@google.com>
Subject: Re: [PATCH] ext4: Fix deadlock with fs freezing and EA inodes
Date: Sun, 29 Nov 2020 14:32:23 -0700	[thread overview]
Message-ID: <13C51452-1690-4F47-8B72-873D2511193D@dilger.ca> (raw)
In-Reply-To: <20201127110649.24730-1-jack@suse.cz>

[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]

On Nov 27, 2020, at 4:06 AM, Jan Kara <jack@suse.cz> wrote:
> 
> Xattr code using inodes with large xattr data can end up dropping last
> inode reference (and thus deleting the inode) from places like
> ext4_xattr_set_entry(). That function is called with transaction started
> and so ext4_evict_inode() can deadlock against fs freezing like:
> 
> CPU1					CPU2
> 
> removexattr()				freeze_super()
>  vfs_removexattr()
>    ext4_xattr_set()
>      handle = ext4_journal_start()
>      ...
>      ext4_xattr_set_entry()
>        iput(old_ea_inode)
>          ext4_evict_inode(old_ea_inode)
> 					  sb->s_writers.frozen = SB_FREEZE_FS;
> 					  sb_wait_write(sb, SB_FREEZE_FS);
> 					  ext4_freeze()
> 					    jbd2_journal_lock_updates()
> 					      -> blocks waiting for all
> 					         handles to stop
>            sb_start_intwrite()
> 	      -> blocks as sb is already in SB_FREEZE_FS state
> 
> Generally it is advisable to delete inodes from a separate transaction
> as it can consume quite some credits however in this case it would be
> quite clumsy and furthermore the credits for inode deletion are quite
> limited and already accounted for. So just tweak ext4_evict_inode() to
> avoid freeze protection if we have transaction already started and thus
> it is not really needed anyway.
> 
> CC: stable@vger.kernel.org
> Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
> CC: Tahsin Erdogan <tahsin@google.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/inode.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 72534319fae5..777eb08b29cd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -175,6 +175,7 @@ void ext4_evict_inode(struct inode *inode)
> 	 */
> 	int extra_credits = 6;
> 	struct ext4_xattr_inode_array *ea_inode_array = NULL;
> +	bool freeze_protected = false;
> 
> 	trace_ext4_evict_inode(inode);
> 
> @@ -232,9 +233,14 @@ void ext4_evict_inode(struct inode *inode)
> 
> 	/*
> 	 * Protect us against freezing - iput() caller didn't have to have any
> -	 * protection against it
> +	 * protection against it. When we are in a running transaction though,
> +	 * we are already protected against freezing and we cannot grab further
> +	 * protection due to lock ordering constraints.
> 	 */
> -	sb_start_intwrite(inode->i_sb);
> +	if (!ext4_journal_current_handle()) {
> +		sb_start_intwrite(inode->i_sb);
> +		freeze_protected = true;
> +	}
> 
> 	if (!IS_NOQUOTA(inode))
> 		extra_credits += EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb);
> @@ -253,7 +259,8 @@ void ext4_evict_inode(struct inode *inode)
> 		 * cleaned up.
> 		 */
> 		ext4_orphan_del(NULL, inode);
> -		sb_end_intwrite(inode->i_sb);
> +		if (freeze_protected)
> +			sb_end_intwrite(inode->i_sb);
> 		goto no_delete;
> 	}
> 
> @@ -294,7 +301,8 @@ void ext4_evict_inode(struct inode *inode)
> stop_handle:
> 		ext4_journal_stop(handle);
> 		ext4_orphan_del(NULL, inode);
> -		sb_end_intwrite(inode->i_sb);
> +		if (freeze_protected)
> +			sb_end_intwrite(inode->i_sb);
> 		ext4_xattr_inode_array_free(ea_inode_array);
> 		goto no_delete;
> 	}
> @@ -323,7 +331,8 @@ void ext4_evict_inode(struct inode *inode)
> 	else
> 		ext4_free_inode(handle, inode);
> 	ext4_journal_stop(handle);
> -	sb_end_intwrite(inode->i_sb);
> +	if (freeze_protected)
> +		sb_end_intwrite(inode->i_sb);
> 	ext4_xattr_inode_array_free(ea_inode_array);
> 	return;
> no_delete:
> --
> 2.16.4
> 


Cheers, Andreas


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]