On Wed, Apr 23, 2014 at 3:07 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> 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. > I plan to add instance IDs in 1.10. They are useful in a variety of scenarios where you want guarantee cache consistency with externally modified repositories. 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. > Before the change, there would be a deadlock when a thread tried to take out the same locks on different repositories that happen to have the same UUID. This is why r1589518 is not a simple revert. 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. > Since no functionality depended on the change so far, except for some extra fool-proving during hotcopy, I reverted the change in r1589518. 1.10 will bring the more reliable instance ID fix. -- Stefan^2.
