+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;
