Committed to the 1.6.x branch in r1103680. This fix will be in the
next release of Subversion.
Thanks again,
-Hyrum
On Mon, May 16, 2011 at 10:05 AM, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> It turns out that on trunk, we are using apr_int64_t which Daniel
> tells me is still incorrect, so r1103665 applies your patch there, and
> I've nominated it for 1.6.x (with Daniel's vote) in r1103668.
>
> Thanks!
>
> -Hyrum
>
> On Mon, May 16, 2011 at 10:39 AM, Daniel Shahaf <d...@daniel.shahaf.name>
> wrote:
>> +1 from Hyrum and I; we'll apply to trunk and nominate a backport.
>>
>> Roderich Schupp wrote on Sun, May 15, 2011 at 19:50:53 +0200:
>>> Hi,
>>>
>>> while looking at strace's of Subversion operations on a repository
>>> using packing
>>> I noticed the following behavior:
>>>
>>> 20829 open(".../db/revs/2.pack/pack", O_RDONLY|O_CLOEXEC) = 13
>>> 20829 open(".../db/revs/2.pack/manifest", O_RDONLY|O_CLOEXEC) = 14
>>> 20829 open(".../db/revs/2.pack/pack", O_RDONLY|O_CLOEXEC) = 14
>>> 20829 open(".../db/revs/2.pack/manifest", O_RDONLY|O_CLOEXEC) = 15
>>> 20829 open(".../db/revs/2.pack/pack", O_RDONLY|O_CLOEXEC) = 12
>>> 20829 open(".../db/revs/2.pack/manifest", O_RDONLY|O_CLOEXEC) = 13
>>>
>>> i.e. every open for a pack file is paired with an open for the corresponding
>>> manifest file. I would have expected that a specific manifest file is only
>>> read once and then its contents are cached in the packed_offset_cache field
>>> of the fs_fs_data_t struct. I saw this on Linux on amd64, i.e. where
>>>
>>> sizeof(int) < sizeof(rev_num_t)
>>>
>>> The packed_offset_cache never finds anything because of the following
>>> mismatch:
>>> - in subversion/libsvn_fs_fs/caching.c packed_offset_cache is created
>>> with a key length of
>>> sizeof(svn_revnum_t)
>>> - in subversion/libsvn_fs_fs/fs_fs.c in function get_packed_offset the
>>> shard number is passed
>>> to svn_cache__{get,set} as (the address of) an int
>>>
>>> Patch below. The problem is still present in branches/1.6.x, while trunk has
>>> rewritten packed_offset_cache() and shard is now an apr_int64_t (which
>>> will work,
>>> though still not literally consistent with the creation of the cache).
>>>
>>> Cheers, Roderich
>>>
>>>
>>> --- subversion-1.6.16.origsubversion/libsvn_fs_fs/fs_fs.c 2011-02-14
>>> 21:54:43.000000000 +0100
>>> +++ subversion-1.6.16/subversion/libsvn_fs_fs/fs_fs.c 2011-05-14
>>> 22:11:39.127424984 +0200
>>> @@ -1800,7 +1800,7 @@
>>> fs_fs_data_t *ffd = fs->fsap_data;
>>> svn_stream_t *manifest_stream;
>>> svn_boolean_t is_cached;
>>> - int shard;
>>> + svn_revnum_t shard;
>>> apr_array_header_t *manifest;
>>> apr_pool_t *iterpool;
>>
>
