On Wed, Apr 23, 2014 at 3:07 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 23 April 2014 15:57, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
> wrote:
> > On Wed, Apr 23, 2014 at 1:28 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> >>
> >> On 22 April 2014 23:36, <stefan2_at_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.
Received on 2014-04-23 23:11:02 CEST