lkml.org 
[lkml]   [2018]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()
Date
On 2018-02-22, Al Viro <viro@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

\
 
 \ /
  Last update: 2018-02-22 09:36    [W:0.088 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site