Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()

[Al Viro]

On Fri, Feb 16, 2018 at 04:09:32PM +0100, John Ogness wrote:
> @@ -2378,22 +2420,36 @@ void d_delete(struct dentry * dentry)
>  	/*
>  	 * Are we the only user?
>  	 */
> -again:
>  	spin_lock(&dentry->d_lock);
> +again:
>  	inode = dentry->d_inode;
>  	isdir = S_ISDIR(inode->i_mode);
>  	if (dentry->d_lockref.count == 1) {
> -		if (!spin_trylock(&inode->i_lock)) {
> -			spin_unlock(&dentry->d_lock);
> -			cpu_relax();
> +		/*
> +		 * Lock the inode. Might drop dentry->d_lock temporarily
> +		 * which allows inode to change. Start over if that happens.
> +		 */
> +		if (!dentry_lock_inode(dentry))
>  			goto again;

IDGI.  First of all, why do we need to fetch ->d_inode (and calculate isdir)
before that dentry_lock_inode() of yours?  That's at least partially understandable
in the current version, where we need inode in d_delete() scope, but here it looks
bloody odd.

And if you move those fetches past the call of dentry_lock_inode(), you suddenly
get the life much simpler:

	grab d_lock
	if d_count is greater than 1, drop it and bugger off
	while !dentry_lock_inode(dentry)
		;
	fetch inode
	recheck d_count, in the unlikely case when it's greater than 1,
			drop and bugger off
	clear CANT_MOUNT
	calculate isdir
	unlink_inode
	fsnotify shite

I mean, do we really want to keep rechecking d_count on each loop iteration?
What does it buy us?  Sure, we want to recheck in the end for correctness
sake, but...

It might make sense to move the loop inside dentry_lock_inode(), IMO.