Re: [ANNOUNCE] CGit v0.1-pre

From: Lars Hjemli <hjemli@gmail.com>
Date: Dec 11, 2006, 17:40 UTC
Message-ID: <8c5c35580612110940p767fd360p52762a8818fbc8c1@mail.gmail.com>
In-Reply-To: <Pine.LNX.4.64.0612110859200.12500@woody.osdl.org>

On 12/11/06, Linus Torvalds <torvalds@osdl.org> wrote:

Show 16 quoted lines

>
> On Mon, 11 Dec 2006, Lars Hjemli wrote:
> > 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)

Show 8 quoted lines

> > > 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?