Re: namecache coherency 3rd turn

["Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>]

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) {
[..]

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

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   / \

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