Re: [PATCH v7 04/31] dcache: remove dentries from LRU before putting on dispose list

[Glauber Costa <glommer@parallels.com>]

On 05/14/2013 09:46 AM, Dave Chinner wrote:
> [ v2: don't decrement nr unused twice, spotted by Sha Zhengju ]
> [ v7: (dchinner)
> - shrink list leaks dentries when inode/parent can't be locked in
>   dentry_kill().
> - fix the scope of the sb locking inside shrink_dcache_sb()
> - remove the readdition of dentry_lru_prune(). ]

Dave,

dentry_lru_prune was removed because it would only prune the dentry if
it was in the LRU list, and it has to be always pruned (61572bb1).

You don't reintroduce dentry_lru_prune here, so the two locations which
prune dentries read as follows:


        if (dentry->d_flags & DCACHE_OP_PRUNE)
                dentry->d_op->d_prune(dentry);

        dentry_lru_del(dentry);

I believe this is wrong. My old version would do:

+static void dentry_lru_prune(struct dentry *dentry)
+{
+	/*
+	 * inform the fs via d_prune that this dentry is about to be
+	 * unhashed and destroyed.
+	 */
+	if (dentry->d_flags & DCACHE_OP_PRUNE)
+		dentry->d_op->d_prune(dentry);
+
+	if (list_empty(&dentry->d_lru))
+		return;
+
+	if ((dentry->d_flags & DCACHE_SHRINK_LIST)) {
+		list_del_init(&dentry->d_lru);
+		dentry->d_flags &= ~DCACHE_SHRINK_LIST;
+	} else {
+		spin_lock(&dentry->d_sb->s_dentry_lru_lock);
+		__dentry_lru_del(dentry);
+		spin_unlock(&dentry->d_sb->s_dentry_lru_lock);
+	}
+}

Which is SHRINK_LIST aware. The code as it reads today after your patch
will only be correct if it is totally impossible for a dentry to be in
the shrink list before we reach both sites that call d_op->d_prune. They
are: dentry_kill and shrink_dcache_for_umount_subtree. So it seems to me
that we do really need to reintroduce dentry_lru_prune or just patch
both call sites with shrik-list aware code.

Comments ?