Author: kotkov Date: Fri Aug 15 10:13:23 2014 New Revision: 1618138 URL: http://svn.apache.org/r1618138 Log: Avoid shared data clashes and false cache key aliasing between repositories duplicated using 'hotcopy' or created as a result of dump / load cycles. See the discussion in http://svn.haxx.se/dev/archive-2014-04/0245.shtml and http://svn.haxx.se/dev/archive-2014-08/0093.shtml
This is not a "scalability issue" (as stated in the first of the referenced threads), but rather a full-fledged problem. We have an ability to share data between different objects pointing to same filesystems. This sharing works within a single process boundary; currently we share locks (svn_mutex__t objects) and certain transaction data. Accessing this kind of shared data requires some sort of a key and we used to use a filesystem UUID for this purpose. However, this is *not* good enough for at least a couple of cases. Filesystem UUIDs aren't really unique for every filesystem an end user might have, because they get duplicated during hotcopy, naive copy (copy-paste) or dump / load cycles. 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 2 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. 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. While doing this, also prevent false aliasing for our cache keys by throwing in the instance ID there as well. This kind of aliasing is no better than the shared data clashes — we might encounter a deadly situation when two entirely different filesystems access each other's cached data due to the UUID aliasing. Patch by: stefan2 me * subversion/libsvn_fs_fs/fs.h (SVN_FS_FS__MIN_INSTANCE_ID_FORMAT): New. (fs_fs_data_t.instance_id): New. * subversion/libsvn_fs_fs/fs_fs.h (svn_fs_fs__set_uuid): Add the instance ID parameter. * subversion/libsvn_fs_fs/fs.c (fs_serialized_init): Use an instance ID as a part of the shared data key. (fs_set_uuid): New adapter wrapping the svn_fs_fs__set_uuid() function. Whenever we set a new UUID, imply that filesystem will be a different instance, i.e. have a new unique instance ID. Strictly speaking, our approach should work fine even if we choose to preserve the instance ID upon the UUID bump. However, we stick to the other option — it doesn't make any real difference, but is a bit simpler to implement and (arguably) fits the concept better. Resetting a filesystem UUID probably implies that the user wants to recreate all identification markers for that filesystem, so we might as well generate a new instance ID. (fs_vtable): Use the new fs_set_uuid() adapter here. * subversion/libsvn_fs_fs/fs_fs.c (svn_fs_fs__open): Read the instance ID when it is supported by the format. (svn_fs_fs__set_uuid): Rework this routine in order to support writing and generating instance IDs when required by the filesystem format. (upgrade_body, svn_fs_fs__create): Generate a new instance ID when necessary. * subversion/libsvn_fs_fs/hotcopy.c (hotcopy_create_empty_dest): Unconditionally generate a new instance ID. * subversion/libsvn_fs_fs/caching.c (svn_fs_fs__initialize_caches, svn_fs_fs__initialize_txn_caches): Use an instance ID as a part of the cache key. * subversion/tests/cmdline/svnadmin_tests.py (check_hotcopy_fsfs_fsx): Allow different instance IDs when comparing the 'db/uuid' file contents. (freeze_freeze): Do not change UUID of hotcopy for new formats supporting instance IDs. 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. (freeze_same_uuid): New (fails without the core change). (test_list): Reference the new test. * 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. Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c subversion/trunk/subversion/libsvn_fs_fs/fs.c subversion/trunk/subversion/libsvn_fs_fs/fs.h subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c subversion/trunk/subversion/libsvn_fs_fs/structure subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original) +++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Fri Aug 15 10:13:23 2014 @@ -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), ":", Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original) +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Fri Aug 15 10:13:23 2014 @@ -75,16 +75,26 @@ fs_serialized_init(svn_fs_t *fs, apr_poo in a subpool, add a reference-count, and add a serialized deconstructor 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 implicitly + shared between hotcopied (1), dump / loaded (2) or naively copied (3) + filesystems. We tackle this problem by using a combination of the UUID + and an instance ID as the key. This allows us to avoid key clashing + in (1) and (2) for formats >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT, which + do support instance IDs. For old formats the shared data (locks, shared + transaction data, ...) will still clash. + + Speaking of (3), there is not so much we can do about it, except maybe + provide a convenient way of fixing things. Naively copied filesystems + have identical filesystem UUIDs *and* instance IDs. With the key being + a combination of these two, clashes can be fixed by changing either of + them (or both), e.g. with svn_fs_set_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 +219,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 +239,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, Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.h URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.h?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/fs.h (original) +++ subversion/trunk/subversion/libsvn_fs_fs/fs.h Fri Aug 15 10:13:23 2014 @@ -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 IDs. */ +#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 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 *); 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=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original) +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Fri Aug 15 10:13:23 2014 @@ -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,18 @@ 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 + have one 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 +1662,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,28 +1705,40 @@ 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) { - char *my_uuid; - apr_size_t my_uuid_len; + fs_fs_data_t *ffd = fs->fsap_data; const char *uuid_path = path_uuid(fs, pool); + svn_stringbuf_t *contents = svn_stringbuf_create_empty(pool); if (! uuid) uuid = 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_len = strlen(my_uuid); + if (! instance_id) + instance_id = svn_uuid_generate(pool); + + svn_stringbuf_appendcstr(contents, uuid); + svn_stringbuf_appendcstr(contents, "\n"); + + if (ffd->format >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT) + { + svn_stringbuf_appendcstr(contents, instance_id); + svn_stringbuf_appendcstr(contents, "\n"); + } /* We use the permissions of the 'current' file, because the 'uuid' file does not exist during repository creation. */ - SVN_ERR(svn_io_write_atomic(uuid_path, my_uuid, my_uuid_len, + SVN_ERR(svn_io_write_atomic(uuid_path, contents->data, contents->len, 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; + fs->uuid = apr_pstrdup(fs->pool, uuid); + + if (ffd->format >= SVN_FS_FS__MIN_INSTANCE_ID_FORMAT) + ffd->instance_id = apr_pstrdup(fs->pool, instance_id); + else + ffd->instance_id = fs->uuid; return SVN_NO_ERROR; } Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h (original) +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h Fri Aug 15 10:13:23 2014 @@ -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 / ID instead. Ignore + INSTANCE_ID whenever instance IDs are not supported by the FS format. + 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. Modified: subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c (original) +++ subversion/trunk/subversion/libsvn_fs_fs/hotcopy.c Fri Aug 15 10:13:23 2014 @@ -1090,9 +1090,10 @@ hotcopy_create_empty_dest(svn_fs_t *src_ ? "0\n" : "0 1 1\n"), pool)); - /* Create lock file and UUID. */ + /* Create the lock and 'uuid' files. Hotcopy destination receives a + new instance ID, but has the same filesystem UUID as the source. */ 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) Modified: subversion/trunk/subversion/libsvn_fs_fs/structure URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/structure?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_fs_fs/structure (original) +++ subversion/trunk/subversion/libsvn_fs_fs/structure Fri Aug 15 10:13:23 2014 @@ -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 Modified: subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py?rev=1618138&r1=1618137&r2=1618138&view=diff ============================================================================== --- subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py (original) +++ subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py Fri Aug 15 10:13:23 2014 @@ -106,6 +106,20 @@ 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(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 +2583,19 @@ 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.is_fs_type_fsx() or \ + (svntest.main.is_fs_type_fsfs() and \ + svntest.main.options.server_minor_version < 9): + # FSFS 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 SVN_ERR_RECURSIVE_LOCK + # for old formats, we have to manually assign a new UUID for the + # hotcopy destination. As of trunk@1618024, the same applies to + # FSX repositories. + svntest.actions.run_and_verify_svnadmin(None, [], None, + 'setuuid', second_repo_dir) svntest.actions.run_and_verify_svnadmin(None, None, [], 'freeze', '--', sbox.repo_dir, @@ -2871,6 +2896,44 @@ def fsfs_hotcopy_progress_old(sbox): sbox.repo_dir, inc_backup_dir) +@SkipUnless(svntest.main.is_fs_type_fsfs) +def freeze_same_uuid(sbox): + "freeze multiple repositories with same UUID" + + sbox.build(create_wc=False) + + first_repo_dir, _ = sbox.add_repo_path('first') + second_repo_dir, _ = sbox.add_repo_path('second') + + # Test that 'svnadmin freeze A (svnadmin freeze B)' does not deadlock or + # error out with SVN_ERR_RECURSIVE_LOCK for new FSFS formats, even if 'A' + # and 'B' share the same UUID. Create two repositories by loading the + # same dump file, ... + svntest.main.create_repos(first_repo_dir) + svntest.main.create_repos(second_repo_dir) + + dump_path = os.path.join(os.path.dirname(sys.argv[0]), + 'svnadmin_tests_data', + 'skeleton_repos.dump') + dump_contents = open(dump_path, 'rb').readlines() + svntest.actions.run_and_verify_load(first_repo_dir, dump_contents) + svntest.actions.run_and_verify_load(second_repo_dir, dump_contents) + + # ...and execute the 'svnadmin freeze -F' command. + if svntest.main.options.server_minor_version < 9: + expected_error = ".*svnadmin: E200043:.*" + else: + expected_error = None + + arg_file = sbox.get_tempname() + svntest.main.file_write(arg_file, + "%s\n%s\n" % (first_repo_dir, second_repo_dir)) + + svntest.actions.run_and_verify_svnadmin(None, None, expected_error, + 'freeze', '-F', arg_file, '--', + sys.executable, '-c', 'True') + + ######################################################################## # Run the tests @@ -2922,6 +2985,7 @@ test_list = [ None, fsfs_hotcopy_progress, fsfs_hotcopy_progress_with_revprop_changes, fsfs_hotcopy_progress_old, + freeze_same_uuid, ] if __name__ == '__main__':
