Re: namecache coherency 3rd turn

[Simon 'corecode' Schubert]

On 03.04.2006, at 09:51, Csaba Henk wrote:

+static struct shadowinfo *
+shadowinfo_fetch(void)
+{
+       struct shadowinfo *shinf = STAILQ_FIRST(&shadowinfo_freeq);
+
+       if (! shinf)
+               goto alloc;

shinf = objcache_alloc(shadowinfo_cache, M_WAITOK);

I'm strongly against adding a cache mechanism here. We should use a generic object cache whereever possible (and bring that one in shape, hint, hint). Then this would be a matter of: the approach you are taking is not MP-friendly at all, as it needs a lock, but I think you aware of this anyways. Just let us not do such optimizations prematurely. using malloc() should be sufficient right now.

@@ -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;

sncp is locked here:
[1]:

why is refs == 2? This would at least need a comment explaining this number.

+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



cheers
  simon

--
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \

thread2 will sleep on ncp, but will never get woken up. tsleep will return after some time and then access a non-existing ncp.

PGP.sig
Description: This is a digitally signed message part