Branko Cibej wrote:
> Blair Zajac wrote:
>> Branko Cibej wrote:
>>> Blair Zajac wrote:
>>>> Are any of our BDB experts available to look at this issue and
>>>> patch? Brane, cmpilato :) It would be greatly appreciated.
>>>>
>>> I'm looking, I'm looking! I've managed to bump my review rate to two
>>> lines per day, heh.
>>> Sorry it's taking so long.
>> Thanks! We're working on a small test case that should demonstrate
>> this bug. Hopefully today we'll have it.
>
> So the first thing I managed to remember from way back when is that the
> answer to the question, "can I use the same svn_fs_t in different
> threads?" was assumed to be "no!" when I was initially coding up the
> DB_REGISTER support.
>
> Apparently either that assumption was naive, or you didn't read the
> non-existent docs when you wrote your server. :)
:)
> There's no reason not to remove that restriction, but I recall some very
> nasty interactions between threads and pool lifetimes and the
> db_register cache lifetime. I'll concentrate on trying to understand
> those interactions again and take a fresh look at them in the context of
> your patch.
By the "db_register cache lifetime" are you referring to the cache of BDB
environments in env.c? Are these lifetime issues only seen at shutdown time?
If you're reviewing the code and if the patch is a good one, I believe there's
then a reference counting issue with the refcount in bdb_error_info_t. Since
the patch now requires a call to get_error_info() to get the bdb_error_info_t,
any call by a new thread can create a new bdb_error_info_t with a 0 refcount.
But in svn_fs_bdb__close(), the refcount can be decremented to -1.
/* Note that we only bother with this cleanup if the pool is non-NULL, to
guard against potential races between this and the cleanup_env cleanup
callback. It's not clear if that can actually happen, but better safe
than sorry. */
if (0 == --bdb_baton->error_info->refcount && bdb->pool)
{
svn_error_clear(bdb_baton->error_info->pending_errors);
#if APR_HAS_THREADS
free(bdb_baton->error_info);
#endif
}
If all access is through get_error_info() which always returns per-thread
instance, maybe this code should be removed and we just use the
cleanup_error_info() passed to apr_threadkey_private_create() to free this memory?
The cleanup code is complicated and I'm still working on understanding it.
Blair
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=980248
Received on 2008-12-08 10:41:14 CET