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

Re: [PATCH] deadlock fix?

From: <kfogel_at_collab.net>
Date: 2003-06-05 20:21:00 CEST

Joe Orton <jorton@redhat.com> writes:
> the problem appears to be simply that the DB_LOCK_DEADLOCK error
> returned by svn_fs__bdb_get_node_revision gets wrapped on the error
> stack by svn_fs__bdb_new_successor_id, so svn_fs__retry_txn doesn't spot
> it and retry appropriately. Or am I being stupidly naive and it's
> really a lot more complicated than this?

Wow, thanks Joe!

No, you're not being stupidly naive :-). This could be the problem
(not that I'm sure or anything, just confirming that it's well within
the realm of possibility!).

If this is the cause, then the question is, what's the best fix? I
think maybe instead of compensating for a misleading error stack, we
should change svn_fs__bdb_new_successor_id() to not mislead in the
first place. So this bit of code in that function

  /* Now, make sure this NEW_ID doesn't already exist in FS. */
  err = svn_fs__bdb_get_node_revision (NULL, fs, new_id, trail);
  if ((! err) || (err->apr_err != SVN_ERR_FS_ID_NOT_FOUND))
    {
      svn_string_t *id_str = svn_fs_unparse_id (id, trail->pool);
      svn_string_t *new_id_str = svn_fs_unparse_id (new_id, trail->pool);
      return svn_error_createf
        (SVN_ERR_FS_ALREADY_EXISTS, err,
         "successor id `%s' (for `%s') already exists in filesystem '%s'",
         new_id_str->data, id_str->data, fs->path);
    }

would become

  /* Now, make sure this NEW_ID doesn't already exist in FS. */
  err = svn_fs__bdb_get_node_revision (NULL, fs, new_id, trail);
  if ((! err) || (err->apr_err != SVN_ERR_FS_ID_NOT_FOUND))
    {
      if (err->apr_err == SVN_ERR_FS_BERKELEY_DB_DEADLOCK)
        return err;

      svn_string_t *id_str = svn_fs_unparse_id (id, trail->pool);
      svn_string_t *new_id_str = svn_fs_unparse_id (new_id, trail->pool);
      return svn_error_createf
        (SVN_ERR_FS_ALREADY_EXISTS, err,
         "successor id `%s' (for `%s') already exists in filesystem '%s'",
         new_id_str->data, id_str->data, fs->path);
    }

... or something like that. (There are various ways the conditionals
could be rearranged to achieve the same thing; the one above just
happens to be the one easiest to type into an email :-) ).

Do you have time to try with that fix instead, and commit if it works?
(I can't remember if there's an issue for this, btw...)

> One way to fix this would be as below - my stress.pl's have got to
> revision 400 now with this applied, without it they would reliably die
> before getting to 200.

By the way, what happens around revision 400? You just end the test
on purpose, or something goes wrong?

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 5 21:05:56 2003

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.