On Wed, Apr 23, 2014 at 1:28 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 22 April 2014 23:36, <stef...@apache.org> wrote: > > Author: stefan2 > > Date: Tue Apr 22 19:36:12 2014 > > New Revision: 1589262 > > > > URL: http://svn.apache.org/r1589262 > > Log: > > Fix a scalability issue with FSFS repositories: The lock mutexes are > > part of the process-wide per-repo shared structs. Using only the > > UUID as identification for the repo is not enough in case a repo gets > > duplicated or the admins don't follow good practice. The result may > > be serializing all commits globally across all repos. > > > > Now, we simply add the normalized repo path to the hash key but keep > > the UUID as well. The latter allows us to detect and handle replaced > > repositories gracefully in some cases. > > > > * subversion/libsvn_fs_fs/fs.c > > (fs_serialized_init): Add FS path to the hash lookup key. > > > > Modified: > > subversion/trunk/subversion/libsvn_fs_fs/fs.c > > > > Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c > > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1589262&r1=1589261&r2=1589262&view=diff > > > ============================================================================== > > --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original) > > +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Tue Apr 22 19:36:12 > 2014 > > @@ -72,19 +72,12 @@ 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 > > - 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. */ > > + in a subpool, add a reference-count, and add a serialized > destructor > > + to the FS vtable. That's more machinery than it's worth. */ > > > > SVN_ERR_ASSERT(fs->uuid); > > key = apr_pstrcat(pool, SVN_FSFS_SHARED_USERDATA_PREFIX, fs->uuid, > > + svn_fs__canonicalize_abspath(fs->path, pool), > > SVN_VA_NULL); > Hi Stefan, > > I agree that problem is important and should be fixed somehow, but > committed fix is wrong IMHO: > 1. svn_fs__canonicalize_abspath is for FS path canonicalization, not > for disk path > 2. fs->path is not absolute path, so you may get different keys for > same repository accessed but different path relative path > 3. On Windows path names is case insensitive and repository can be > accessed using 'C:\REPO' and 'C:\repo' path names. Converting path to > upper case because Windows performs different normalization like > removing trailing dot and etc. Reimplementing this logic in Subversion > will be very hard. > Alright, do you have a suggestion for how to handle that problem? Without r1589262, we can't lock the source *and* target of a hotcopy - which is how I ran into this issue. -- Stefan^2.
