Re: [PATCH] Destroy the dentries contributed by a superblock on unmounting

[Neil Brown]

On Saturday June 24, dhowells@xxxxxxxxxx wrote:
> 
> Neil Brown <neilb@xxxxxxx> wrote:
> 
> > Do you not have easy access to the roots of all trees in your
> > super-block-sharing situation so that shrink_dcache_parent can be
> > called on them all?
> 
> How about this?

I think this is definitely going in the right direction.
A few comments.

 - The test for IS_ROOT surprises me.  I thought anonymous dentries
   were all IS_ROOT.   Maybe this changes with your shared-superblock
   changes? 

 - You have two nested loops implemented with gotos.  Dijykstra would
   be shocked!  The logic looks like it should be:

     for(;;) {
        while (!list_empty(&dentry->d_subdirs)) {
		stuff-1;
		dentry = list_entry(dentry->d_subdirs.next,....)
	}
	while (list_empty(dentry->d_subdirs)) {
		parent = dentry->d_parent (or NULL)
		d_free(dentry)
		if (!parent) return;
		dentry = parent;
	}
     }

    Which I think makes it a lot more readable (obviously I have left
    out lots of the details in the above.

  - The section that I have called 'stuff-1' above seems excessive.
    Everytime you visit a dentry with children, you remove them from
    the unused list (if present) and d_drop them from the hash.  After
    the first time, these should all be no-ops.  If there some
    particular reason for this that I'm not seeing (which case I'd
    like a comment) or can you just unuse/drop the dentry just before
    freeing it

  - Think the reference counting deserves a comment.  I think that
    this routine holds a reference count on 'dentry' and every parent
    up to the IS_ROOT.  Is that right?

In summary, it looks right, though I feel I would want to go through
and double check the refcounting again, but I would rather do that
with it restructured into while loops.
Is that fair?

NeilBrown
-
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/