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.