Daniel Shahaf wrote:
> Daniel Shahaf wrote on Sat, 27 Dec 2008 at 20:50 +0200:
>> I tracked it to get_root_changes_offset(). It returns the offset from the
>> start of the revision, but later it's used to seek from the start of the
>> pack file.
>>
>> IOW, it forgets to add the offset of the rev within the pack file.
>>
>
> And here's the patch I'm testing:
>
> [[[
> Fix 'svnadmin verify' of some packed FSFS repositories by making the
> calculation of offsets in the rev file consistent.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (get_root_changes_offset):
> Return the offsets from the start of the pack file, not from the start
> of the revision data.
> (get_fs_id_at_offset):
> Don't repeat the logic that's now in get_root_changes_offset().
> ]]]
>
> [[[
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 34956)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -2239,14 +2239,6 @@ get_fs_id_at_offset(svn_fs_id_t **id_p,
> apr_hash_t *headers;
> const char *node_id_str;
>
> - if (rev < ffd->min_unpacked_rev)
> - {
> - apr_off_t rev_offset;
> -
> - SVN_ERR(get_packed_offset(&rev_offset, fs, rev, pool));
> - offset += rev_offset;
> - }
> -
> SVN_ERR(svn_io_file_seek(rev_file, APR_SET, &offset, pool));
>
> SVN_ERR(read_header_block(&headers,
> @@ -2287,6 +2279,7 @@ get_root_changes_offset(apr_off_t *root_offset,
> {
> fs_fs_data_t *ffd = fs->fsap_data;
> apr_off_t offset;
> + apr_off_t rev_offset;
> char buf[64];
> int i, num_bytes;
> apr_size_t len;
> @@ -2309,9 +2302,15 @@ get_root_changes_offset(apr_off_t *root_offset,
> else
> {
> seek_relative = APR_END;
> - offset = 0;
> + rev_offset = offset = 0;
I don't know what our style is here, but I recommend having these assignments on
separate lines, as it sure helps readability.
> }
>
> + /* Offset of the revision from the start of the pack file, if applicable. */
> + if (rev < ffd->min_unpacked_rev)
> + SVN_ERR(get_packed_offset(&rev_offset, fs, rev, pool));
> + else
> + rev_offset = 0;
> +
> /* We will assume that the last line containing the two offsets
> will never be longer than 64 characters. */
> SVN_ERR(svn_io_file_seek(rev_file, seek_relative, &offset, pool));
> @@ -2350,7 +2349,7 @@ get_root_changes_offset(apr_off_t *root_offset,
> i++;
>
> if (root_offset)
> - *root_offset = apr_atoi64(&buf[i]);
> + *root_offset = rev_offset + apr_atoi64(&buf[i]);
>
> /* find the next space */
> for ( ; i < (num_bytes - 2) ; i++)
> @@ -2365,7 +2364,7 @@ get_root_changes_offset(apr_off_t *root_offset,
> /* note that apr_atoi64() will stop reading as soon as it encounters
> the final newline. */
> if (changes_offset)
> - *changes_offset = apr_atoi64(&buf[i]);
> + *changes_offset = rev_offset + apr_atoi64(&buf[i]);
>
> return SVN_NO_ERROR;
> }
> ]]]
>
> I'll appreciate a review by someone more familiar with FSFS than me.
Other than that, it looks good. It took me a minute to figure out what was
going on here, but your solution looks reasonable.
> Daniel
>
> P.S. I've also noticed that 'svnadmin recover svn-r9-packed' (where
> svn-r9-packed is the repository created by the script I gave in the
> original mail) fails (both with and without the patch) with the error:
>
> ..\..\..\subversion\libsvn_fs_fs\fs_fs.c:6155: (apr_err=160004)
> svnadmin: Expected current rev to be <= 0 but found 9
Does this happen with the inconsistent repository you created, or with the sync
running to some sort of completion? What about a non-packed repository which
was aborted?
>> Daniel
>>
>> Daniel Shahaf wrote on Sat, 27 Dec 2008 at 15:39 +0200:
>>> Summary: 'svnadmin pack' can bring the repository to a state where
>>> 'svnadmin verify' fails.
>>>
>>>
>>> I can reproduce it with the svn repository, I couldn't find how to
>>> reproduce it in a Greek tree repository.
>>>
>>> Reproduction script:
>>>
>>> @echo off
>>>
>>> cd \tmp\svn
>>>
>>> echo @@@ Versions
>>> svn --version | head -2
>>> svnsync --version | head -2
>>> svnadmin --version | head -2
>>>
>>> echo @@@ Recreate the repository
>>> rd/s/q r
>>> svnadmin create r
>>> touch r\hooks\pre-revprop-change.bat
>>>
>>> echo @@@ Set small shards (so packing has an effect)
>>> ### Couldn't get perl to maintain EOL style.
>>> vim -Nneu NONE -i NONE -b -c "set noreadonly | 2s/sharded 1000/sharded 4/ | wq!" r\db\format
>>>
>>> echo @@@ Hack. Force the sync to abort after syncing r9.
>>> mkdir r\db\revprops\2\10
>>>
>>> echo @@@ Sync.
>>> svnsync init file:///tmp/svn/r http://svn.collab.net/repos/svn
>>> svnsync sync file:///tmp/svn/r
>>> echo @@@ The above error was expected.
>>>
>>> echo @@@ This will work:
>>> svnadmin verify -r2 r
>>>
>>> echo @@@ Packing...
>>> svnadmin pack r
>>>
>>> echo @@@ This will fail:
>>> svnadmin verify -r2 r
>>>
>>>
>>> Output:
>>>
>>> @@@ Versions
>>> svn, version 1.6.0 (dev build-r34949)
>>> compiled Dec 27 2008, 00:28:10
>>> svnsync, version 1.6.0 (dev build-r34949)
>>> compiled Dec 27 2008, 00:28:10
>>> svnadmin, version 1.6.0 (dev build-r34949)
>>> compiled Dec 27 2008, 00:28:10
>>> ### I actually used r34952; the output is wrong.
>>> @@@ Recreate the repository
>>> @@@ Set small shards (so packing has an effect)
>>> @@@ Hack. Force the sync to abort after syncing r9.
>>> @@@ Sync.
>>> Copied properties for revision 0.
>>> Transmitting file data .........................................................
>>> ................................................................................
>>> ................................................................................
>>> ................................................................................
>>> ................................................................................
>>> ................................................................................
>>> .........................................................
>>> Committed revision 1.
>>> Copied properties for revision 1.
>>> Transmitting file data .
>>> Committed revision 2.
>>> Copied properties for revision 2.
>>> Transmitting file data .
>>> Committed revision 3.
>>> Copied properties for revision 3.
>>> Transmitting file data .
>>> Committed revision 4.
>>> Copied properties for revision 4.
>>> Transmitting file data .
>>> Committed revision 5.
>>> Copied properties for revision 5.
>>> Transmitting file data .
>>> Committed revision 6.
>>> Copied properties for revision 6.
>>> Transmitting file data .
>>> Committed revision 7.
>>> Copied properties for revision 7.
>>> Transmitting file data .
>>> Committed revision 8.
>>> Copied properties for revision 8.
>>> Transmitting file data .
>>> Committed revision 9.
>>> Copied properties for revision 9.
>>> Transmitting file data .
>>> ..\..\..\subversion\libsvn_subr\io.c:2900: (apr_err=720005)
>>> svnsync: Can't move '\tmp\svn\r\db\transactions\9-9.txn\props' to '\tmp\svn\r\db\revprops\2\10': Access is denied.
>>> @@@ The above error was expected.
>>> @@@ This will work:
>>> * Verified revision 2.
>>> @@@ Packing...
>>> @@@ This will fail:
>>> ..\..\..\subversion\libsvn_fs_fs\fs_fs.c:3695: (apr_err=160004)
>>> svnadmin: Invalid changes line in rev-file
>>>
>>> Daniel
>>>
>>> ------------------------------------------------------
>>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=993717
>>>
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=993838
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=993860
Received on 2008-12-27 21:01:02 CET