Blair Zajac wrote:
> 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?
Those issues have allegedly been fixed for the one-thread-per-fs_t case,
but they showed up as segfaults at shutdown time which IIRC resulted in
database recovery being run more often than necessary. Also not too
friendly to a threaded httpd.
> 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?
Maybe. Like I said, I'll have to remember what I was smoking^Wthinking
when I wrote that code. I'm sure there was a good reason that all access
wasn't through that function.
> The cleanup code is complicated and I'm still working on understanding it.
You don't say. I *wrote* it and still I have trouble understanding it. :(
-- Brane
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=980295
Received on 2008-12-08 10:41:40 CET