svn commit: r1605633 - /subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c

[ko...@apache.org]

Author: kotkov
Date: Wed Jun 25 23:40:08 2014
New Revision: 1605633

URL: http://svn.apache.org/r1605633
Log:
Drop the workaround for concurrent 'hotcopy' and 'pack' race, because it is
broken in at least one common scenario.  The workaround only applies to FSFS
formats 4-6, because in FSFS 7 these operations are always serialized via the
new 'pack-lock' file.  Prior to this changeset, we were trying to be smart in
this racy situation and tried the following (see hotcopy_revisions()):

 - Say, at some point we are done with copying packed shards and are now
   copying the unpacked revisions one-by-one.

 - We know the source path for the unpacked revision file
   (e.g. /db/revs/1/358).

 - Whenever an attempt to copy that revision file raises an ENOENT error,
   we assume that this happened due to a concurrent pack.  In this case we
   determine the pack location for this revision (/db/revs/1.pack/) and
   copy it (and only it!) instead.

The last step is not good enough, because it might actually render the backup
destination unusable and painful to recover.  For instance, imagine running
'hotcopy' for a fully unpacked source repository with several shards:

    [ unpacked ]    [ unpacked ]      [ always unpacked ]
  0             1000             2000                     2537 (youngest)
                           (min-unpacked-rev)

Now, somewhere in the middle of copying the second shard, we invoke a
concurrent pack.  The hotcopy has a workaround for that.  So it says: "Aha,
this revision I was about to copy got packed in the source.  Fine, just copy
the corresponding (second!) packed shard."  When this hotcopy operation
finishes, the destination would look like this ...

    [ unpacked !!! ]    [ packed ]      [ always unpacked ]
  0                 1000           2000                     2537 (youngest)
                             (min-unpacked-rev)

...and this repository is broken, because it contains unpacked revisions
in the [0, min-unpacked-rev) range.  It is unusable, cannot be automatically
recovered and that is definitely not something one would like to see as a
data backup.

A general solution to this problem would probably mean copying everything
from the start in some cases.  However, having a possibility of a retry loop
in a backup routine seems to be worse than exiting with an ENOENT error
("E720003: Can't open file 'db\revs\2\358' ...").  In the latter case, the
hotcopy destination is still valid, just not up-to-date and one can run the
incremental hotcopy after the packing is done.  As an another alternative, we
could tweak the existing workaround and only permit it for the *last* packed
shard, which should be safe to copy.  However, this would look like we are
trying to solve an edge case of an edge case.

I chose to entirely drop this code.  Backup routine that fails in an edge
case (without wreaking havoc) is a better choice compared to a backup routine
that might work in an edge case, but also might produce broken backups.

* subversion/libsvn_fs_fs/hotcopy.c
  (hotcopy_revisions): Do not attempt to workaround a possible scenario of
    concurrent packing.  Rework the existing comment in order to preserve the
    information about the possible race condition and justify why we no
    longer try to handle it.

Modified:
    subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c

Modified: subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c?rev=1605633&r1=1605632&r2=1605633&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c Wed Jun 25 23:40:08 2014
@@ -693,63 +693,28 @@ hotcopy_revisions(svn_revnum_t *dst_youn
    * If necessary, update 'current' after copying all files from a shard. */
   for (; rev <= src_youngest; rev++)
     {
-      svn_error_t *err;
-
       svn_pool_clear(iterpool);
 
       if (cancel_func)
         SVN_ERR(cancel_func(cancel_baton));
 
-      /* Copy the rev file. */
-      err = hotcopy_copy_shard_file(src_revs_dir, dst_revs_dir,
-                                    rev, max_files_per_dir,
-                                    iterpool);
-      if (err)
-        {
-          if (APR_STATUS_IS_ENOENT(err->apr_err) &&
-              src_ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
-            {
-              svn_error_clear(err);
-
-              /* The source rev file does not exist. This can happen if the
-               * source repository is being packed concurrently with this
-               * hotcopy operation.
-               *
-               * If the new revision is now packed, and the youngest revision
-               * we're interested in is not inside this pack, try to copy the
-               * pack instead.
-               *
-               * If the youngest revision ended up being packed, don't try
-               * to be smart and work around this. Just abort the hotcopy. */
-              SVN_ERR(svn_fs_fs__update_min_unpacked_rev(src_fs, pool));
-              if (svn_fs_fs__is_packed_rev(src_fs, rev))
-                {
-                  if (svn_fs_fs__is_packed_rev(src_fs, src_youngest))
-                    return svn_error_createf(
-                             SVN_ERR_FS_NO_SUCH_REVISION, NULL,
-                             _("The assumed HEAD revision (%lu) of the "
-                               "hotcopy source has been packed while the "
-                               "hotcopy was in progress; please restart "
-                               "the hotcopy operation"),
-                             src_youngest);
-
-                  SVN_ERR(hotcopy_copy_packed_shard(&dst_min_unpacked_rev,
-                                                    src_fs, dst_fs,
-                                                    rev, max_files_per_dir,
-                                                    iterpool));
-                  rev = dst_min_unpacked_rev;
-                  continue;
-                }
-              else
-                return svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, NULL,
-                                         _("Revision %lu disappeared from the "
-                                           "hotcopy source while hotcopy was "
-                                           "in progress"), rev);
-            }
-          else
-            return svn_error_trace(err);
-        }
+      /* Copying non-packed revisions is racy in case the source repository is
+       * being packed concurrently with this hotcopy operation. The race can
+       * happen with FS formats prior to SVN_FS_FS__MIN_PACK_LOCK_FORMAT that
+       * support packed revisions. With the pack lock, however, the race is
+       * impossible, because hotcopy and pack operations block each other.
+       *
+       * We assume that all revisions coming after 'min-unpacked-rev' really
+       * are unpacked and that's not necessarily true with concurrent packing.
+       * Don't try to be smart in this edge case, because handling it properly
+       * might require copying *everything* from the start. Just abort the
+       * hotcopy with an ENOENT (revision file moved to a pack, so it is no
+       * longer where we expect it to be). */
 
+      /* Copy the rev file. */
+      SVN_ERR(hotcopy_copy_shard_file(src_revs_dir, dst_revs_dir,
+                                      rev, max_files_per_dir,
+                                      iterpool));
       /* Copy the revprop file. */
       SVN_ERR(hotcopy_copy_shard_file(src_revprops_dir,
                                       dst_revprops_dir,