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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
