[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Locks and recoveries of packed repositories (was: Re: packing causes 'svnadmin verify' to fail)

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Sun, 28 Dec 2008 15:36:43 +0200 (Jerusalem Standard Time)

Daniel Shahaf wrote on Sat, 27 Dec 2008 at 23:42 +0200:
> Hyrum K. Wright wrote on Sat, 27 Dec 2008 at 14:00 -0600:
> > Daniel Shahaf wrote:
> > > 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?
> >
>
> It happens on a 3-revisions packed Greek repository, and doesn't happen
> on the same repository after I dump|load it to get a non-packed version.
> The results with the packed/non-packed syncs of the svn repository are
> consistent (error only with the packed sync). In short, it seems to
> happen iff the repos is packed.
>

And a patch:

[[[
Fix 'svnadmin recover' of packed FSFS repositories.

* subversion/libsvn_fs_fs/fs_fs.c
  (recover_get_largest_revision):
    Use open_pack_or_rev_file() instead of svn_fs_fs__path_rev() to look
    for the rev files.
]]]

[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 34960)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -5878,13 +5881,19 @@ recover_get_largest_revision(svn_fs_t *fs, svn_rev
   /* Keep doubling right, until we find a revision that doesn't exist. */
   while (1)
     {
- svn_node_kind_t kind;
- SVN_ERR(svn_io_check_path(svn_fs_fs__path_rev(fs, right, iterpool),
- &kind, iterpool));
+ svn_error_t *err;
+ apr_file_t *file;
+
+ err = open_pack_or_rev_file(&file, fs, right, iterpool);
       svn_pool_clear(iterpool);
 
- if (kind == svn_node_none)
- break;
+ if (err && err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
+ {
+ svn_error_clear(err);
+ break;
+ }
+ else
+ SVN_ERR(err);
 
       right <<= 1;
     }
@@ -5896,16 +5905,22 @@ recover_get_largest_revision(svn_fs_t *fs, svn_rev
   while (left + 1 < right)
     {
       svn_revnum_t probe = left + ((right - left) / 2);
- svn_node_kind_t kind;
+ svn_error_t *err;
+ apr_file_t *file;
 
- SVN_ERR(svn_io_check_path(svn_fs_fs__path_rev(fs, probe, iterpool),
- &kind, iterpool));
+ err = open_pack_or_rev_file(&file, fs, probe, iterpool);
       svn_pool_clear(iterpool);
 
- if (kind == svn_node_none)
- right = probe;
+ if (err && err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
+ {
+ svn_error_clear(err);
+ right = probe;
+ }
       else
- left = probe;
+ {
+ SVN_ERR(err);
+ left = probe;
+ }
     }
 
   svn_pool_destroy(iterpool);
]]]

The root of the problem was that svn_fs_fs__path_rev() only works
correctly for non-packed revs. I suggest also the following patch:

[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 34960)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -196,6 +196,9 @@ svn_fs_fs__path_rev(svn_fs_t *fs, svn_revnum_t rev
 
   if (ffd->max_files_per_dir)
     {
+ /* We assume here that REV is not yet in a pack file. */
+ assert(rev >= ffd->min_unpacked_rev);
+
       return svn_path_join(path_rev_shard(fs, rev, pool),
                            apr_psprintf(pool, "%ld", rev),
                            pool);
Index: subversion/libsvn_fs_fs/fs_fs.h
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.h (revision 34960)
+++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
@@ -402,6 +402,7 @@ svn_error_t *svn_fs_fs__dup_perms(const char *file
                                   apr_pool_t *pool);
 
 /* Return the path to the file containing revision REV in FS.
+ Only works when REV has not yet been packed.
    Allocate the new char * from POOL. */
 const char *svn_fs_fs__path_rev(svn_fs_t *fs,
                                 svn_revnum_t rev,
]]]

I have checked all callers of __dup_perms(), and I think they should all
be okay, except for this one:

[[[
Index: subversion/libsvn_fs_fs/lock.c
===================================================================
--- subversion/libsvn_fs_fs/lock.c (revision 34960)
+++ subversion/libsvn_fs_fs/lock.c (working copy)
@@ -206,7 +206,7 @@ write_digest_file(apr_hash_t *children,
 
   SVN_ERR(svn_stream_close(stream));
   SVN_ERR(svn_io_file_rename(tmp_path, digest_path, pool));
- return svn_fs_fs__dup_perms
+ return svn_fs_fs__dup_perms /* XXX triggers the assertion here */
          (digest_path, svn_fs_fs__path_rev(fs, 0, pool), pool);
 }
 
]]]

which triggers the assertion added by the second patch. (This code path
is run during 'svn lock'.)

Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=994384
Received on 2008-12-28 14:48:39 CET

This is an archived mail posted to the Subversion Dev mailing list.