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

Glauber Costa
2013-05-14T12:43:04Z

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 ?