Re: [PATCH 2/5] vfs: d_genocide() doesnt add dentries to unused list

[Jan Blunck]

On Mon, Jun 19, Balbir Singh wrote:

> > 			this_parent = dentry;
> > 			goto repeat;
> > 		}
> >-		atomic_dec(&dentry->d_count);
> >+		if (!list_empty(&dentry->d_lru)) {
> >+			dentry_stat.nr_unused--;
> >+			list_del_init(&dentry->d_lru);
> >+		}
> >+		if (atomic_dec_and_test(&dentry->d_count)) {
> >+			list_add(&dentry->d_lru, dentry_unused.prev);
> >+			dentry_stat.nr_unused++;
> >+		}
> 
> We could have dentries on the LRU list with non-zero d_count. If
> we have a dentry on the LRU list with a count of 1, then the code
> will remove it from LRU list and then add it back subsequently.
> 

So you think this is better?

   if (atomic_dec_and_test(&dentry->d_count)) {
      if (!list_empty(&dentry_d_lru))
         list_move_tail(&dentry->d_lru, dentry_unused);
   } else
      if (!list_empty(&dentry->d_lru)) {
         dentry_stat.nr_unused--;
         list_del_init(&dentry->d_lru);
      }


> I think the condition below should be an else if
> 

No. We always lower the reference count in d_genocide.

> 
> d_genocide() now almost looks like select_parent(). I think we can share a 
> lot
> of code between the two.
> 

Hmm, interesting idea. This would save the dentry-tree walking code in
have_submounts too. Maybe something like this:

+static int select_parent_walker(struct dentry * dentry, int * found)
+{
+       if (!list_empty(&dentry->d_lru)) {
+               dentry_stat.nr_unused--;
+               list_del_init(&dentry->d_lru);
+       }
+
+       /*
+        * move only zero ref count dentries to the end
+        * of the unused list for prune_dcache
+        */
+       if (!atomic_read(&dentry->d_count)) {
+               list_add(&dentry->d_lru, dentry_unused.prev);
+               dentry_stat.nr_unused++;
+               *found++;
+       }
+
+       /*
+        * We can return to the caller if we have found some (this
+        * ensures forward progress). We'll be coming back to find
+        * the rest.
+        */
+       if (*found && need_resched())
+               return -1;
+
+       return 0;
+}
+
+typedef int (*walker_t)(struct dentry * dentry, int * return);
+
+static int dentry_tree_walk(struct dentry * parent, walker_t walker)
+{
+       struct dentry *this_parent = parent;
+       struct list_head *next;
+       int ret = 0;
+
+       spin_lock(&dcache_lock);
+repeat:
+       next = this_parent->d_subdirs.next;
+resume:
+       while (next != &this_parent->d_subdirs) {
+               struct list_head *tmp = next;
+               struct dentry *dentry = list_entry(tmp, struct dentry,
+                                                  d_u.d_child);
+               next = tmp->next;
+
+               if (walker(dentry, &ret))
+                       goto out;
+
+               /*
+                * Descend a level if the d_subdirs list is non-empty.
+                */
+               if (!list_empty(&dentry->d_subdirs)) {
+                       this_parent = dentry;
+                       goto repeat;
+               }
+       }
+       /*
+        * All done at this level ... ascend and resume the search.
+        */
+       if (this_parent != parent) {
+               next = this_parent->d_u.d_child.next;
+               this_parent = this_parent->d_parent;
+               goto resume;
+       }
+out:
+       spin_unlock(&dcache_lock);
+       return ret;
+}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/