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

[Dave Chinner <david@fromorbit.com>]

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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com