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

[Brian Foster <bfoster@redhat.com>]

On Tue, Aug 20, 2019 at 04:53:22PM +0800, kaixuxia wrote:
> 
> 
> On 2019/8/20 16:07, Dave Chinner wrote:
> > On Tue, Aug 20, 2019 at 02:45:36PM +0800, kaixuxia wrote:
> > > On 2019/8/19 23:13, Brian Foster wrote:
> > > > 	/* error checks before we dirty the transaction */
> > > > 	if (!target_ip && !spaceres) {
> > > > 		error = xfs_dir_canenter();
> > > > 		...
> > > > 	} else if (S_ISDIR() && !(empty || nlink > 2))
> > > > 		error = -EEXIST;
> > > > 		...
> > > > 	}
> > > > 
> > > > 	if (wip) {
> > > > 		...
> > > > 		xfs_iunlink_remove();
> > > > 	}
> > > > 
> > > > 	if (!target_ip) {
> > > > 		xfs_dir_create();
> > > > 		...
> > > > 	} else {
> > > > 		xfs_dir_replace();
> > > > 		...
> > > > 	}
> > > > 
> > > > ... but that may not be any cleaner..? It could also be done as a
> > > > followup cleanup patch as well.
> > > 
> > > Yep, it is cleaner that making the whole check before the transaction
> > > becomes dirty, just return the error code if check failed and
> > > the filesystem is clean.
> > 
> > *nod*
> > 
> > > Dave gave another solution in the other subthread that using
> > > XFS_DIR3_FT_WHT, it's a bit more work for this bug, include
> > > refactoring the xfs_rename() and xfs_lookup(), not sure whether
> > > it's worth the complex changes for this bug.
> > 

Yeah, I wasn't aware of that option. What Dave describes wrt to
replacing the on-disk whiteout inode with a dirent + in-core variant
sounds like the clear best option to me over the ones previously
discussed.

> > It's not necessary to fix the bug, but it's somethign we should
> > be looking to do because it makes whiteout handling a lot more
> > efficient - it's just dirent modifications at that point, no inodes
> > are necessary.
> > 
> > This is how I always intended to handle whiteouts - it's just
> > another thing on the "we need to fix" list....
> 
> Right, it is more efficient because there is no need to store it on disk,
> and it will improve performance just like the async deferred operations.
> Maybe it is on the roadmap, so I'm not sure whether I should send the V3
> patch to address Brian's comments. Maybe we can choose the V3 patch first,
> and then the whiteout improvement could be done as the followup patch
> in future...
> 

I agree. I think a two step process makes sense because we may want a
backportable fix around for the locking bug that doesn't depend on
replacing the implementation.

FWIW if we do take that approach, then IMO it's worth reconsidering the
1-2 liner I originally proposed to fix the locking. It's slightly hacky,
but really all three options are hacky in slightly different ways. The
flipside is it's trivial to implement, review and backport and now would
be removed shortly thereafter when we replace the on-disk whiteout with
the in-core fake whiteout thing. Just my .02 though..

Brian

> > 
> > Cheers,
> > 
> > Dave.
> > 
> 
> -- 
> kaixuxia