On 2018-02-22, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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.
I tried to change the function as little as possible. You are right that
it now looks odd. I seem to have missed the forest for the trees.
> 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...
I have been unable to produce a test case where dentry_lock_inode() can
fail. AFAICT it is not possible from userspace. Perhaps some filesystem
could trigger it. But if it would fail, getting the refcount to increase
in the dropped d_lock window is quite easy to reproduce. And in that
case we wouldn't need to keep trying to aquire the inode lock and could
just drop.
> It might make sense to move the loop inside dentry_lock_inode(), IMO.
Agreed. I will change dentry_lock_inode() so that it will only fail if
the refcount changes. If there are inode changes, it will loop
internally. That will change your suggestion to:
grab d_lock
if d_count is greater than 1
drop it and bugger off
if !dentry_lock_inode(dentry)
drop it and bugger off
fetch inode
clear CANT_MOUNT
calculate isdir
unlink_inode
fsnotify shite
John Ogness
