You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Max Bowsher <ma...@ukf.net> on 2004/12/16 02:44:18 UTC
Re: svn commit: r12327 - trunk/subversion/libsvn_repos
maxb@tigris.org wrote:
> Author: maxb
> Date: Wed Dec 15 20:28:24 2004
> New Revision: 12327
>
> Modified:
> trunk/subversion/libsvn_repos/load.c
> Log:
> Fix a nasty race condition in 'svnadmin load', which could result in
> incorrect
> copyfrom revisions being chosen if other commits were occurring at the
> same
> time as the load - a common occurrence in large active repositories, like
> the
> ASF main repository.
>
> * subversion/libsvn_repos/load.c (new_revision_record): Move the recording
> of
> the dump-file-revision -> in-repos-revision mapping from here, where the
> in-repos-revision is not yet known, and was being guessed as HEAD+1, to
> ...
> (close_revision): ... here, where the actual in-repos-revision is known.
This patch was written at 2am during an IRC conversation on #cvs2svn with
jerenkrantz, who discovered the bug when trying to load a newly converted
project into the ASF main repository.
Whilst I feel confident in the change, given that it went through the entire
cycle of debug->commit in less than an hour, at 2am in the morning, I'd
quite like a review on this one.
Max.
> Modified: trunk/subversion/libsvn_repos/load.c
> Url:
> http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_repos/load.c?view=diff&rev=12327&p1=trunk/subversion/libsvn_repos/load.c&r1=12326&p2=trunk/subversion/libsvn_repos/load.c&r2=12327
> ==============================================================================
> --- trunk/subversion/libsvn_repos/load.c (original) +++
> trunk/subversion/libsvn_repos/load.c Wed Dec 15 20:28:24 2004 @@ -801,22
> +801,16 @@ struct parse_baton *pb = parse_baton;
> struct revision_baton *rb;
> svn_revnum_t head_rev;
> - svn_revnum_t *old_rev, *new_rev;
>
> rb = make_revision_baton (headers, pb, pool);
> SVN_ERR (svn_fs_youngest_rev (&head_rev, pb->fs, pool));
>
> - /* Calculate the revision 'offset' for finding copyfrom sources.
> + /* FIXME: This is a lame fallback loading multiple segments of dump in
> + several seperate operations. It is highly susceptible to race
> conditions. + Calculate the revision 'offset' for finding copyfrom
> sources. It might be positive or negative. */
> rb->rev_offset = (rb->rev) - (head_rev + 1);
>
> - /* Store the new revision for finding copyfrom sources. */
> - old_rev = apr_palloc (pb->pool, sizeof(svn_revnum_t) * 2);
> - new_rev = old_rev + 1;
> - *old_rev = rb->rev;
> - *new_rev = head_rev + 1;
> - apr_hash_set (pb->rev_map, old_rev, sizeof(svn_revnum_t), new_rev);
> -
> if (rb->rev > 0)
> {
> /* Create a new fs txn. */
> @@ -1110,13 +1104,18 @@
> struct revision_baton *rb = baton;
> struct parse_baton *pb = rb->pb;
> const char *conflict_msg = NULL;
> - svn_revnum_t new_rev;
> + svn_revnum_t *old_rev, *new_rev;
> svn_error_t *err;
>
> if (rb->rev <= 0)
> return SVN_NO_ERROR;
>
> - err = svn_fs_commit_txn (&conflict_msg, &new_rev, rb->txn, rb->pool);
> + /* Prepare memory for saving dump-rev -> in-repos-rev mapping. */
> + old_rev = apr_palloc (pb->pool, sizeof(svn_revnum_t) * 2);
> + new_rev = old_rev + 1;
> + *old_rev = rb->rev;
> +
> + err = svn_fs_commit_txn (&conflict_msg, &(*new_rev), rb->txn,
> rb->pool);
>
> if (err)
> {
> @@ -1127,29 +1126,34 @@
> return err;
> }
>
> + /* After a successful commit, must record the dump-rev -> in-repos-rev
> + mapping, so that copyfrom instructions in the dump file can look up
> the
> + correct repository revision to copy from. */
> + apr_hash_set (pb->rev_map, old_rev, sizeof(svn_revnum_t), new_rev);
> +
> /* Deltify the predecessors of paths changed in this revision. */
> - SVN_ERR (svn_fs_deltify_revision (pb->fs, new_rev, rb->pool));
> + SVN_ERR (svn_fs_deltify_revision (pb->fs, *new_rev, rb->pool));
>
> /* Grrr, svn_fs_commit_txn rewrites the datestamp property to the
> current clock-time. We don't want that, we want to preserve
> history exactly. Good thing revision props aren't versioned! */
> if (rb->datestamp)
> - SVN_ERR (svn_fs_change_rev_prop (pb->fs, new_rev,
> + SVN_ERR (svn_fs_change_rev_prop (pb->fs, *new_rev,
> SVN_PROP_REVISION_DATE,
> rb->datestamp,
> rb->pool));
>
> - if (new_rev == rb->rev)
> + if (*new_rev == rb->rev)
> {
> SVN_ERR (svn_stream_printf (pb->outstream, rb->pool,
> _("\n------- Committed revision %ld"
> - " >>>\n\n"), new_rev));
> + " >>>\n\n"), *new_rev));
> }
> else
> {
> SVN_ERR (svn_stream_printf (pb->outstream, rb->pool,
> _("\n------- Committed new rev %ld"
> " (loaded from original rev %ld"
> - ") >>>\n\n"), new_rev, rb->rev));
> + ") >>>\n\n"), *new_rev, rb->rev));
> }
>
> return SVN_NO_ERROR;
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12327 - trunk/subversion/libsvn_repos
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, December 16, 2004 2:44 AM +0000 Max Bowsher <ma...@ukf.net>
wrote:
> This patch was written at 2am during an IRC conversation on #cvs2svn with
> jerenkrantz, who discovered the bug when trying to load a newly converted
> project into the ASF main repository.
Correct. With this fix, we were able to load the dump file into the live
repository without any race conditions.
> Whilst I feel confident in the change, given that it went through the entire
> cycle of debug->commit in less than an hour, at 2am in the morning, I'd
> quite like a review on this one.
Well, Fitz, Sussman, and I were also in #cvs2svn at the same time and we all
agreed that this was the right fix. I also reviewed the patch before I merged
it into our install on apache.org. So, I'm confident on this patch (or as
much as I can be).
I think this needs to get into 1.1.2 as well as this is a gnarly race
condition that will hose the load: so I've proposed it for backport in STATUS.
-- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org