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
