You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2013/03/26 23:11:51 UTC
[PATCH] verify at each commit
[[[
Run the per-revision verify code on a transaction just before it becomes
a revision. The intent is to catch corruption bugs as early as possible.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_commit_txn): As above.
]]]
[[[
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 1461335)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -761,6 +761,11 @@ svn_fs_commit_txn(const char **conflict_p, svn_rev
fs_path = svn_fs_path(fs, pool);
#endif
+#ifdef SVN_DEBUG
+ /* Verify. */
+ SVN_ERR(svn_fs_verify_rev(fs, txn_root, pool));
+#endif
+
err = txn->vtable->commit(conflict_p, new_rev, txn, pool);
#ifdef SVN_DEBUG
]]]
Maybe this should be optional behaviour in release mode, too?
Re: [PATCH] verify at each commit
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Wed, Mar 27, 2013 at 23:31:29 +0200:
> Branko Čibej wrote on Wed, Mar 27, 2013 at 12:57:13 +0100:
> > On 27.03.2013 07:54, Daniel Shahaf wrote:
> > > Branko Čibej wrote on Wed, Mar 27, 2013 at 05:14:51 +0100:
> > >> On 26.03.2013 23:11, Daniel Shahaf wrote:
> > >>> [[[
> > >>> Run the per-revision verify code on a transaction just before it becomes
> > >>> a revision. The intent is to catch corruption bugs as early as possible.
> > >>>
> > >>> * subversion/libsvn_fs/fs-loader.c
> > >>> (svn_fs_commit_txn): As above.
> > >>> ]]]
> > >>>
> > >>> Maybe this should be optional behaviour in release mode, too?
>
> And here's the new one. (I haven't colored the section name bikeshed
> yet, nor moved the new macros to svn_fs.h.)
>
> I wonder whether doing a verify of a revision root as the very last
> thing before incrementing db/current -- specifically, in commit_body()
> immediately before the sole call to write_final_current() --- would be
> better. Thoughts?
I have a problem implementing that one: I'd like to open a new svn_fs_t,
but the public API functions that do this are defined in a shared
library I cannot call into (libsvn_fs) and the vtable function
(fs.c:fs_open) should be passed the *same* COMMON_POOL algorithm that
libsvn_fs uses (since it uses apr_pool_userdata_get() on that pool to
access fs_fs_shared_data_t, which hosts the intra-process
per-filesystem mutexes).
How can fs_fs.c code open an svn_fs_t handle to another filesystem? (or
an independent handle to the current filesystem)
Daniel
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1461737)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -8262,6 +8262,31 @@ write_current(svn_fs_t *fs, svn_revnum_t rev, cons
return move_into_place(tmp_name, name, name, pool);
}
+/* */
+static svn_error_t *
+verify_as_revision_before_current_plus_plus(svn_fs_t *fs,
+ svn_revnum_t new_rev,
+ apr_pool_t *pool)
+{
+ /* fs++ == ft */
+ svn_fs_t *ft;
+ svn_fs_root_t *root;
+
+ SVN_ERR(svn_fs_open(&ft, fs->path, NULL /* ### TODO fs_config */, pool));
+
+ /* Time travel! */
+ {
+ fs_fs_data_t *ft_ffd = ft->fsap_data;
+ ft_ffd->youngest_rev_cache = new_rev;
+ }
+
+ SVN_ERR(svn_fs_fs__revision_root(&root, ft, new_rev, pool));
+#if 0
+ SVN_ERR(svn_fs_verify_root(root, pool));
+#endif
+ return SVN_NO_ERROR;
+}
+
/* Update the 'current' file to hold the correct next node and copy_ids
from transaction TXN_ID in filesystem FS. The current revision is
set to REV. Perform temporary allocations in POOL. */
@@ -8532,6 +8557,7 @@ commit_body(void *baton, apr_pool_t *pool)
old_rev_filename, pool));
/* Update the 'current' file. */
+ SVN_ERR(verify_as_revision_before_current_plus_plus(cb->fs, new_rev, pool));
SVN_ERR(write_final_current(cb->fs, cb->txn->id, new_rev, start_node_id,
start_copy_id, pool));
Re: [PATCH] verify at each commit
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Wed, Mar 27, 2013 at 12:57:13 +0100:
> On 27.03.2013 07:54, Daniel Shahaf wrote:
> > Branko Čibej wrote on Wed, Mar 27, 2013 at 05:14:51 +0100:
> >> On 26.03.2013 23:11, Daniel Shahaf wrote:
> >>> [[[
> >>> Run the per-revision verify code on a transaction just before it becomes
> >>> a revision. The intent is to catch corruption bugs as early as possible.
> >>>
> >>> * subversion/libsvn_fs/fs-loader.c
> >>> (svn_fs_commit_txn): As above.
> >>> ]]]
> >>>
> >>> Maybe this should be optional behaviour in release mode, too?
And here's the new one. (I haven't colored the section name bikeshed
yet, nor moved the new macros to svn_fs.h.)
I wonder whether doing a verify of a revision root as the very last
thing before incrementing db/current -- specifically, in commit_body()
immediately before the sole call to write_final_current() --- would be
better. Thoughts?
[[[
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 1461743)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -1,3 +1,6 @@
+#define SVN_FS_CONFIG_SECTION_FOO "foo"
+#define SVN_FS_CONFIG_OPTION_VERIFY_AT_COMMIT "verify-at-commit"
+
/*
* fs_loader.c: Front-end to the various FS back ends
*
@@ -32,6 +35,7 @@
#include "svn_hash.h"
#include "svn_ctype.h"
+#include "svn_config.h"
#include "svn_types.h"
#include "svn_dso.h"
#include "svn_version.h"
@@ -57,6 +61,7 @@
#endif
#define FS_TYPE_FILENAME "fs-type"
+#define CONFIG_FILENAME "fs.conf"
/* A pool common to all FS objects. See the documentation on the
open/create functions in fs-loader.h and for svn_fs_initialize(). */
@@ -338,6 +343,26 @@ write_fs_type(const char *path, const char *fs_typ
return svn_error_trace(svn_io_file_close(file, pool));
}
+static svn_error_t *
+write_config(const char *path, apr_pool_t *pool)
+{
+ static const char * const fs_conf_contents =
+#define NL APR_EOL_STR
+"### This file controls backend-independent filesystem configuration." NL
+"" NL
+"[" SVN_FS_CONFIG_SECTION_FOO "]" NL
+"### When set, Subversion will run the equivalent of 'svnadmin verify -r'" NL
+"### on each transaction (commit-in-progress) just before it becomes" NL
+"### a revision. This may slow down commits, since the cost of" NL
+"### verification is proportional to the size of the commit (e.g., number" NL
+"### of files 'svn log -q -v -r N' shows)." NL
+"# " SVN_FS_CONFIG_OPTION_VERIFY_AT_COMMIT " = false" NL
+;
+#undef NL
+ SVN_ERR(svn_io_file_create(svn_dirent_join(path, CONFIG_FILENAME, pool),
+ fs_conf_contents, pool));
+ return SVN_NO_ERROR;
+}
/* --- Functions for operating on filesystems by pathname --- */
@@ -414,6 +439,11 @@ fs_new(apr_hash_t *fs_config, apr_pool_t *pool)
fs->vtable = NULL;
fs->fsap_data = NULL;
fs->uuid = NULL;
+#ifdef SVN_DEBUG
+ fs->verify_at_commit = TRUE;
+#else
+ fs->verify_at_commit = FALSE;
+#endif
return fs;
}
@@ -445,6 +475,7 @@ svn_fs_create(svn_fs_t **fs_p, const char *path, a
/* Create the FS directory and write out the fsap-name file. */
SVN_ERR(svn_io_dir_make_sgid(path, APR_OS_DEFAULT, pool));
SVN_ERR(write_fs_type(path, fs_type, pool));
+ SVN_ERR(write_config(path, pool));
/* Perform the actual creation. */
*fs_p = fs_new(fs_config, pool);
@@ -459,9 +490,20 @@ svn_fs_open(svn_fs_t **fs_p, const char *path, apr
apr_pool_t *pool)
{
fs_library_vtable_t *vtable;
+ svn_config_t *config;
SVN_ERR(fs_library_vtable(&vtable, path, pool));
*fs_p = fs_new(fs_config, pool);
+
+ SVN_ERR(svn_config_read2(&config,
+ svn_dirent_join(path, CONFIG_FILENAME, pool),
+ FALSE /* must_exist */, TRUE /* case-sensitive */,
+ pool));
+ SVN_ERR(svn_config_get_bool(config, &(*fs_p)->verify_at_commit,
+ SVN_FS_CONFIG_SECTION_FOO,
+ SVN_FS_CONFIG_OPTION_VERIFY_AT_COMMIT,
+ (*fs_p)->verify_at_commit));
+
SVN_MUTEX__WITH_LOCK(common_pool_lock,
vtable->open_fs(*fs_p, path, pool, common_pool));
return SVN_NO_ERROR;
@@ -750,22 +792,18 @@ svn_fs_commit_txn(const char **conflict_p, svn_rev
svn_fs_txn_t *txn, apr_pool_t *pool)
{
svn_error_t *err;
-#if defined(PACK_AFTER_EVERY_COMMIT) || defined(SVN_DEBUG)
- svn_fs_root_t *txn_root;
-#endif
+ svn_fs_t *fs = txn->fs;
*new_rev = SVN_INVALID_REVNUM;
-#if defined(PACK_AFTER_EVERY_COMMIT) || defined(SVN_DEBUG)
- SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
-#endif
+ if (fs->verify_at_commit)
+ {
+ /* ### TODO: should this run just before incrementing 'current'? */
+ svn_fs_root_t *txn_root;
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_verify_root(txn_root, pool));
+ }
-#ifdef SVN_DEBUG
- /* ### TODO: add db/fs.conf with a knob to enable this in release builds */
- /* ### TODO: should this run just before incrementing 'current'? */
- SVN_ERR(svn_fs_verify_root(txn_root, pool));
-#endif
-
err = txn->vtable->commit(conflict_p, new_rev, txn, pool);
#ifdef SVN_DEBUG
@@ -784,7 +822,6 @@ svn_fs_commit_txn(const char **conflict_p, svn_rev
#ifdef PACK_AFTER_EVERY_COMMIT
{
- svn_fs_t *fs = svn_fs_root_fs(txn_root);
const char *fs_path = svn_fs_path(fs, pool);
err = svn_fs_pack(fs_path, NULL, NULL, NULL, NULL, pool);
if (err && err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
Index: subversion/libsvn_fs/fs-loader.h
===================================================================
--- subversion/libsvn_fs/fs-loader.h (revision 1461737)
+++ subversion/libsvn_fs/fs-loader.h (working copy)
@@ -407,6 +407,9 @@ struct svn_fs_t
/* UUID, stored by open(), create(), and set_uuid(). */
const char *uuid;
+
+ /* Parsed contents of fs.conf */
+ svn_boolean_t verify_at_commit;
};
]]]
Re: [PATCH] verify at each commit
Posted by Branko Čibej <br...@wandisco.com>.
On 27.03.2013 07:54, Daniel Shahaf wrote:
> Branko Čibej wrote on Wed, Mar 27, 2013 at 05:14:51 +0100:
>> On 26.03.2013 23:11, Daniel Shahaf wrote:
>>> [[[
>>> Run the per-revision verify code on a transaction just before it becomes
>>> a revision. The intent is to catch corruption bugs as early as possible.
>>>
>>> * subversion/libsvn_fs/fs-loader.c
>>> (svn_fs_commit_txn): As above.
>>> ]]]
>>>
>>> [[[
>>> Index: subversion/libsvn_fs/fs-loader.c
>>> ===================================================================
>>> --- subversion/libsvn_fs/fs-loader.c (revision 1461335)
>>> +++ subversion/libsvn_fs/fs-loader.c (working copy)
>>> @@ -761,6 +761,11 @@ svn_fs_commit_txn(const char **conflict_p, svn_rev
>>> fs_path = svn_fs_path(fs, pool);
>>> #endif
>>>
>>> +#ifdef SVN_DEBUG
>>> + /* Verify. */
>>> + SVN_ERR(svn_fs_verify_rev(fs, txn_root, pool));
>>> +#endif
>>> +
>>> err = txn->vtable->commit(conflict_p, new_rev, txn, pool);
>>>
>>> #ifdef SVN_DEBUG
>>> ]]]
>>>
>>> Maybe this should be optional behaviour in release mode, too?
>> I was thinking the same; as it is, it doesn't really help, since most of
>> the time only Subversion developers will be running our test suite with
>> SVN_DEBUG enabled.
> Right. The patch as is is useful primarily for developers. A
> user-oriented feature would be the same minus the #ifdef plus a knob
> (off by default), possibly combined with a recommendation for 'verify -r'
> in the post-commit hook.
>
> What config file do I put the knob in? I think my options are
> db/fs-type, db/fs.conf, db/fsfs.conf. (with the middle one being the
> cleanest design, but also the most code to implement and introduces a
> new uesr-facing config file)
A new db/config kind of feels like the right place.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: [PATCH] verify at each commit
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Wed, Mar 27, 2013 at 05:14:51 +0100:
> On 26.03.2013 23:11, Daniel Shahaf wrote:
> > [[[
> > Run the per-revision verify code on a transaction just before it becomes
> > a revision. The intent is to catch corruption bugs as early as possible.
> >
> > * subversion/libsvn_fs/fs-loader.c
> > (svn_fs_commit_txn): As above.
> > ]]]
> >
> > [[[
> > Index: subversion/libsvn_fs/fs-loader.c
> > ===================================================================
> > --- subversion/libsvn_fs/fs-loader.c (revision 1461335)
> > +++ subversion/libsvn_fs/fs-loader.c (working copy)
> > @@ -761,6 +761,11 @@ svn_fs_commit_txn(const char **conflict_p, svn_rev
> > fs_path = svn_fs_path(fs, pool);
> > #endif
> >
> > +#ifdef SVN_DEBUG
> > + /* Verify. */
> > + SVN_ERR(svn_fs_verify_rev(fs, txn_root, pool));
> > +#endif
> > +
> > err = txn->vtable->commit(conflict_p, new_rev, txn, pool);
> >
> > #ifdef SVN_DEBUG
> > ]]]
> >
> > Maybe this should be optional behaviour in release mode, too?
>
> I was thinking the same; as it is, it doesn't really help, since most of
> the time only Subversion developers will be running our test suite with
> SVN_DEBUG enabled.
Right. The patch as is is useful primarily for developers. A
user-oriented feature would be the same minus the #ifdef plus a knob
(off by default), possibly combined with a recommendation for 'verify -r'
in the post-commit hook.
What config file do I put the knob in? I think my options are
db/fs-type, db/fs.conf, db/fsfs.conf. (with the middle one being the
cleanest design, but also the most code to implement and introduces a
new uesr-facing config file)
Re: [PATCH] verify at each commit
Posted by Branko Čibej <br...@wandisco.com>.
On 26.03.2013 23:11, Daniel Shahaf wrote:
> [[[
> Run the per-revision verify code on a transaction just before it becomes
> a revision. The intent is to catch corruption bugs as early as possible.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_commit_txn): As above.
> ]]]
>
> [[[
> Index: subversion/libsvn_fs/fs-loader.c
> ===================================================================
> --- subversion/libsvn_fs/fs-loader.c (revision 1461335)
> +++ subversion/libsvn_fs/fs-loader.c (working copy)
> @@ -761,6 +761,11 @@ svn_fs_commit_txn(const char **conflict_p, svn_rev
> fs_path = svn_fs_path(fs, pool);
> #endif
>
> +#ifdef SVN_DEBUG
> + /* Verify. */
> + SVN_ERR(svn_fs_verify_rev(fs, txn_root, pool));
> +#endif
> +
> err = txn->vtable->commit(conflict_p, new_rev, txn, pool);
>
> #ifdef SVN_DEBUG
> ]]]
>
> Maybe this should be optional behaviour in release mode, too?
I was thinking the same; as it is, it doesn't really help, since most of
the time only Subversion developers will be running our test suite with
SVN_DEBUG enabled.
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com