You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2012/02/28 07:23:29 UTC
svn commit: r1294479 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Author: danielsh
Date: Tue Feb 28 06:23:28 2012
New Revision: 1294479
URL: http://svn.apache.org/viewvc?rev=1294479&view=rev
Log:
Check for issue #4129 (bogus predecessor fields in the root node-revision) in
'svnadmin verify'.
Notes:
- This check precedes all the other non-backend-specific check. A subsequent
revision will address that.
- Interaction with FSFS caches: see comment within.
* subversion/libsvn_fs_fs/fs_fs.c
(validate_root_noderev): Point to svn_fs_fs__verify().
(verify_walker): Update docstring's reference.
(svn_fs_fs__verify):
Look in every revision for an instance of this bug.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1294479&r1=1294478&r2=1294479&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Feb 28 06:23:28 2012
@@ -5865,6 +5865,8 @@ write_hash_delta_rep(representation_t *r
/* Sanity check ROOT_NODEREV, a candidate for being the root node-revision
of (not yet committed) revision REV in FS. Use POOL for temporary
allocations.
+
+ If you change this function, consider updating svn_fs_fs__verify() too.
*/
static svn_error_t *
validate_root_noderev(svn_fs_t *fs,
@@ -7881,7 +7883,7 @@ svn_fs_fs__pack(svn_fs_t *fs,
/** Verifying. **/
-/* Body of svn_fs_fs__verify().
+/* Used by svn_fs_fs__verify().
Implements svn_fs_fs__walk_rep_reference().walker. */
static svn_error_t *
verify_walker(representation_t *rep,
@@ -7906,6 +7908,8 @@ svn_fs_fs__verify(svn_fs_t *fs,
{
fs_fs_data_t *ffd = fs->fsap_data;
svn_boolean_t exists;
+ svn_revnum_t youngest = ffd->youngest_rev_cache; /* cache is current */
+ apr_pool_t *iterpool = svn_pool_create(pool);
if (ffd->format < SVN_FS_FS__MIN_REP_SHARING_FORMAT)
return SVN_NO_ERROR;
@@ -7919,6 +7923,48 @@ svn_fs_fs__verify(svn_fs_t *fs,
cancel_func, cancel_baton,
pool));
+ /* Issue #4129: bogus pred-counts on the root node-rev. */
+ {
+ svn_revnum_t i;
+ int predecessor_predecessor_count = -1;
+
+ for (i = 0; i <= youngest; i++)
+ {
+ /* ### Caching.
+
+ svn_fs_fs__rev_get_root() consults caches, which in verify we
+ don't want. But we can't easily bypass that, as
+ svn_fs_revision_root()+svn_fs_node_id()'s implementation uses
+ svn_fs_fs__rev_get_root() too.
+
+ ### A future revision will make fs_verify() disable caches when it
+ ### opens ffd.
+ */
+ svn_fs_id_t *root_id;
+ node_revision_t *root_noderev;
+
+ if ((i % 100) == 0) /* uneducated guess */
+ svn_pool_clear(iterpool);
+
+ /* Fetch ROOT_NODEREV. */
+ SVN_ERR(svn_fs_fs__rev_get_root(&root_id, fs, i, iterpool));
+ SVN_ERR(svn_fs_fs__get_node_revision(&root_noderev, fs, root_id,
+ iterpool));
+
+ /* Check correctness. (Compare validate_root_noderev().) */
+ if (1+predecessor_predecessor_count != root_noderev->predecessor_count)
+ return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
+ _("predecessor count for "
+ "the root node-revision is wrong: "
+ "r%ld has %d, but r%ld has %d"),
+ i, root_noderev->predecessor_count,
+ i-1, predecessor_predecessor_count);
+
+ predecessor_predecessor_count = root_noderev->predecessor_count;
+ }
+ }
+
+ svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
}
Re: svn commit: r1294479 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <da...@elego.de>.
danielsh@apache.org wrote on Tue, Feb 28, 2012 at 06:23:29 -0000:
> Author: danielsh
> Date: Tue Feb 28 06:23:28 2012
> New Revision: 1294479
>
> URL: http://svn.apache.org/viewvc?rev=1294479&view=rev
> Log:
> Check for issue #4129 (bogus predecessor fields in the root node-revision) in
> 'svnadmin verify'.
>
> Notes:
>
> - This check precedes all the other non-backend-specific check. A subsequent
> revision will address that.
>
> - Interaction with FSFS caches: see comment within.
>
...
> @@ -7919,6 +7923,48 @@ svn_fs_fs__verify(svn_fs_t *fs,
> cancel_func, cancel_baton,
> pool));
>
> + /* Issue #4129: bogus pred-counts on the root node-rev. */
> + {
> + svn_revnum_t i;
> + int predecessor_predecessor_count = -1;
> +
> + for (i = 0; i <= youngest; i++)
> + {
> + /* ### Caching.
> +
> + svn_fs_fs__rev_get_root() consults caches, which in verify we
> + don't want. But we can't easily bypass that, as
> + svn_fs_revision_root()+svn_fs_node_id()'s implementation uses
> + svn_fs_fs__rev_get_root() too.
> +
> + ### A future revision will make fs_verify() disable caches when it
> + ### opens ffd.
Having had another look at this, I'm not sure to what extent that is
necessary.
---
All caches live in fs_fs_data_t. The cache prefix embeds the UUID and
the OS path to the filesystem root (/srv/svn/repos/calc/db, or so) (see
svn_fs_fs__initialize_caches()).
The cache types are three: 'inprocess', 'membuffer', or 'memcache'. The
first two are per-process, but the third need not be.
The net result of all this is that verify() cannot be sure that, if it
got a cached data, it is a cache of information computed from the
repository it is interrogating: in memcache situations the information
may have been installed in the cache by another memcache client (running
on a different box), and even in in-process situations the following
scenario would invalidate the cache:
% svnadmin dump repos > backup.dump
% vi repos
% svnadmin is-repos-corrupt repos
yes
% svnadmin create repos2
% svnadmin load -q repos2 < backup.dump
% mv repos rm && mv repos2 repos
(And no, I don't think it's contrived. I imagine that's a fairly common
thing to do --- to restore a backup repository to its original name.)
---
So, I'm debating between:
- Disable all caches in svn_fs_verify().
- Disable memcached caches in svn_fs_verify().
- None of the above, but document that users of svn_fs_verify() should
disable caches. (And do that in libsvn_repos and/or 'svnadmin verify',
too.)
Thoughts?
> + */
> + svn_fs_id_t *root_id;
> + node_revision_t *root_noderev;
> +
> + if ((i % 100) == 0) /* uneducated guess */
> + svn_pool_clear(iterpool);
> +
> + /* Fetch ROOT_NODEREV. */
> + SVN_ERR(svn_fs_fs__rev_get_root(&root_id, fs, i, iterpool));
> + SVN_ERR(svn_fs_fs__get_node_revision(&root_noderev, fs, root_id,
> + iterpool));
> +
> + /* Check correctness. (Compare validate_root_noderev().) */
> + if (1+predecessor_predecessor_count != root_noderev->predecessor_count)
> + return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> + _("predecessor count for "
> + "the root node-revision is wrong: "
> + "r%ld has %d, but r%ld has %d"),
> + i, root_noderev->predecessor_count,
> + i-1, predecessor_predecessor_count);
> +
> + predecessor_predecessor_count = root_noderev->predecessor_count;
> + }
> + }
> +
> + svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
>
>
>
Re: svn commit: r1294479 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <da...@elego.de>.
danielsh@apache.org wrote on Tue, Feb 28, 2012 at 06:23:29 -0000:
> Author: danielsh
> Date: Tue Feb 28 06:23:28 2012
> New Revision: 1294479
>
> URL: http://svn.apache.org/viewvc?rev=1294479&view=rev
> Log:
> Check for issue #4129 (bogus predecessor fields in the root node-revision) in
> 'svnadmin verify'.
>
> Notes:
>
> - This check precedes all the other non-backend-specific check. A subsequent
> revision will address that.
>
> - Interaction with FSFS caches: see comment within.
>
...
> @@ -7919,6 +7923,48 @@ svn_fs_fs__verify(svn_fs_t *fs,
> cancel_func, cancel_baton,
> pool));
>
> + /* Issue #4129: bogus pred-counts on the root node-rev. */
> + {
> + svn_revnum_t i;
> + int predecessor_predecessor_count = -1;
> +
> + for (i = 0; i <= youngest; i++)
> + {
> + /* ### Caching.
> +
> + svn_fs_fs__rev_get_root() consults caches, which in verify we
> + don't want. But we can't easily bypass that, as
> + svn_fs_revision_root()+svn_fs_node_id()'s implementation uses
> + svn_fs_fs__rev_get_root() too.
> +
> + ### A future revision will make fs_verify() disable caches when it
> + ### opens ffd.
Having had another look at this, I'm not sure to what extent that is
necessary.
---
All caches live in fs_fs_data_t. The cache prefix embeds the UUID and
the OS path to the filesystem root (/srv/svn/repos/calc/db, or so) (see
svn_fs_fs__initialize_caches()).
The cache types are three: 'inprocess', 'membuffer', or 'memcache'. The
first two are per-process, but the third need not be.
The net result of all this is that verify() cannot be sure that, if it
got a cached data, it is a cache of information computed from the
repository it is interrogating: in memcache situations the information
may have been installed in the cache by another memcache client (running
on a different box), and even in in-process situations the following
scenario would invalidate the cache:
% svnadmin dump repos > backup.dump
% vi repos
% svnadmin is-repos-corrupt repos
yes
% svnadmin create repos2
% svnadmin load -q repos2 < backup.dump
% mv repos rm && mv repos2 repos
(And no, I don't think it's contrived. I imagine that's a fairly common
thing to do --- to restore a backup repository to its original name.)
---
So, I'm debating between:
- Disable all caches in svn_fs_verify().
- Disable memcached caches in svn_fs_verify().
- None of the above, but document that users of svn_fs_verify() should
disable caches. (And do that in libsvn_repos and/or 'svnadmin verify',
too.)
Thoughts?
> + */
> + svn_fs_id_t *root_id;
> + node_revision_t *root_noderev;
> +
> + if ((i % 100) == 0) /* uneducated guess */
> + svn_pool_clear(iterpool);
> +
> + /* Fetch ROOT_NODEREV. */
> + SVN_ERR(svn_fs_fs__rev_get_root(&root_id, fs, i, iterpool));
> + SVN_ERR(svn_fs_fs__get_node_revision(&root_noderev, fs, root_id,
> + iterpool));
> +
> + /* Check correctness. (Compare validate_root_noderev().) */
> + if (1+predecessor_predecessor_count != root_noderev->predecessor_count)
> + return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> + _("predecessor count for "
> + "the root node-revision is wrong: "
> + "r%ld has %d, but r%ld has %d"),
> + i, root_noderev->predecessor_count,
> + i-1, predecessor_predecessor_count);
> +
> + predecessor_predecessor_count = root_noderev->predecessor_count;
> + }
> + }
> +
> + svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
>
>
>