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.
