d_instantiate() and unlock_new_inode() order in btrfs_mkdir()

Hi Chris and other btrfs folks,

btrfs_mkdir() calls d_instantiate() before unlock_new_inode(), which is wrong because it exposes the inode to lookups before it's been fully initialized. Most filesystems get it right, but f2fs and btrfs don't. I sent a f2fs patch
(https://marc.info/?l=linux-fsdevel&m=152409178431350) and was going to send a btrfs patch too, but in btrfs_mkdir() there is actually a comment claiming that the existing order is intentional:

        d_instantiate(dentry, inode);
        /*
         * mkdir is special.  We're unlocking after we call d_instantiate
         * to avoid a race with nfsd calling d_instantiate.
         */
        unlock_new_inode(inode);

Unfortunately, I cannot find what it is refering to. The comment was added by commit b0d5d10f41a0 ("Btrfs: use insert_inode_locked4 for inode creation"). Chris, do you remember exactly what you had in mind when you wrote this?

And in case anyone wants it, here's a reproducer for the deadlock caused by the current code that calls d_instantiate() before unlock_new_inode(). Note: it needs CONFIG_DEBUG_LOCK_ALLOC=y.

	#include <sys/stat.h>
	#include <unistd.h>

	int main()
	{
		struct stat stbuf;

		if (fork() == 0) {
			for (;;)
				stat("dir/file", &stbuf);
		} else {
			for (;;) {
				mkdir("dir", 0777);
				stat("dir/file", &stbuf);
				rmdir("dir");
			}
		}
	}

Huh? It *is* fully initialized by that point; what else is left to do?

ISTR something about false positives from lockdep (with
lockdep_annotate_inode_mutex_key() called too late, perhaps?); said that, it was a long time ago and I don't remember details at the moment... Are you actually seeing a deadlock there or is that just lockdep complaining?

It's an actual deadlock. unlock_new_inode() calls lockdep_annotate_inode_mutex_key() which calls init_rwsem(), which resets i_rwsem->count while it's read-locked by lookup_slow(). Then the unlock in lookup_slow() makes i_rwsem->count negative, which makes it appear to be write-locked.

So no, the inode isn't fully initialized until unlock_new_inode() ran.

Eric