Q: spin_unlock(dentry) after lock_parent(dentry)

Q: spin_unlock(dentry) after lock_parent(dentry)

[J. R. Okajima]

Hello Al Viro,

I have a question about spin_unlock(dentry) after lock_parent(dentry).
In lock_parent(dentry), spin_unlock(dentry) is called. And
spin_lock(dentry) is called again when (parent != dentry) is
true. Otherwise, dentry left spin_unlock-ed and lock_parent() returns
NULL.

Even in the case of lock_parent() returns NULL, shrink_dentry_list()
calls spin_unlock(dentry). Is it balanced and correct?


J. R. Okajima

Re: Q: spin_unlock(dentry) after lock_parent(dentry)

[Al Viro]

On Tue, Jun 03, 2014 at 11:36:55PM +0900, J. R. Okajima wrote:
> 
> Hello Al Viro,
> 
> I have a question about spin_unlock(dentry) after lock_parent(dentry).
> In lock_parent(dentry), spin_unlock(dentry) is called. And
> spin_lock(dentry) is called again when (parent != dentry) is
> true. Otherwise, dentry left spin_unlock-ed and lock_parent() returns
> NULL.

Nope.  You are misreading it.  Note that in *all* cases the parent is
locked.  And NULL is returned exactly in the case when that parent is
equal to dentry itself.  IOW, in all cases, lock_parent(d) returns with d
locked and after p = lock_parent(d) either
	1) d->d_parent == d and p == NULL, or
	2) d->d_parent == p, p != d and p is locked as well.
That holds whether we had succeeded with trylock or had to drop and regain
the lock on d.

> Even in the case of lock_parent() returns NULL, shrink_dentry_list()
> calls spin_unlock(dentry). Is it balanced and correct?

Yes.

Re: Q: spin_unlock(dentry) after lock_parent(dentry)

[J. R. Okajima]

Al Viro:
> Nope.  You are misreading it.  Note that in *all* cases the parent is
> locked.  And NULL is returned exactly in the case when that parent is
	:::

Thanks.
Please allow me another question.

In the case of concurrent dentry_kill() and shrink_dentry_list(),
dentry_kill() kills the dentry and calls list_del(&dentry->d_u.d_child).
lock_parent() (called by shrink_dentry_list()) doesn't check d_child
nor DCACHE_DENTRY_KILLED. So shrink_dentry_list() may get a parent of a
dead dentry and tries traversing via d_parent.
Is it correct to track d_parent after list_del(&dentry->d_u.d_child)?
Should lock_parent() consider DCACHE_DENTRY_KILLED too?

--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -533,7 +533,8 @@ failed:
 static inline struct dentry *lock_parent(struct dentry *dentry)
 {
 	struct dentry *parent = dentry->d_parent;
-	if (IS_ROOT(dentry))
+	if (IS_ROOT(dentry)
+	    || (dentry->d_flags & DCACHE_DENTRY_KILLED))
 		return NULL;
 	if (likely(spin_trylock(&parent->d_lock)))
 		return parent;


J. R. Okajima

Re: Q: spin_unlock(dentry) after lock_parent(dentry)

[J. R. Okajima]

Al Viro,

I think I could find a clearer problematic scenario.
Can it happen in real world?

- /.../dirA/dirB/fileC exists.
- shrink_dentry_list() gets a list containing fileC and dirB, but not
  dirA
  + Round 1: fileC
    - shrink_dentry_list() gets d_parent which is dirB
    - dirB is DCACHE_SHRINK_LIST set, so __dentry_kill() is called but
      not dentry_free()
    - shrink_dentry_list() gets higher d_parent which is dirA
    - dirA is not DCACHE_SHRINK_LIST set, so both of __dentry_kill() and
      dentry_free() are is called
  + Round 2: dirB
    - shrink_dentry_list() tries getting d_parent which is dirA
    - dirA is freed by RCU. if RCU already activated __d_free(), BANG!


> In the case of concurrent dentry_kill() and shrink_dentry_list(),
> dentry_kill() kills the dentry and calls list_del(&dentry->d_u.d_child).
> lock_parent() (called by shrink_dentry_list()) doesn't check d_child
> nor DCACHE_DENTRY_KILLED. So shrink_dentry_list() may get a parent of a
> dead dentry and tries traversing via d_parent.
> Is it correct to track d_parent after list_del(&dentry->d_u.d_child)?
> Should lock_parent() consider DCACHE_DENTRY_KILLED too?
>
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -533,7 +533,8 @@ failed:
>  static inline struct dentry *lock_parent(struct dentry *dentry)
>  {
>  	struct dentry *parent = dentry->d_parent;
> -	if (IS_ROOT(dentry))
> +	if (IS_ROOT(dentry)
> +	    || (dentry->d_flags & DCACHE_DENTRY_KILLED))
>  		return NULL;
>  	if (likely(spin_trylock(&parent->d_lock)))
>  		return parent;


J. R. Okajima