DragonFlyBSD

Minor update:

- made cache code topology agnostic (ie., tree layout hints [height] are
   maintained but not used by the cache code itself, group members are
   treated uniformly);

- made group traversal routine public;

- panic fix for nullfs.

The respective diffs for the cache system and nullfs are available here:

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

as well as incremental diffs from the latest posted patches.

Regards,
Csaba

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;

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:

shinf = objcache_alloc(shadowinfo_cache, M_WAITOK);

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;

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

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) {

[..]

[..]

+ 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.

cheers
simon

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

There has been a lot of work on the namecache since this was posted; is it
still of interest?

Cheers,
Alex Hornung

I think these things have been dealt with since then. (Pls confirm someone who is
more involved than me in Dfly...(O

Csaba