Re: [PATCH v3] xfs: Fix ABBA deadlock between AGI and AGF when performing rename() with RENAME_WHITEOUT flag

[Eric Sandeen <sandeen@sandeen.net>]

On 8/28/19 8:27 PM, kaixuxia wrote:
> ping...
> Because there isn't this patch in the latest xfs-for-next branch 
> update...

1) V3 appears to have no changes from V2.  Why was V3 sent?

2) Neither version has a reviewed-by yet, Darrick has questions outstanding
   AFAICT, they may need to be answered prior to review and merge.

-Eric
 
> 
> On 2019/8/27 10:54, kaixuxia wrote:
>> When performing rename operation with RENAME_WHITEOUT flag, we will
>> hold AGF lock to allocate or free extents in manipulating the dirents
>> firstly, and then doing the xfs_iunlink_remove() call last to hold
>> AGI lock to modify the tmpfile info, so we the lock order AGI->AGF.
>>
>> The big problem here is that we have an ordering constraint on AGF
>> and AGI locking - inode allocation locks the AGI, then can allocate
>> a new extent for new inodes, locking the AGF after the AGI. Hence
>> the ordering that is imposed by other parts of the code is AGI before
>> AGF. So we get an ABBA deadlock between the AGI and AGF here.
>>
>> Process A:
>> Call trace:
>>  ? __schedule+0x2bd/0x620
>>  schedule+0x33/0x90
>>  schedule_timeout+0x17d/0x290
>>  __down_common+0xef/0x125
>>  ? xfs_buf_find+0x215/0x6c0 [xfs]
>>  down+0x3b/0x50
>>  xfs_buf_lock+0x34/0xf0 [xfs]
>>  xfs_buf_find+0x215/0x6c0 [xfs]
>>  xfs_buf_get_map+0x37/0x230 [xfs]
>>  xfs_buf_read_map+0x29/0x190 [xfs]
>>  xfs_trans_read_buf_map+0x13d/0x520 [xfs]
>>  xfs_read_agf+0xa6/0x180 [xfs]
>>  ? schedule_timeout+0x17d/0x290
>>  xfs_alloc_read_agf+0x52/0x1f0 [xfs]
>>  xfs_alloc_fix_freelist+0x432/0x590 [xfs]
>>  ? down+0x3b/0x50
>>  ? xfs_buf_lock+0x34/0xf0 [xfs]
>>  ? xfs_buf_find+0x215/0x6c0 [xfs]
>>  xfs_alloc_vextent+0x301/0x6c0 [xfs]
>>  xfs_ialloc_ag_alloc+0x182/0x700 [xfs]
>>  ? _xfs_trans_bjoin+0x72/0xf0 [xfs]
>>  xfs_dialloc+0x116/0x290 [xfs]
>>  xfs_ialloc+0x6d/0x5e0 [xfs]
>>  ? xfs_log_reserve+0x165/0x280 [xfs]
>>  xfs_dir_ialloc+0x8c/0x240 [xfs]
>>  xfs_create+0x35a/0x610 [xfs]
>>  xfs_generic_create+0x1f1/0x2f0 [xfs]
>>  ...
>>
>> Process B:
>> Call trace:
>>  ? __schedule+0x2bd/0x620
>>  ? xfs_bmapi_allocate+0x245/0x380 [xfs]
>>  schedule+0x33/0x90
>>  schedule_timeout+0x17d/0x290
>>  ? xfs_buf_find+0x1fd/0x6c0 [xfs]
>>  __down_common+0xef/0x125
>>  ? xfs_buf_get_map+0x37/0x230 [xfs]
>>  ? xfs_buf_find+0x215/0x6c0 [xfs]
>>  down+0x3b/0x50
>>  xfs_buf_lock+0x34/0xf0 [xfs]
>>  xfs_buf_find+0x215/0x6c0 [xfs]
>>  xfs_buf_get_map+0x37/0x230 [xfs]
>>  xfs_buf_read_map+0x29/0x190 [xfs]
>>  xfs_trans_read_buf_map+0x13d/0x520 [xfs]
>>  xfs_read_agi+0xa8/0x160 [xfs]
>>  xfs_iunlink_remove+0x6f/0x2a0 [xfs]
>>  ? current_time+0x46/0x80
>>  ? xfs_trans_ichgtime+0x39/0xb0 [xfs]
>>  xfs_rename+0x57a/0xae0 [xfs]
>>  xfs_vn_rename+0xe4/0x150 [xfs]
>>  ...
>>
>> In this patch we move the xfs_iunlink_remove() call to
>> before acquiring the AGF lock to preserve correct AGI/AGF locking
>> order.
>>
>> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> ---
>>  fs/xfs/xfs_inode.c | 83 +++++++++++++++++++++++++++---------------------------
>>  1 file changed, 42 insertions(+), 41 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 6467d5e..8ffd44f 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -3282,7 +3282,8 @@ struct xfs_iunlink {
>>  					spaceres);
>>  
>>  	/*
>> -	 * Set up the target.
>> +	 * Check for expected errors before we dirty the transaction
>> +	 * so we can return an error without a transaction abort.
>>  	 */
>>  	if (target_ip == NULL) {
>>  		/*
>> @@ -3294,6 +3295,46 @@ struct xfs_iunlink {
>>  			if (error)
>>  				goto out_trans_cancel;
>>  		}
>> +	} else {
>> +		/*
>> +		 * If target exists and it's a directory, check that whether
>> +		 * it can be destroyed.
>> +		 */
>> +		if (S_ISDIR(VFS_I(target_ip)->i_mode) &&
>> +		    (!xfs_dir_isempty(target_ip) ||
>> +		     (VFS_I(target_ip)->i_nlink > 2))) {
>> +			error = -EEXIST;
>> +			goto out_trans_cancel;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Directory entry creation below may acquire the AGF. Remove
>> +	 * the whiteout from the unlinked list first to preserve correct
>> +	 * AGI/AGF locking order. This dirties the transaction so failures
>> +	 * after this point will abort and log recovery will clean up the
>> +	 * mess.
>> +	 *
>> +	 * For whiteouts, we need to bump the link count on the whiteout
>> +	 * inode. After this point, we have a real link, clear the tmpfile
>> +	 * state flag from the inode so it doesn't accidentally get misused
>> +	 * in future.
>> +	 */
>> +	if (wip) {
>> +		ASSERT(VFS_I(wip)->i_nlink == 0);
>> +		error = xfs_iunlink_remove(tp, wip);
>> +		if (error)
>> +			goto out_trans_cancel;
>> +
>> +		xfs_bumplink(tp, wip);
>> +		xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
>> +		VFS_I(wip)->i_state &= ~I_LINKABLE;
>> +	}
>> +
>> +	/*
>> +	 * Set up the target.
>> +	 */
>> +	if (target_ip == NULL) {
>>  		/*
>>  		 * If target does not exist and the rename crosses
>>  		 * directories, adjust the target directory link count
>> @@ -3312,22 +3353,6 @@ struct xfs_iunlink {
>>  		}
>>  	} else { /* target_ip != NULL */
>>  		/*
>> -		 * If target exists and it's a directory, check that both
>> -		 * target and source are directories and that target can be
>> -		 * destroyed, or that neither is a directory.
>> -		 */
>> -		if (S_ISDIR(VFS_I(target_ip)->i_mode)) {
>> -			/*
>> -			 * Make sure target dir is empty.
>> -			 */
>> -			if (!(xfs_dir_isempty(target_ip)) ||
>> -			    (VFS_I(target_ip)->i_nlink > 2)) {
>> -				error = -EEXIST;
>> -				goto out_trans_cancel;
>> -			}
>> -		}
>> -
>> -		/*
>>  		 * Link the source inode under the target name.
>>  		 * If the source inode is a directory and we are moving
>>  		 * it across directories, its ".." entry will be
>> @@ -3417,30 +3442,6 @@ struct xfs_iunlink {
>>  	if (error)
>>  		goto out_trans_cancel;
>>  
>> -	/*
>> -	 * For whiteouts, we need to bump the link count on the whiteout inode.
>> -	 * This means that failures all the way up to this point leave the inode
>> -	 * on the unlinked list and so cleanup is a simple matter of dropping
>> -	 * the remaining reference to it. If we fail here after bumping the link
>> -	 * count, we're shutting down the filesystem so we'll never see the
>> -	 * intermediate state on disk.
>> -	 */
>> -	if (wip) {
>> -		ASSERT(VFS_I(wip)->i_nlink == 0);
>> -		xfs_bumplink(tp, wip);
>> -		error = xfs_iunlink_remove(tp, wip);
>> -		if (error)
>> -			goto out_trans_cancel;
>> -		xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
>> -
>> -		/*
>> -		 * Now we have a real link, clear the "I'm a tmpfile" state
>> -		 * flag from the inode so it doesn't accidentally get misused in
>> -		 * future.
>> -		 */
>> -		VFS_I(wip)->i_state &= ~I_LINKABLE;
>> -	}
>> -
>>  	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>>  	xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
>>  	if (new_parent)
>>
>