You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2012/08/21 19:00:23 UTC
FS vtable common_pool and pack_fs/recover
The FS loader uses a vtable fs_library_vtable_t defined in fs-loader.h.
Some of the functions defined in the vtable have a common_pool
parameter:
/* The open_fs/create/open_fs_for_recovery/upgrade_fs functions are
serialized so that they may use the common_pool parameter to
allocate fs-global objects such as the bdb env cache. */
svn_error_t *(*create)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
apr_pool_t *common_pool);
svn_error_t *(*open_fs)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
apr_pool_t *common_pool);
In FSFS this common_pool gets passed to fs_serialized_init in fs.c where
it is used to setup setup mutexes:
status = apr_pool_userdata_get(&val, key, common_pool);
if (status)
return svn_error_wrap_apr(status, _("Can't fetch FSFS shared data"));
ffsd = val;
if (!ffsd)
{
ffsd = apr_pcalloc(common_pool, sizeof(*ffsd));
ffsd->common_pool = common_pool;
/* POSIX fcntl locks are per-process, so we need a mutex for
intra-process synchronization when grabbing the repository write
lock. */
SVN_ERR(svn_mutex__init(&ffsd->fs_write_lock,
SVN_FS_FS__USE_LOCK_MUTEX, common_pool));
Functions like fs_library_vtable_t.pack_fs and
fs_library_vtable_t.recover that don't take the common_pool parameter
pass the pool parameter to fs_serialized_init. I think that means these
functions should only be used from a single threaded process.
Is that correct? If so we should document it or change the functions to
handle the common_pool.
I'm not sure how the absence of common_pool affects the BDB
implementations.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: FS vtable common_pool and pack_fs/recover
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Philip Martin wrote on Tue, Aug 21, 2012 at 20:42:55 +0100:
>> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>>
>> > Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
>> >> Functions like fs_library_vtable_t.pack_fs and
>> >> fs_library_vtable_t.recover that don't take the common_pool parameter
>> >> pass the pool parameter to fs_serialized_init. I think that means these
>> >> functions should only be used from a single threaded process.
>> >>
>> >> Is that correct? If so we should document it or change the functions to
>> >> handle the common_pool.
>> >>
>> >
>> > I think the correct fix is to propagate common_pool to have fs_pack()
>> > and pass it as to the appropriate parameter of fs_serialized_init().
>>
>> Yes, I think that would be correct. It's probably possible to backport
>> to 1.7 since these functions are all private.
>>
>
> +1
Our API is odd. When I run 'svnadmin pack repo' the svnadmin code calls
svn_repos_open2 and svn_repos_fs_pack2.
The call to:
svn_repos_open2("repo")
calls:
svn_fs_open("repo/db")
which returns an svn_fs_t that is stored in the svn_repos_t returned by
svn_repos_open2. This FS handle uses common_pool correctly.
The call to
svn_repos_fs_pack2(svn_repos_t)
calls:
svn_fs_pack("repo/db")
and that in turn calls:
svn_fs_open("repo/db")
to get a second svn_fs_t. This second FS handle doesn't use common_pool
and so doesn't properly exclude other threads.
It seems that several of the FS functions work like this, although pack
is the only one that fails to use the common_pool correctly when opening
the second handle.
Perhaps we should introduce new FS functions that accept an svn_fs_t
rather than a path? Or perhaps svn_repos_open2 should delay openning
the FS until the handle is needed?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: FS vtable common_pool and pack_fs/recover
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Tue, Aug 21, 2012 at 20:42:55 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
> >> Functions like fs_library_vtable_t.pack_fs and
> >> fs_library_vtable_t.recover that don't take the common_pool parameter
> >> pass the pool parameter to fs_serialized_init. I think that means these
> >> functions should only be used from a single threaded process.
> >>
> >> Is that correct? If so we should document it or change the functions to
> >> handle the common_pool.
> >>
> >
> > I think the correct fix is to propagate common_pool to have fs_pack()
> > and pass it as to the appropriate parameter of fs_serialized_init().
>
> Yes, I think that would be correct. It's probably possible to backport
> to 1.7 since these functions are all private.
>
+1
Re: FS vtable common_pool and pack_fs/recover
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
>> Functions like fs_library_vtable_t.pack_fs and
>> fs_library_vtable_t.recover that don't take the common_pool parameter
>> pass the pool parameter to fs_serialized_init. I think that means these
>> functions should only be used from a single threaded process.
>>
>> Is that correct? If so we should document it or change the functions to
>> handle the common_pool.
>>
>
> I think the correct fix is to propagate common_pool to have fs_pack()
> and pass it as to the appropriate parameter of fs_serialized_init().
Yes, I think that would be correct. It's probably possible to backport
to 1.7 since these functions are all private.
>> I'm not sure how the absence of common_pool affects the BDB
>> implementations.
>
> Is it even used? A quick grep in libsvn_fs_base suggests that only fs.c
> has variables by that name; therein, only svn_fs_base__init() uses them;
> and that function seems to have no callers(!?).
It's called by the FS dynamic loader via a function pointer when
get_library_vtable_direct calls initfunc.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: FS vtable common_pool and pack_fs/recover
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
> The FS loader uses a vtable fs_library_vtable_t defined in fs-loader.h.
> Some of the functions defined in the vtable have a common_pool
> parameter:
>
> /* The open_fs/create/open_fs_for_recovery/upgrade_fs functions are
> serialized so that they may use the common_pool parameter to
> allocate fs-global objects such as the bdb env cache. */
> svn_error_t *(*create)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
> apr_pool_t *common_pool);
> svn_error_t *(*open_fs)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
> apr_pool_t *common_pool);
>
>
> In FSFS this common_pool gets passed to fs_serialized_init in fs.c where
> it is used to setup setup mutexes:
>
> status = apr_pool_userdata_get(&val, key, common_pool);
> if (status)
> return svn_error_wrap_apr(status, _("Can't fetch FSFS shared data"));
> ffsd = val;
>
> if (!ffsd)
> {
> ffsd = apr_pcalloc(common_pool, sizeof(*ffsd));
> ffsd->common_pool = common_pool;
>
> /* POSIX fcntl locks are per-process, so we need a mutex for
> intra-process synchronization when grabbing the repository write
> lock. */
> SVN_ERR(svn_mutex__init(&ffsd->fs_write_lock,
> SVN_FS_FS__USE_LOCK_MUTEX, common_pool));
>
>
> Functions like fs_library_vtable_t.pack_fs and
> fs_library_vtable_t.recover that don't take the common_pool parameter
> pass the pool parameter to fs_serialized_init. I think that means these
> functions should only be used from a single threaded process.
>
> Is that correct? If so we should document it or change the functions to
> handle the common_pool.
>
I think the correct fix is to propagate common_pool to have fs_pack()
and pass it as to the appropriate parameter of fs_serialized_init().
> I'm not sure how the absence of common_pool affects the BDB
> implementations.
Is it even used? A quick grep in libsvn_fs_base suggests that only fs.c
has variables by that name; therein, only svn_fs_base__init() uses them;
and that function seems to have no callers(!?).
(These results are so odd that I'm pretty sure I typoed the argument to
grep at some point...)