Re: [PATCH v4] xfs: Fix agi&agf ABBA deadlock when performing rename with RENAME_WHITEOUT flag

[kaixuxia <xiakaixu1987@gmail.com>]

On 2019/8/22 14:06, Dave Chinner wrote:
> On Thu, Aug 22, 2019 at 01:45:48PM +0800, kaixuxia wrote:
>> On 2019/8/22 13:01, Dave Chinner wrote:
>>> On Thu, Aug 22, 2019 at 12:33:23PM +0800, kaixuxia wrote:
>>>
>>>> @@ -3419,25 +3431,15 @@ struct xfs_iunlink {
>>>>  
>>>>  	/*
>>>>  	 * For whiteouts, we need to bump the link count on the whiteout inode.
>>>
>>> Shouldn't this line be removed as well?
>>
>> Because the xfs_bumplink() call below will do this.
> 
> Oh, yeah, I just assumed that from the "we have a real link" part of
> the new comment :P
> 
>>>> -	 * 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.
>>>> +	 * The whiteout inode has been removed from the unlinked list and log
>>>> +	 * recovery will clean up the mess for the failures up to this point.
>>>> +	 * 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);
>>>>  		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;
>>>>  	}
>>>
>>> Why not move all this up into the same branch that removes the
>>> whiteout from the unlinked list? Why separate this logic as none of
>>> what is left here could cause a failure even if it is run earlier?
>>
>> Yep, it could not cause a failure if we move all this into the same
>> branch that xfs_iunlink_remove() call. We move the xfs_iunlink_remove()
>> first to preserve correct AGI/AGF locking order, and maybe it is better
>> we bump the link count after using the whiteout inode really, such as
>> xfs_dir_replace(...,wip,...) ...
> 
> It makes no difference where we bump the link count as long as we do
> it after the xfs_iunlink_remove() call. At that point, any failure
> will result in a shutdown and so it doesn't matter that we've
> already bumped the link count because the shutdown with prevent
> it from reaching the disk...

Yeah, so it can be like this:

	/*
	 * For whiteouts, we need to bump the link count on the whiteout inode.
	 * The whiteout inode is removed from the unlinked list and log recovery
	 * will clean up the mess for the failures after this point. 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)
			...
		xfs_bumplink(tp, wip);
		xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
		VFS_I(wip)->i_state &= ~I_LINKABLE;
	}

Right?

> 
> Cheers,
> 
> Dave.
> 

-- 
kaixuxia