Re: [PATCH] shrink_dcache_parent() races against shrink_dcache_memory()

[Balbir Singh <balbir@in.ibm.com>]

Hi, Kirill,

On Tue, Jan 24, 2006 at 12:48:13PM +0300, Kirill Korotaev wrote:
> I like your idea, but some comments below... I doubt it works.
> I will think it over a bit later...
> 

Thanks. Please find my comments and updated patch below

> Kirill
> P.S. it's not easily reproducable. Before my fix it took us 3-6 hours on 
> automated stress testing to hit this bug. Right now I can't setup it for 
> testing, maybe in a week or so.

Sure, please test whenever you set it up.

[snip]

> >+		spin_lock(&sb_lock);
> <<<< 1. sb_lock doesn't protect atomic_read() anyhow...
> <<<<    I mean, sb_lock is not required to read its value...

Good point, the sb_lock is not required. I have removed it.

> >+		if (!atomic_read(&dentry->d_sb->s_active)) {
> >+			/*
> >+			 * Race condition, umount and other pruning is 
> >happening
> >+			 * in parallel.
> >+			 */
> >+			if (current->flags & PF_MEMALLOC) {
> >+				/*
> >+				 * let the allocator leave this dentry alone
> >+				 */
> >+				spin_unlock(&sb_lock);
> >+				spin_unlock(&dentry->d_lock);
> >+				spin_unlock(&dcache_lock);
> >+				return;
> <<<< you should not return, but rather 'continue'. otherwise you skip 
> _all_ dentries, even from active super blocks.

Good point.

> >+			}
> >+		}
> >+		spin_unlock(&sb_lock);
> >+
> <<<< and here, when you drop sb_lock, and dentry->d_lock/dcache_lock in 
> prune_dentry() it looks to me that we have exactly the same situation as 
> it was without your patch:
> <<<< another CPU can start umount in parallel.
> <<<< maybe sb_lock barrier helps this somehow, but I can't see how yet...

>From the unmount path, __mntput() is called. It sets s_active to 0 in
deactivate_super(), hence our check would prevent us from pruning a dentry
that is a part of a super block that is going to go away soon. The idea
is to let the unmount do all the work here, the allocator can concentrate
on other dentries.

> 
> <<<< another idea: down_read(&sb->s_umount) probably could help...
> <<<< because it will block the whole umount operation...
> <<<< but we can't take it under dcache_lock...

Yes, we cannot do a down* under a spinlock

[snip]

How does the modified patch look? 

Regards,
Balbir


Signed-off-by: Balbir Singh <balbir@in.ibm.com>
---

 fs/dcache.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+)

diff -puN fs/dcache.c~dcache_race_fix2 fs/dcache.c
--- linux-2.6/fs/dcache.c~dcache_race_fix2	2006-01-24 11:05:46.000000000 +0530
+++ linux-2.6-balbir/fs/dcache.c	2006-01-24 15:49:30.000000000 +0530
@@ -425,6 +425,21 @@ static void prune_dcache(int count)
  			spin_unlock(&dentry->d_lock);
 			continue;
 		}
+
+		if (!atomic_read(&dentry->d_sb->s_active)) {
+			/*
+			 * Race condition, umount and other pruning is happening
+			 * in parallel.
+			 */
+			if (current->flags & PF_MEMALLOC) {
+				/*
+				 * Ask the allocator leave this dentry alone
+				 */
+				spin_unlock(&dentry->d_lock);
+				continue;
+			}
+		}
+
 		prune_one_dentry(dentry);
 	}
 	spin_unlock(&dcache_lock);
_