Daniel Shahaf wrote:
should our policy be to provide administrative choice?
A knob sounds good to me [...]
And Bert said "+1" on IRC too.
The attached patch is a basic implementation. The fsfs.conf option is
spelled
[debug]
verify-before-commit = true
I put it under [debug] because that section name already exists and
holds a "pack after commit" option (not documented in the file template
comments) which seems conceptually similar.
What sort of testing do you think this needs? A first level could be to
test that the verification code runs when the option is explicitly
enabled and doesn't when disabled, and preferably test the default state
too. A second level could be to test that the verification actually
picks up some (injected) corruption and prevents the commit and exits
cleanly. This second level is not directly related to making the feature
optional, but it is related to providing the feature at all in a
release-mode build.
If we enable this outside maintainer mode, we should look into the
points raised upthread about ensuring that 'verify' is read-only.
We don't have to fix everything, but we should at least add roadsign
comments in the code to reduce the chance that future changes to verify
will interact badly with the unusual callsite in
verify_as_revision_before_current_plus_plus().
Ack.
By the way, are you planning to enable
verify_as_revision_before_current_plus_plus()
in alpha3? I would suggest enabling it unconditionally before alpha3,
then reverting it [or adding the knob under discussion] at some point
before branching 1.10.x.
Sounds good. To keep track of the need to change the default back again,
would a bold coloured box in the release notes would be a suitable place
for such a note?
[We should probably give that function a shorter name... ;)]
Could do. verify_before_commit?
- Julian
Make verify-before-commit a configurable option in FSFS.
The option causes FSFS to verify each new revision immediately before
finalizing the commit (bumping the 'current' revision number).
It is useful to be able to enable this after repository corruption has been
observed that is suspected to be bug-induced.
The option is disabled by default in release-mode builds, and enabled by
default in debug-mode builds. Previously, the verification was permanently
disabled in release-mode builds and enabled in debug-mode builds.
* subversion/libsvn_fs_fs/fs_fs.c
(read_config): Read the option.
(write_config): Write a template for the option.
* subversion/libsvn_fs_fs/fs.h
(CONFIG_OPTION_VERIFY_BEFORE_COMMIT): New.
(fs_fs_data_t): Store the option value.
* subversion/libsvn_fs_fs/transaction.c
(verify_as_revision_before_current_plus_plus): Compile unconditionally,
not just in debug builds.
(commit_body): Call the function only if the option is enabled.
--This line, and those below, will be ignored--
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1787000)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -813,12 +813,23 @@ read_config(fs_fs_data_t *ffd,
FALSE));
}
else
{
ffd->pack_after_commit = FALSE;
}
+#ifdef SVN_DEBUG
+ SVN_ERR(svn_config_get_bool(config, &ffd->verify_before_commit,
+ CONFIG_SECTION_DEBUG,
+ CONFIG_OPTION_VERIFY_BEFORE_COMMIT,
+ TRUE));
+#else
+ SVN_ERR(svn_config_get_bool(config, &ffd->verify_before_commit,
+ CONFIG_SECTION_DEBUG,
+ CONFIG_OPTION_VERIFY_BEFORE_COMMIT,
+ FALSE));
+#endif
/* memcached configuration */
SVN_ERR(svn_cache__make_memcache_from_config(&ffd->memcache, config,
result_pool, scratch_pool));
SVN_ERR(svn_config_get_bool(config, &ffd->fail_stop,
@@ -1017,12 +1028,19 @@ write_config(svn_fs_t *fs,
"### file at the expense of a slightly increased latency in sections with" NL
"### smaller changes." NL
"### For source code repositories, this should be about 16x the block-size." NL
"### Must be a power of 2." NL
"### p2l-page-size is given in kBytes and with a default of 1024 kBytes." NL
"# " CONFIG_OPTION_P2L_PAGE_SIZE " = 1024" NL
+"" NL
+"[" CONFIG_SECTION_DEBUG "]" NL
+"###" NL
+"### Whether to verify each new revision immediately before finalizing" NL
+"### the commit. The default is false in release-mode builds, and true" NL
+"### in debug-mode builds." NL
+"# " CONFIG_OPTION_VERIFY_BEFORE_COMMIT " = false" NL
;
#undef NL
return svn_io_file_create(svn_dirent_join(fs->path, PATH_CONFIG, pool),
fsfs_conf_contents, pool);
}
Index: subversion/libsvn_fs_fs/fs.h
===================================================================
--- subversion/libsvn_fs_fs/fs.h (revision 1787000)
+++ subversion/libsvn_fs_fs/fs.h (working copy)
@@ -114,12 +114,13 @@ extern "C" {
#define CONFIG_SECTION_IO "io"
#define CONFIG_OPTION_BLOCK_SIZE "block-size"
#define CONFIG_OPTION_L2P_PAGE_SIZE "l2p-page-size"
#define CONFIG_OPTION_P2L_PAGE_SIZE "p2l-page-size"
#define CONFIG_SECTION_DEBUG "debug"
#define CONFIG_OPTION_PACK_AFTER_COMMIT "pack-after-commit"
+#define CONFIG_OPTION_VERIFY_BEFORE_COMMIT "verify-before-commit"
/* The format number of this filesystem.
This is independent of the repository format number, and
independent of any other FS back ends.
Note: If you bump this, please update the switch statement in
@@ -472,12 +473,15 @@ typedef struct fs_fs_data_t
/* Compression level to use with txdelta storage format in new revs. */
int delta_compression_level;
/* Pack after every commit. */
svn_boolean_t pack_after_commit;
+ /* Verify each new revision before commit. */
+ svn_boolean_t verify_before_commit;
+
/* Per-instance filesystem ID, which provides an additional level of
uniqueness for filesystems that share the same UUID, but should
still be distinguishable (e.g. backups produced by svn_fs_hotcopy()
or dump / load cycles). */
const char *instance_id;
Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c (revision 1787000)
+++ subversion/libsvn_fs_fs/transaction.c (working copy)
@@ -3428,13 +3428,12 @@ write_final_changed_path_info(apr_off_t
is bumped. This implies that we are holding the write lock. */
static svn_error_t *
verify_as_revision_before_current_plus_plus(svn_fs_t *fs,
svn_revnum_t new_rev,
apr_pool_t *pool)
{
-#ifdef SVN_DEBUG
fs_fs_data_t *ffd = fs->fsap_data;
svn_fs_t *ft; /* fs++ == ft */
svn_fs_root_t *root;
fs_fs_data_t *ft_ffd;
apr_hash_t *fs_config;
@@ -3458,13 +3457,12 @@ verify_as_revision_before_current_plus_p
ft_ffd->youngest_rev_cache = new_rev;
SVN_ERR(svn_fs_fs__revision_root(&root, ft, new_rev, pool));
SVN_ERR_ASSERT(root->is_txn_root == FALSE && root->rev == new_rev);
SVN_ERR_ASSERT(ft_ffd->youngest_rev_cache == new_rev);
SVN_ERR(svn_fs_fs__verify_root(root, pool));
-#endif /* SVN_DEBUG */
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
@@ -3878,14 +3876,19 @@ commit_body(void *baton, apr_pool_t *poo
/* Write final revprops file. */
SVN_ERR_ASSERT(! svn_fs_fs__is_packed_revprop(cb->fs, new_rev));
revprop_filename = svn_fs_fs__path_revprops(cb->fs, new_rev, pool);
SVN_ERR(write_final_revprop(revprop_filename, old_rev_filename,
cb->txn, ffd->flush_to_disk, pool));
+ if (ffd->verify_before_commit)
+ {
+ SVN_ERR(verify_as_revision_before_current_plus_plus(cb->fs, new_rev,
+ 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, txn_id, new_rev, start_node_id,
start_copy_id, pool));
/* At this point the new revision is committed and globally visible
so let the caller know it succeeded by giving it the new revision
number, which fulfills svn_fs_commit_txn() contract. Any errors
