Re: [RFC] possible badness in prune_dcache()

[Josef Bacik <jbacik@redhat.com>]

On Fri, Apr 04, 2008 at 09:01:26PM +0200, Miklos Szeredi wrote:
> > > > > probably worth looking at doing something different in the case of
> > > > > shrinking the dcache on the parent, and leaving prune_dcache to
> > > > > only be called in the case of trying to free up dcache under
> > > > > memory pressure, where the superblock doesn't actually matter.
> > > > > For the RHEL3 issue you are reffering to I fixed it by creating a
> > > > > private list when we shrunk the parent, and submitting that list
> > > > > to prune_dcache that way we didn't spend all this time looping.  I
> > > > > will see what can be done for upstream.
> > > 
> > > Which sounds racy with umount.  A hashed dentry must either have a
> > > refcount greater than one, or be on dentry_unused list.  This patch
> > > breaks that assumption.
> > >
> > 
> > It should be racy with umount, if we notice that we're being
> > unmounted we just break, as the unmount will free the dentry's
> > itself through another means.
> 
> But unmount could have already finished by then.  And now you are
> dereferencing a super block, that no longer exists.  Not good.
> 
> This separate list thing can't work, unfortunately.  On the other hand
> we should probably split prune_dcache() into two separate functions:
> the garbage collector one (with sb == NULL argument) and the sb
> specific shrinker.  It should I think be possible to share the second
> one with shrink_dcache_sb().
>

Hmm good point.  How bout this?  Keeps us from doing the skip thing and keeps
the garbage collector path a little cleaner.  Thanks much,

Josef

 
diff --git a/fs/dcache.c b/fs/dcache.c
index 4345577..2d22176 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -413,8 +413,6 @@ static void prune_one_dentry(struct dentry * dentry)
 /**
  * prune_dcache - shrink the dcache
  * @count: number of entries to try and free
- * @sb: if given, ignore dentries for other superblocks
- *         which are being unmounted.
  *
  * Shrink the dcache. This is done when we need
  * more memory, or simply when we need to unmount
@@ -425,7 +423,7 @@ static void prune_one_dentry(struct dentry * dentry)
  * all the dentries are in use.
  */
  
-static void prune_dcache(int count, struct super_block *sb)
+static void prune_dcache(int count)
 {
 	spin_lock(&dcache_lock);
 	for (; count ; count--) {
@@ -436,18 +434,6 @@ static void prune_dcache(int count, struct super_block *sb)
 		cond_resched_lock(&dcache_lock);
 
 		tmp = dentry_unused.prev;
-		if (sb) {
-			/* Try to find a dentry for this sb, but don't try
-			 * too hard, if they aren't near the tail they will
-			 * be moved down again soon
-			 */
-			int skip = count;
-			while (skip && tmp != &dentry_unused &&
-			    list_entry(tmp, struct dentry, d_lru)->d_sb != sb) {
-				skip--;
-				tmp = tmp->prev;
-			}
-		}
 		if (tmp == &dentry_unused)
 			break;
 		list_del_init(tmp);
@@ -475,21 +461,10 @@ static void prune_dcache(int count, struct super_block *sb)
 		}
 		/*
 		 * If the dentry is not DCACHED_REFERENCED, it is time
-		 * to remove it from the dcache, provided the super block is
-		 * NULL (which means we are trying to reclaim memory)
-		 * or this dentry belongs to the same super block that
-		 * we want to shrink.
+		 * to remove it from the dcache
 		 */
 		/*
-		 * If this dentry is for "my" filesystem, then I can prune it
-		 * without taking the s_umount lock (I already hold it).
-		 */
-		if (sb && dentry->d_sb == sb) {
-			prune_one_dentry(dentry);
-			continue;
-		}
-		/*
-		 * ...otherwise we need to be sure this filesystem isn't being
+		 * we need to be sure this filesystem isn't being
 		 * unmounted, otherwise we could race with
 		 * generic_shutdown_super(), and end up holding a reference to
 		 * an inode while the filesystem is unmounted.
@@ -840,7 +815,7 @@ void shrink_dcache_parent(struct dentry * parent)
 	int found;
 
 	while ((found = select_parent(parent)) != 0)
-		prune_dcache(found, parent->d_sb);
+		shrink_dcache_sb(parent->d_sb);
 }
 
 /*
@@ -860,7 +835,7 @@ static int shrink_dcache_memory(int nr, gfp_t gfp_mask)
 	if (nr) {
 		if (!(gfp_mask & __GFP_FS))
 			return -1;
-		prune_dcache(nr, NULL);
+		prune_dcache(nr);
 	}
 	return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
 }