Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()
On 2018-02-22, Al Viro 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
Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()
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.
Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()
On Fri, Feb 16, 2018 at 04:09:32PM +0100, John Ogness wrote: > >inode = dentry->d_inode; >rcu_read_lock(); <- Protects d_inode from being freed, >i.e. dentry->d_inode is a valid pointer >even after dentry->d_lock is dropped >unlock(dentry->d_lock); >lock(inode->i_lock); >lock(dentry->d_lock); >rcu_read_unlock(); So that is entirely tricky, typically we have to have a lookup _after_ rcu_read_lock(). Here, we rely on not being able to call dentry_free() while we hold d_lock, which ensure dentry must be valid in the freshly started rcu-section. And I suppose that same ensures dentry->d_ionde stays alive. But this needs a comment at least.
Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()
On Fri, Feb 16, 2018 at 04:09:32PM +0100, John Ogness wrote:
> +static bool dentry_lock_inode(struct dentry *dentry)
> +{
> + struct inode *inode = dentry->d_inode;
> +
> + lockdep_assert_held(&dentry->d_lock);
> +
> + if (unlikely(!spin_trylock(&inode->i_lock))) {
if (likely(spin_trylock(&inode->i_lock)))
return true;
and then unindent by 1 stop the below code:
> + rcu_read_lock();
> + spin_unlock(&dentry->d_lock);
> + spin_lock(&inode->i_lock);
> + spin_lock(&dentry->d_lock);
> + rcu_read_unlock();
> +
> + /*
> + * @dentry->d_inode might have changed after dropping
> + * @dentry->d_lock. If so, release @inode->i_lock and
> + * signal the caller to restart the operation.
> + */
> + if (unlikely(inode != dentry->d_inode)) {
> + spin_unlock(&inode->i_lock);
> + return false;
> + }
> + }
> + return true;
> +}
