Re: [ANNOUNCE] CGit v0.1-pre

On 12/11/06, Linus Torvalds [off-list ref] wrote:

On Mon, 11 Dec 2006, Lars Hjemli wrote:

quoted

Anyway, I must say I find it rather unlikely for these cases to occur
(frequently) in real life. That would seem to imply that the caching
isn't really needed at all.

The point is, if you have races, you will hit them _occasionally_. It may
not be a performance problem in real life, BUT:

 - quite often, you can take advantage of the serialization guarantees
   that a front-side cache offers you, and do the uncached accesses (or
   writing the final cache-file) knowing that there's only one thing that
   does that.

 - If so, then a race condition in the cache goes from a "unlikely
   performance problem" to a BUG.

Yes, I finally understood (see my other reply)

quoted

quoted

As a side note: how do you release your caches?

Simple timeouts (time()-stat.st_mtime), depending on what kind of page
was requested. If anyone cares about invalid cache content (branch
head moving), relevant cachefiles can be deleted with an update-hook.

I was more thinking about the locking part. Again, to be safe, you should
probably take the lock before releasing any cache.

Ok. Code speeks louder than words, so I'll blatantly paste the key functions:

--->8----

const int NOLOCK = -1;

int cache_lock(struct cacheitem *item)
{
	int i = 0;
	char *lockfile = fmt("%s.lock", item->name);

 top:
	if (++i > cgit_max_lock_attempts)
		die("cache_lock: unable to lock %s: %s",
		    item->name, strerror(errno));

       	item->fd = open(lockfile, O_WRONLY|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR);

	if (item->fd == NOLOCK && errno == ENOENT && cache_create_dirs())
		goto top;

	if (item->fd == NOLOCK && errno == EEXIST &&
	    cache_refill_overdue(lockfile) && !unlink(lockfile))
			goto top;

	return (item->fd > 0);
}

int cache_unlock(struct cacheitem *item)
{
	close(item->fd);
	return (rename(fmt("%s.lock", item->name), item->name) == 0);
}


static void cgit_check_cache(struct cacheitem *item)
{
	int i = 0;

	cache_prepare(item);
 top:
	if (++i > cgit_max_lock_attempts) {
		die("cgit_refresh_cache: unable to lock %s: %s",
		    item->name, strerror(errno));
	}
	if (!cache_exist(item)) {
		if (!cache_lock(item)) {
			sleep(1);
			goto top;
		}
		if (!cache_exist(item))
			cgit_fill_cache(item);
		cache_unlock(item);
	} else if (cache_expired(item) && cache_lock(item)) {
		if (cache_expired(item))
			cgit_fill_cache(item);
		cache_unlock(item);
	}
}

--->8----

I am a bit conserned about the effect of cache_unlink() if another
concurrent process gets "lucky" with the test
"cache_refill_overdue(lockfile) && !unlink(lockfile)".

This is supposed to be a safety valve against a stale lock file (lock
file not modified in n secs), but it doesn't feel quite right.

Probably, if cache_unlink() fails in cgit_check_cache(), I should "goto top".

Opinions?

--