On 2018-02-22, Al Viro <v...@zeniv.linux.org.uk> 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

Reply via email to