Q. locking order of dcache_lru_lock

Q. locking order of dcache_lru_lock

[J. R. Okajima]

Hello Al Viro, Christoph Hellwig and Nick Piggin,

I have a question about the locking order of dcache_lru_lock.

The comment in fs/dcache.c says
 * Ordering:
 * dentry->d_inode->i_lock
 *   dentry->d_lock
 *     dcache_lru_lock
	:::

d_lock should be before dcache_lru_lock.
Actually dentry_lru_(add|del|move_tail) functions (and their callers) do
it expectedly.
But __shrink_dcache_sb() looks different.

__shrink_dcache_sb()
{
	:::
relock:
	spin_lock(&dcache_lru_lock);
	while (!list_empty(&sb->s_dentry_lru)) {
		::
		if (!spin_trylock(&dentry->d_lock)) {
			spin_unlock(&dcache_lru_lock);
			cpu_relax();
			goto relock;
		}
	:::
}

When spin_trylock(&dentry->d_lock) successfully acquired d_lock, does
the violation of locking order happen (or a deadlock, in worse case)?

By the way, the code is introduced by the commit
	2304450 2011-01-07 fs: dcache scale lru
by Nick Piggin.
Is he allright? Does anyone know anything?
We have not received from him for a long time.


J. R. Okajima

Re: Q. locking order of dcache_lru_lock

[Peter Zijlstra]

On Fri, 2011-04-08 at 22:20 +0900, J. R. Okajima wrote:
> 
> When spin_trylock(&dentry->d_lock) successfully acquired d_lock, does
> the violation of locking order happen (or a deadlock, in worse case)? 

No, since a trylock never actually blocks a deadlock cannot occur.

Re: Q. locking order of dcache_lru_lock

[J. R. Okajima]

Peter Zijlstra:
> On Fri, 2011-04-08 at 22:20 +0900, J. R. Okajima wrote:
> >=20
> > When spin_trylock(&dentry->d_lock) successfully acquired d_lock, does
> > the violation of locking order happen (or a deadlock, in worse case)?=20
>
> No, since a trylock never actually blocks a deadlock cannot occur.

Ah, exactly. I had to be sleeping when I wrote about deadlock.
How about the locking order? Do you think d_lock after dcache_lru_lock
is a problem?


J. R. Okajima

Re: Q. locking order of dcache_lru_lock

[Dave Chinner]

On Mon, Apr 11, 2011 at 02:09:31PM +0900, J. R. Okajima wrote:
> 
> Peter Zijlstra:
> > On Fri, 2011-04-08 at 22:20 +0900, J. R. Okajima wrote:
> > >=20
> > > When spin_trylock(&dentry->d_lock) successfully acquired d_lock, does
> > > the violation of locking order happen (or a deadlock, in worse case)?=20
> >
> > No, since a trylock never actually blocks a deadlock cannot occur.
> 
> Ah, exactly. I had to be sleeping when I wrote about deadlock.
> How about the locking order? Do you think d_lock after dcache_lru_lock
> is a problem?

>From fs/dcache.c:

 * Ordering:
 * dentry->d_inode->i_lock
 *   dentry->d_lock
 *     dcache_lru_lock
 *     dcache_hash_bucket lock
 *     s_anon lock

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Q. locking order of dcache_lru_lock

[Peter Zijlstra]

On Mon, 2011-04-11 at 14:09 +0900, J. R. Okajima wrote:
> Peter Zijlstra:
> > On Fri, 2011-04-08 at 22:20 +0900, J. R. Okajima wrote:
> > >=20
> > > When spin_trylock(&dentry->d_lock) successfully acquired d_lock, does
> > > the violation of locking order happen (or a deadlock, in worse case)?=20
> >
> > No, since a trylock never actually blocks a deadlock cannot occur.
> 
> Ah, exactly. I had to be sleeping when I wrote about deadlock.
> How about the locking order? Do you think d_lock after dcache_lru_lock
> is a problem?

Not really a problem, locking order is simply a tool/scheme to avoid
deadlocks. Since there is no deadlock potential its fine to 'violate'
locking order.

This is a common pattern with trylocks. In situations where you would
need somewhat expensive lock operations to grab the locks in the right
order, you can trylock to see if you can get them in the wrong order. If
the trylock succeeds, yay! you got it cheap. If not, bummer, and you
have to try the expensive way.

Re: Q. locking order of dcache_lru_lock

[J. R. Okajima]

Peter Zijlstra:
> Not really a problem, locking order is simply a tool/scheme to avoid
> deadlocks. Since there is no deadlock potential its fine to 'violate'
> locking order.

I see.
Then lockdep always ignore all tyrlocks?

Thank you for answering.

J. R. Okajima

Re: Q. locking order of dcache_lru_lock

[Peter Zijlstra]

On Mon, 2011-04-11 at 21:33 +0900, J. R. Okajima wrote:
> Peter Zijlstra:
> > Not really a problem, locking order is simply a tool/scheme to avoid
> > deadlocks. Since there is no deadlock potential its fine to 'violate'
> > locking order.
> 
> I see.
> Then lockdep always ignore all tyrlocks?

For tracking dependencies, yes. It does register we hold the lock when
the trylock is successful.