Philip Martin wrote:
> First issue: the new "spinlock" appears to use CAS more than
> necessary, I think it can be simplified to this:
>
Don't we all wish. Saith apr_atomic.h:
> * @warning do not mix apr_atomic's with the CAS function.
> * on some platforms they may be implemented by different mechanisms
> Second issue: bdb_cache_state is declared as "volatile svn__atomic_t"
> and always accessed through the svn__atomic_xxx functions, and that
> combination is obviously intended to provide "thread safety". I guess
> it's reasonable in practice :) However, the svn__atomic_xxx functions
> are also used to access the the panic flag in bdb_env_t and yet that
> flag is declared as plain svn_boolean_t, that's confusing.
>
Really? Saith bdb_env_t in env.c:
> svn__atomic_t panic;
It's not svn_boolean_t, but I agree it should be volatile. The return
value of svn_fs_bdb__get_panic is boolean, but that's irrelevant.
> If the svn__atomic_xxx calls are necessary then I think the panic flag
> should be "volatile svn__atomic_t", however I don't really see why the
> svn__atomic_xxx calls are needed or even why the flag is needed. In
> bdb_cache_get the flag is redundant, it serves to avoid a BDB
> get_flags() call but only when the flag is TRUE and there is little
> point in such micro-optimisations in the "recovery needed" path. The
> other uses of the panic flag are in the code used to close the
> database, again it seems unnecessary to avoid a get_flags() call in
> that code. I think we should remove the panic flag and substitute
> get_flags() calls in the code to close the database.
>
The real purpose of the panic flag is to avoid calls to DB_ENV->open if
we know that the cached environment handle is useless. Once the
environment is panicked, it's important to release all references to it
quickly, so that the queue of processes waiting to do the recovery
doesn't get too long.
At first I'd considered removing a broken environment from the cache,
but that created races that would've been really hard to avoid. So I
left it in the cache and marked it useless instead. This also made the
reference counting cleaner.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jan 28 02:40:19 2006