Re: FS vtable common_pool and pack_fs/recover

Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:

> Philip Martin wrote on Tue, Aug 21, 2012 at 20:42:55 +0100:
>> Daniel Shahaf <d.s_at_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