Re: cache_inval

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:First of all: thanks for your excellent VFS work Matt!
:That the kernel now only uses the new namecache API has
:made things much simpler for me. My port of arla's nnpfs
:is now working again.
:
:cache_inval doesn't seem to perform as advertized.
:I've inlined most of the function below and marked
:the portions I'm a bit confused about.
:[
:...
:	if (flags & CINV_CHILDREN) {
:		if ((kid = TAILQ_FIRST(&ncp->nc_list)) != NULL)
:			cache_hold(kid);
:		while (kid) {
:			if ((nextkid = TAILQ_NEXT(kid, nc_entry)) != NULL)
:				cache_hold(nextkid);
:			if (kid->nc_refs == 0 &&                 ??? [1]
:			    ((kid->nc_flag & NCF_UNRESOLVED) || 
:			     kid->nc_vp == NULL)
:			) {
:				cache_inval(kid, CINV_PARENT);   ??? [2]
:			}
:			cache_unlink_parent(kid);
:			cache_drop(kid);
:			kid = nextkid;
:		}
:	}
:]
:
:??? [1]: Isn't kid->nc_refs always > 0 here? We have done a cache_hold on
:         the kid earlier in the code.

    "oops".

:??? [2]: Shouldn't it be CINV_CHILDREN instead of CINV_PARENT?
:         The comments talk about recursing but it's really just
:         doing cache_unlink_parent(kid) which is done after anyway.

    "oops oops".  As you indicate later in your email, it should be
    CINV_CHILDREN|CINV_PARENT.  The CINV_PARENT is what unlinks the kid,
    but without the recursion we can still be left with dangling 
    namecache records.

    I'm going to have to think about this a bit.  I already have an XXX
    in there due to the recursion, and there are other problems which
    I will outline below.

:I'm using a similar function to cache_inval in nnpfs.
:I recurse on children to set them to unresolved.
:The reason for this is that I can get a message saying
:that the contents of a directory has changed so
:1) I don't want to unlink children that still exists and
:2) I want to force reresolving so that unexisting children
:   isn't still in the cache.
:Is it safe to do this? Except for the kernel stack issue :)

    This is an excellent description of what I *should* have done :-).
    You may not have realized this, but the recursion is *mandatory*
    in this case because the only way the topology can be destroyed
    naturally is bottom-up.  i.e. cache_drop() only destroys a NCP
    if it has 0 refs (after the drop) AND no children AND is unresolved.
    So by recursing all the way to the leafs you get the beneficial
    side effect of cache_drop() cleaning out the hierarchy as the
    recursion returns back up the tree.  The recursion depth problem is
    still an issue, but it's the only issue.

    I think I will rework the cache_inval() code to do precisely 
    that... set the records to an unresolved state but not attempt to
    destroy the sub-topology.  It makes a lot of sense... I've already
    invested a great deal of time making sure the topology stays intact,
    I probably shouldn't go destroying it here.

    There is one big difference between what you are doing and what the
    callers of cache_inval(... CINV_CHILDREN) expect, which I will also
    have to solve.  Both or valid scenarios and I think both have to be
    officially supported.  The issue is really simple:

	mkdir ~/test
	cd ~/test
	rm -rf ~/test
	mkdir ~/test
	ls		<<<<<<< should fail, the new test is not the same
				as the 'test' we originally CD'd into.  The
				old 'test' is no longer connected with the
				original topology.

    The only callers of cache_inval(CINV_CHILDREN) are cache_rename() and
    NFS's sillyrename... they make the call to destroy the namecache 
    topology for the target of a rename.  e.g. if you rename "a" to "b"
    and "b" already exists, then "b" is being destroyed by the rename and
    there is no namecache topology left for "b".  Anyone with a reference
    to "b" has to see that reference go dead (become unresolvable), even if
    "b" is created later on.

    But even so I agree with you that the topological *structure* should
    remain intact.  What I think I will do is add an NCP flag for the
    resolver so it can detect a 'dead' node in the topology and then either
    NULL-out the name part of the ncp or, even better, leave the name intact
    but make cache_nlookup() ignore it if it is flagged as having been
    destroyed.  This way we won't have the kernel ripping parent NCP's out
    from under any referenced children under any circumstances.

    I will test and put together a patch set on Tuesday.

    I really appreciate the analysis, Richard!

					-Matt
					Matthew Dillon 
					<dillon@xxxxxxxxxxxxx>

:[
:		if ((kid = TAILQ_FIRST(&ncp->nc_list)) != NULL)
:			cache_hold(kid);
:		while (kid) {
:			if ((nextkid = TAILQ_NEXT(kid, nc_entry)) != NULL)
:				cache_hold(nextkid);
:                        nnpfs_inval(kid, CINV_SELF|CINV_CHILDREN);
:			cache_drop(kid);
:			kid = nextkid;
:		}
:]
:
:Happy hacking!
:        -Richard