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

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

On Mon, Jan 23, 2006 at 04:57:28PM +0100, Jan Blunck wrote:
> On Mon, Jan 23, Kirill Korotaev wrote:
>
> [snip] 
>
> Hmm, will think about that one again. shrink_dcache_parent() and
> shrink_dcache_memory()/dput() are not racing against each other now since the
> reference counting is done before giving up dcache_lock and the select_parent
> could start.
> 
> Regards,
> 	Jan
>

I have been playing around with a possible solution to the problem.
I have not been able to reproduce this issue, hence I am unable to verify
if the patch below fixes the problem. I have run the system with this
patch and verified that no obvious badness is observed.

Kirill, Jan if you can easily reproduce the problem, could you
try this patch and review it as well for correctness of the solution?

All callers that try to free memory set the PF_MEMALLOC flag, we check
if the super block is going away due to an unmount, if so we ask the
allocator to return.

The patch adds additional cost of holding the sb_lock for each dentry
being pruned. It holds sb_lock under dentry->d_lock and dcache_lock,
I am not sure about the locking order of these locks.

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

 fs/dcache.c |   23 +++++++++++++++++++++++
 1 files changed, 23 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 11:05:46.000000000 +0530
@@ -425,6 +425,29 @@ static void prune_dcache(int count)
  			spin_unlock(&dentry->d_lock);
 			continue;
 		}
+
+		/*
+		 * Note to reviewers: our current lock order is dcache_lock,
+		 * dentry->d_lock & sb_lock. Could this create a deadlock?
+		 */
+		spin_lock(&sb_lock);
+		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;
+			}
+		}
+		spin_unlock(&sb_lock);
+
 		prune_one_dentry(dentry);
 	}
 	spin_unlock(&dcache_lock);

Thanks,
Balbir
_