Re: namecache coherency 3rd turn

[Csaba Henk <csaba.henk@xxxxxxx>]

On Mon, Apr 03, 2006 at 01:24:50PM +0200, Simon 'corecode' Schubert wrote:

I updated the patch according to your suggestions (plus fixed a panic). See
again

http://creo.hu/~csaba/tmp/visible/dfly/ ,

updated.

> generic object cache whereever possible (and bring that one in shape,
> hint, hint).  Then this would be a matter of:
> 
> shinf = objcache_alloc(shadowinfo_cache, M_WAITOK);

What do you think of? In what ways would this be more than a
macro-glorified version of the simplistic design I had for shadowinfo?
Would it use low-level optimized allocation routines?

> Just let us not do such optimizations prematurely.  using malloc()
> should be sufficient right now.

Well OK, that just made the code simpler ATM.

> >@@ -295,6 +360,10 @@ cache_alloc(int nlen)
> > 	ncp->nc_error = ENOTCONN;	/* needs to be resolved */
> > 	ncp->nc_refs = 1;
> > 	ncp->nc_fsmid = 1;
> >+	ncp->nc_shadowinfo = &ncp->nc_shadowinfo_internal;
> >+	ncp->nc_shadowinfo_internal.sh_refs = 2;
> >+	ncp->nc_shadow_prev = NULL;
> >+	ncp->nc_shadow_next = NULL;
> 
> why is refs == 2?  This would at least need a comment explaining this 
> number.


Sure, I added:

/*
 * nc_shadowinfo_internal:
 * one ref for being contained in ncp, one for ncp being locked via it.
 * (This leverages us from dealing with "internal" vs. "standalone"
 * when dealing with shadowinfo references).
 */

Alternatively, there could be a flag displaying if the shadowinfo is
internal or standalone, and make the ref/put routines act based on this.
Would that be preferable?

> sncp is locked here:
> [1]:
> >+int
> >+cache_shadow_attach(struct namecache *ncp, struct namecache *sncp)
> >+{
> >+	struct migrate_param mpm;
> [..]
> >+	if (sncp->nc_shadowinfo == &sncp->nc_shadowinfo_internal) {
> >+		mpm.heightdelta = 0;
> >+		mpm.shadowinfo = shadowinfo_fetch();
> >+		mpm.exlocks = sncp->nc_shadowinfo->sh_exlocks;
> >+		migrate_updater(sncp, &mpm);
> >+	}
> 
> [2]:
> [..]
> >@@ -372,16 +628,15 @@ cache_lock(struct namecache *ncp)
> [..]
> >-		ncp->nc_flag |= NCF_LOCKREQ;
> >-		if (tsleep(ncp, 0, "clock", nclockwarn) == EWOULDBLOCK) {
> >+		shinf->sh_lockreq = 1;
> >+		if (tsleep(shinf, 0, "clock", nclockwarn) == EWOULDBLOCK) {
> [..]
> 
> [3]:
> >@@ -436,17 +695,45 @@ cache_unlock(struct namecache *ncp)
> > cache_unlock(struct namecache *ncp)
> > {
> [..]
> >+	if (--shinf->sh_exlocks == 0) {
> >+		shinf->sh_locktd = NULL;
> >+		if (shinf->sh_lockreq) {
> >+			shinf->sh_lockreq = 0;
> >+			wakeup(shinf);
> 
> 
> I think there is a race condition which Matt already pointed out:
> 
> Thread1		Thread2
> [2]
> 
> 			[2] (tsleeps on ncp->nc_shadowinfo_internal)
> 
> [1] (migrates shinfo to an external struct)
> [3] (wakes up external shinfo)
> ncp is unlocked and gets freed
> 
> thread2 will sleep on ncp, but will never get woken up.  tsleep will 
> return after some time and then access a non-existing ncp.

Right -- I just forgot about it... Now I simply inserted a wakeup() before
putting the shadowinfo in the migration routine.

AFAICS that's enough and we don't need to hold an extra ref for the
shadowinfo while sleeping on it in cache_lock(), because the variable
"shinf" gets re-initialized when sleeping is over (to the same value or
to something else -- that doesn't make a difference for the lock
routine).

Regards,
Csaba