On 23 April 2014 15:57, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote: > 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. > I don't have solution, except adding second 'instance UUID' that's updated on hotcopy and repository create.
But you fix making things worse: before your change locks only scalability/performance issue exists, but now same repositories accessed with different path will get different shared data and transaction list will not be locked correctly. I disagree with Bert that problem I describe is not possible in normal application: any server using SVNParentPath and svnserve are affected, because it uses part of URL as repository name. So users accessing http://server/repo and http://server/REPO will access repository with different path and get different FS shared data. -- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com
