On Tue, Aug 12, 2014 at 11:47 PM, Stefan Fuhrmann < stefan.fuhrm...@wandisco.com> wrote:
> Arrgs!! Random key combo made me sent the mail before completing it ... > > > On Tue, Aug 12, 2014 at 11:29 PM, Stefan Fuhrmann < > stefan.fuhrm...@wandisco.com> wrote: > >> >> On Sun, Aug 10, 2014 at 3:56 PM, Evgeny Kotkov < >> evgeny.kot...@visualsvn.com> wrote: >> > [...] > I have found myself in the same situation as you before. > A seemingly simple change causing lots of discussion. > It's surprising and somewhat disheartening, but in the > end the code has been better than it would have been > without the discussions. The key is that everyone needs > to stay constructive in their criticisms. > To be constructive, I implemented the feature in the db/uuid instead of db/format - taking your code and commentary where possible. That automatically covered the dump / load issue and shortened the patch quite a bit. Cache keying and structure description update are also addressed. I did not implement an extension to 'svnadmin setuuid' to only update the instance ID because I'm not sure I really want it. A minor thing I did is to call the new feature 'instance ID' instead of 'instance UUID' because there is no need to guarantee global uniqueness. Our current implementation just happens to generate UUIDs. Future code might use something else. But that is a only a minor point. -- Stefan^2.
[[[ Avoid shared data clashes between repositories duplicated using 'hotcopy', see http://svn.haxx.se/dev/archive-2014-04/0245.shtml This is not a "scalability issue" (as stated in the referenced thread), but rather a full-fledged problem. We have an ability to share data between different objects pointing to same filesystems. The sharing works within a single process boundary; currently we share locks (svn_mutex__t objects) and certain transaction data. Accessing shared data requires some sort of a key and currently we use a filesystem UUID for this purpose. However, this is *not* good enough for at least a couple of scenarios. Filesystem UUIDs aren't really unique for every filesystem an end user might have, because they get duplicated during hotcopy or naive copy (copy-paste). Whenever we have two filesystems with the same UUIDs open within a single process, the shared data starts clashing and things can get pretty ugly. For example, one can experience random errors with parallel commits to two repositories with the same UUID (hosted by Apache HTTP Server). Another example was recently mitigated by http://svn.apache.org/r1589653 — we did encounter a deadlock within nested 'svnadmin freeze' commands executed for two repositories with the same UUID. Errors that I witnessed include (but might not be limited to): - Cannot write to the prototype revision file of transaction '392-ax' because a previous representation is currently being written by this process (SVN_ERR_FS_CORRUPT) - Can't unlock unknown transaction '392-ax' (SVN_ERR_FS_CORRUPT) - Recursive locks are not supported (SVN_ERR_RECURSIVE_LOCK) # This used to be deadlock prior to http://svn.apache.org/r1591919 Fix the issue by introducing a concept of "instance IDs" on the FS layer. Basically, this gives us an ability to distinguish filesystem duplicates or near-duplicates produced via our API (svn_fs_hotcopy3(), to be exact). We can now have different filesystems with the same "original" UUID, but with different instance IDs. With this concept, it is rather easy to get rid of the shared data clashes described above. By making svn_fs_set_uuid automatically bump the instance ID, we implicitly solve the problem of svnadmin dump / load creating a similar problem as 'svnadmin hotcopy'. Finally, we make the instance ID partof our cache keys to prevent false aliasing for that class of shared data as well. * subversion/libsvn_fs_fs/fs.h (SVN_FS_FS__MIN_INSTANCE_UUID_FORMAT): New. (fs_fs_data_t.instance_uuid): New. * subversion/libsvn_fs_fs/fs_fs.h (svn_fs_fs__set_uuid): Add INSTANCE_ID parameter. * subversion/libsvn_fs_fs/fs.c (fs_serialized_init): Use a combination of two UUIDs as the shared data key (see the huge comment block about why we concatenate them). (fs_set_uuid): New adapter function. (fs_vtable): Invoke the new V-table compatible adapter. * subversion/libsvn_fs_fs/fs_fs.c (svn_fs_fs__open): Read the instance ID as well. (upgrade_body): If necessary, rewrite the UUID file to automagically add an instance ID. (svn_fs_fs__set_uuid): Write / auto-generate instance IDs when required by the FS format. Use pools more sanely. (svn_fs_fs__create): Update caller. Instance IDs are auto-created. * subversion/libsvn_fs_fs/hotcopy.c (hotcopy_create_empty_dest): Update caller, implicitly and unconditionally generating a new instance ID. * subversion/libsvn_fs_fs/caching.c (svn_fs_fs__initialize_caches, svn_fs_fs__initialize_txn_caches): Make the instance ID part of cache key. * subversion/libsvn_fs_fs/structure (Layout of the FS directory): Tweak wording for the db/uuid file. (Filesystem formats): Shortly list the format specifics of that file. * subversion/tests/cmdline/svnadmin_tests.py (check_hotcopy_fsfs_fsx): Allow different instance ids in uuid files. (freeze_freeze): Do not change UUID of hotcopy for new formats supporting instance UUIDs. For new formats, 'svnadmin freeze A (svnadmin freeze B)' should not deadlock or error out with SVN_ERR_RECURSIVE_LOCK even if 'A' and 'B' share the same UUID. ]]] Index: subversion/libsvn_fs_fs/caching.c =================================================================== --- subversion/libsvn_fs_fs/caching.c (revision 1617633) +++ subversion/libsvn_fs_fs/caching.c (working copy) @@ -361,6 +361,7 @@ svn_fs_fs__initialize_caches(svn_fs_t *f fs_fs_data_t *ffd = fs->fsap_data; const char *prefix = apr_pstrcat(pool, "fsfs:", fs->uuid, + ":", ffd->instance_id, "/", normalize_key_part(fs->path, pool), ":", SVN_VA_NULL); @@ -787,6 +788,7 @@ svn_fs_fs__initialize_txn_caches(svn_fs_ Therefore, throw in a uuid as well - just to be sure. */ const char *prefix = apr_pstrcat(pool, "fsfs:", fs->uuid, + ":", ffd->instance_id, "/", fs->path, ":", txn_id, ":", svn_uuid_generate(pool), ":", Index: subversion/libsvn_fs_fs/fs.c =================================================================== --- subversion/libsvn_fs_fs/fs.c (revision 1617633) +++ subversion/libsvn_fs_fs/fs.c (working copy) @@ -72,19 +72,31 @@ fs_serialized_init(svn_fs_t *fs, apr_poo each separate repository opened during the lifetime of the svn_fs_initialize pool. It's unlikely that anyone will notice the modest expenditure; the alternative is to allocate each structure - in a subpool, add a reference-count, and add a serialized deconstructor + in a subpool, add a reference-count, and add a serialized destructor to the FS vtable. That's more machinery than it's worth. - Using the uuid to obtain the lock creates a corner case if a - caller uses svn_fs_set_uuid on the repository in a process where - other threads might be using the same repository through another - FS object. The only real-world consumer of svn_fs_set_uuid is - "svnadmin load", so this is a low-priority problem, and we don't - know of a better way of associating such data with the - repository. */ + Picking an appropriate key for the shared data is tricky, because, + unfortunately, a filesystem UUID is not really unique -- it is shared + between hotcopied (1) or naively copied (2) filesystems. We tackle this + problem by using a combination of a filesystem UUID and an instance UUID + as the key. This allows us to avoid key clashing in (1) for FS formats + >= SVN_FS_FS__MIN_INSTANCE_UUID_FORMAT, which do support instance UUIDs. + For old formats the shared data (locks, shared transaction data, ...) + will still clash. + + Speaking of (2), there is not so much we can do about it, except maybe + provide a convenient way of fixing things. That's the only reason why + we combine two UUIDs instead of just using the instance UUID. Naively + copied filesystems have identical FS *and* instance UUIDs. With the key + being a combination of these, clashes can be fixed by setting a new FS + UUID (e.g. using svn_fs_set_uuid()), which is much simpler compared to + altering the instance UUID. */ + SVN_ERR_ASSERT(fs->uuid); - key = apr_pstrcat(pool, SVN_FSFS_SHARED_USERDATA_PREFIX, fs->uuid, - SVN_VA_NULL); + SVN_ERR_ASSERT(ffd->instance_id); + + key = apr_pstrcat(pool, SVN_FSFS_SHARED_USERDATA_PREFIX, + fs->uuid, ":", ffd->instance_id, SVN_VA_NULL); status = apr_pool_userdata_get(&val, key, common_pool); if (status) return svn_error_wrap_apr(status, _("Can't fetch FSFS shared data")); @@ -209,6 +221,18 @@ fs_info(const void **fsfs_info, return SVN_NO_ERROR; } +/* Wrapper around svn_fs_fs__set_uuid() adapting between function signatures. + */ +static svn_error_t * +fs_set_uuid(svn_fs_t *fs, + const char *uuid, + apr_pool_t *pool) +{ + /* Whenever we set a new UUID, imply that FS will also be a different + * instance (on formats that support this). */ + return svn_error_trace(svn_fs_fs__set_uuid(fs, uuid, NULL, pool)); +} + /* The vtable associated with a specific open filesystem. */ @@ -217,7 +241,7 @@ static fs_vtable_t fs_vtable = { svn_fs_fs__revision_prop, svn_fs_fs__get_revision_proplist, svn_fs_fs__change_rev_prop, - svn_fs_fs__set_uuid, + fs_set_uuid, svn_fs_fs__revision_root, svn_fs_fs__begin_txn, svn_fs_fs__open_txn, Index: subversion/libsvn_fs_fs/fs_fs.c =================================================================== --- subversion/libsvn_fs_fs/fs_fs.c (revision 1617633) +++ subversion/libsvn_fs_fs/fs_fs.c (working copy) @@ -1033,6 +1033,17 @@ svn_fs_fs__open(svn_fs_t *fs, const char SVN_ERR(svn_io_read_length_line(uuid_file, buf, &limit, pool)); fs->uuid = apr_pstrdup(fs->pool, buf); + if (format >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT) + { + limit = sizeof(buf); + SVN_ERR(svn_io_read_length_line(uuid_file, buf, &limit, pool)); + ffd->instance_id = apr_pstrdup(fs->pool, buf); + } + else + { + ffd->instance_id = fs->uuid; + } + SVN_ERR(svn_io_file_close(uuid_file, pool)); /* Read the min unpacked revision. */ @@ -1164,11 +1175,21 @@ upgrade_body(void *baton, apr_pool_t *po * max_files_per_dir; } - /* Bump the format file. */ + /* Update the format info in the FS struct. + * + * Upgrade steps further down will use the format from FS to create + * missing info. + */ ffd->format = SVN_FS_FS__FORMAT_NUMBER; ffd->max_files_per_dir = max_files_per_dir; ffd->min_log_addressing_rev = min_log_addressing_rev; + /* Come up with a new instance ID in case our filesystem + did not support it before. Keep the UUID. */ + if (format < SVN_FS_FS__MIN_INSTANCE_ID_FORMAT) + SVN_ERR(svn_fs_fs__set_uuid(fs, fs->uuid, NULL, pool)); + + /* Bump the format file. */ SVN_ERR(svn_fs_fs__write_format(fs, TRUE, pool)); if (upgrade_baton->notify_func) SVN_ERR(upgrade_baton->notify_func(upgrade_baton->notify_baton, @@ -1644,7 +1665,7 @@ svn_fs_fs__create(svn_fs_t *fs, ? "0\n" : "0 1 1\n"), pool)); SVN_ERR(svn_io_file_create_empty(svn_fs_fs__path_lock(fs, pool), pool)); - SVN_ERR(svn_fs_fs__set_uuid(fs, NULL, pool)); + SVN_ERR(svn_fs_fs__set_uuid(fs, NULL, NULL, pool)); /* Create the fsfs.conf file if supported. Older server versions would simply ignore the file but that might result in a different behavior @@ -1687,17 +1708,26 @@ svn_fs_fs__create(svn_fs_t *fs, svn_error_t * svn_fs_fs__set_uuid(svn_fs_t *fs, const char *uuid, + const char *instance_id, apr_pool_t *pool) { + fs_fs_data_t *ffd = fs->fsap_data; char *my_uuid; apr_size_t my_uuid_len; const char *uuid_path = path_uuid(fs, pool); if (! uuid) uuid = svn_uuid_generate(pool); + if (! instance_id && ffd->format >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT) + instance_id = svn_uuid_generate(pool); /* Make sure we have a copy in FS->POOL, and append a newline. */ - my_uuid = apr_pstrcat(fs->pool, uuid, "\n", SVN_VA_NULL); + my_uuid = ffd->format >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT + ? apr_pstrcat(pool, uuid, "\n", instance_id, "\n", + SVN_VA_NULL) + : apr_pstrcat(pool, uuid, "\n", + SVN_VA_NULL); + my_uuid_len = strlen(my_uuid); /* We use the permissions of the 'current' file, because the 'uuid' @@ -1706,9 +1736,11 @@ svn_fs_fs__set_uuid(svn_fs_t *fs, svn_fs_fs__path_current(fs, pool) /* perms */, pool)); - /* Remove the newline we added, and stash the UUID. */ - my_uuid[my_uuid_len - 1] = '\0'; - fs->uuid = my_uuid; + /* Store the new value in the FS struct. */ + fs->uuid = apr_pstrdup(fs->pool, uuid); + ffd->instance_id = ffd->format >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT + ? apr_pstrdup(fs->pool, instance_id) + : fs->uuid; return SVN_NO_ERROR; } Index: subversion/libsvn_fs_fs/fs_fs.h =================================================================== --- subversion/libsvn_fs_fs/fs_fs.h (revision 1617633) +++ subversion/libsvn_fs_fs/fs_fs.h (working copy) @@ -115,11 +115,13 @@ svn_error_t *svn_fs_fs__create(svn_fs_t const char *path, apr_pool_t *pool); -/* Set the uuid of repository FS to UUID, if UUID is not NULL; - otherwise, set the uuid of FS to a newly generated UUID. Perform - temporary allocations in POOL. */ +/* Set the uuid of repository FS to UUID and the instance ID to INSTANCE_ID. + If any of them is NULL, use a newly generated UUID in their stead. + Ignore INSTANCE_ID for FS formats that don't support them. + Perform temporary allocations in POOL. */ svn_error_t *svn_fs_fs__set_uuid(svn_fs_t *fs, const char *uuid, + const char *instance_id, apr_pool_t *pool); /* Return the path to the 'current' file in FS. Index: subversion/libsvn_fs_fs/fs.h =================================================================== --- subversion/libsvn_fs_fs/fs.h (revision 1617633) +++ subversion/libsvn_fs_fs/fs.h (working copy) @@ -179,6 +179,9 @@ extern "C" { /* Minimum format number that stores mergeinfo-mode flag in changed paths */ #define SVN_FS_FS__MIN_MERGEINFO_IN_CHANGES_FORMAT 7 +/* Minimum format number that supports per-instance filesystem UUIDs. */ +#define SVN_FS_FS__MIN_INSTANCE_ID_FORMAT 7 + /* The minimum format number that supports a configuration file (fsfs.conf) */ #define SVN_FS_FS__MIN_CONFIG_FILE 4 @@ -467,6 +470,12 @@ typedef struct fs_fs_data_t /* Pack after every commit. */ svn_boolean_t pack_after_commit; + /* Per-instance filesystem ID, which provides an additional level of + uniqueness for filesystems that share the same FS UUID, but should + still be distinguishable (e.g. backups produced by svn_fs_hotcopy() + or dump / load cycles). */ + const char *instance_id; + /* Pointer to svn_fs_open. */ svn_error_t *(*svn_fs_open_)(svn_fs_t **, const char *, apr_hash_t *, apr_pool_t *, apr_pool_t *); Index: subversion/libsvn_fs_fs/hotcopy.c =================================================================== --- subversion/libsvn_fs_fs/hotcopy.c (revision 1617633) +++ subversion/libsvn_fs_fs/hotcopy.c (working copy) @@ -1090,9 +1090,9 @@ hotcopy_create_empty_dest(svn_fs_t *src_ ? "0\n" : "0 1 1\n"), pool)); - /* Create lock file and UUID. */ + /* Create lock file and UUID plus new instance ID. */ SVN_ERR(svn_io_file_create_empty(svn_fs_fs__path_lock(dst_fs, pool), pool)); - SVN_ERR(svn_fs_fs__set_uuid(dst_fs, src_fs->uuid, pool)); + SVN_ERR(svn_fs_fs__set_uuid(dst_fs, src_fs->uuid, NULL, pool)); /* Create the min unpacked rev file. */ if (dst_ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT) Index: subversion/libsvn_fs_fs/structure =================================================================== --- subversion/libsvn_fs_fs/structure (revision 1617633) +++ subversion/libsvn_fs_fs/structure (working copy) @@ -62,7 +62,7 @@ repository) is: write-lock Empty file, locked to serialise writers pack-lock Empty file, locked to serialise 'svnadmin pack' (f. 7+) txn-current-lock Empty file, locked to serialise 'txn-current' - uuid File containing the UUID of the repository + uuid File containing the repository IDs format File containing the format number of this filesystem fsfs.conf Configuration file min-unpacked-rev File containing the oldest revision not in a pack file @@ -204,6 +204,10 @@ Addressing: Format 7+: Logical addressing; uses item index that will be translated on-the-fly to the actual rev / pack file location +Repository IDs: + Format 1+: The first line of db/uuid contains the repository UUID + Format 7+: The second line contains the instance ID (in UUID formatting) + # Incomplete list. See SVN_FS_FS__MIN_*_FORMAT Index: subversion/tests/cmdline/svnadmin_tests.py =================================================================== --- subversion/tests/cmdline/svnadmin_tests.py (revision 1617633) +++ subversion/tests/cmdline/svnadmin_tests.py (working copy) @@ -106,6 +106,23 @@ def check_hotcopy_fsfs_fsx(src, dst): raise svntest.Failure("%s does not exist in hotcopy " "destination" % dst_path) + # Special case for db/uuid: Only the UUID in the first line needs + # to match. Source and target must have the same number of lines + # (due to having the same format). + if src_path == os.path.join(src, 'db', 'uuid'): + lines1 = open(src_path, 'rb').read().split("\n") + lines2 = open(dst_path, 'rb').read().split("\n") + if len(lines2) == 0: + raise svntest.Failure("%s is an empty UUID file" + % dst_path) + if len(lines1) != len(lines2): + raise svntest.Failure("%s differs in number of lines" + % dst_path) + if lines1[0] != lines2[0]: + raise svntest.Failure("%s contains different uuid: '%s' vs. '%s'" + % (dst_path, lines1[0], lines2[0])) + continue + # Special case for rep-cache: It will always differ in a byte-by-byte # comparison, so compare db tables instead. if src_file == 'rep-cache.db': @@ -2569,8 +2586,16 @@ def freeze_freeze(sbox): second_repo_dir, _ = sbox.add_repo_path('backup') svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy", sbox.repo_dir, second_repo_dir) - svntest.actions.run_and_verify_svnadmin(None, [], None, - 'setuuid', second_repo_dir) + + if svntest.main.options.server_minor_version < 9: + # Repositories created with --compatible-version=1.8 and less erroneously + # share the filesystem data (locks, shared transaction data, ...) between + # hotcopy source and destination. This is fixed for new FS formats, but + # in order to avoid the SVN_ERR_RECURSIVE_LOCK for old formats, we have + # to manually assign a new UUID for the hotcopy destination. + svntest.actions.run_and_verify_svnadmin(None, [], None, + 'setuuid', second_repo_dir) + svntest.actions.run_and_verify_svnadmin(None, None, [], 'freeze', '--', sbox.repo_dir,
