Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: domain of evgeny.kotkov@visualsvn.com
 designates 209.85.215.47 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CA+t0gk3fGP1K_5+GQq5c6rUFtW7WNE2LUwVwY8crVj_b-v69-g@mail.gmail.com>
References: 
 <CAP_GPNiMBAJPo9_f-SBcmOFz==BUm96fbUkFb8SbKb5fknr+4A@mail.gmail.com>
 <CA+t0gk0B0V9hmDWHvbEJ8zQDc6FMjEzn-uiT6keGaqh+c4MtxA@mail.gmail.com>
 <CAP_GPNgHdcaObPtW3pApOLfdgk4K_UGbM6aXr33-V3C3eV2fYg@mail.gmail.com>
 <CA+t0gk15pek0UBR9DeqJ0e_QkjSoTEQNHhvxtwMn6L8Qaecjag@mail.gmail.com>
 <CAP_GPNjAWehP2P4rUERqxL-3e38sNnTudtmYhkxsBRJf7voUWg@mail.gmail.com>
 <CA+t0gk3fGP1K_5+GQq5c6rUFtW7WNE2LUwVwY8crVj_b-v69-g@mail.gmail.com>
From: Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
Date: Fri, 19 Jun 2015 15:26:14 +0300
Message-ID: 
 <CAP_GPNiMGozbgwnoOBeJBMc0XF8aM3meC0QiMubVLkxz=kV=iw@mail.gmail.com>
Subject: Re: FSFS7: 'svnadmin hotcopy' requires write access to the source
To: Stefan Fuhrmann <stefan.fuhrmann@wandisco.com>
Cc: Subversion Development <dev@subversion.apache.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Stefan Fuhrmann <stefan.fuhrmann@wandisco.com> writes:

> And the regression should be fixed.

[...]

> There is a trivial fix: Try to acquire the lock and fall back to the
> non-locking behavior if you get an EACCESS. The nicer way is to check
> beforehand and make the check slightly more generic. That's what I did in
> r1686232.

I don't see how r1686232 could possibly work on Windows.  The APR_FREADONLY
check tells us if a file has FILE_ATTRIBUTE_READONLY.  This is orthogonal
to the DACL of the file =E2=80=94 i.e., you cannot write or lock a file wit=
hout having
an appropriate access control entry in the DACL, even if the file does not
have a read-only attribute.  With this fix, the following scenario (attempt
to hotcopy a source repository while only having R access in DACL) is still
going to fail:

  svnadmin create source
  svnadmin pack source
  icacls source /inheritance:r /grant MyAccountName:(OI)(CI)(R)
  svnadmin hotcopy source target
  svnadmin: E720005: Can't open file 'source\db\pack-lock': Access is denie=
d.

I cannot say that I feel confident about the other proposed option (handlin=
g
EACCES after trying to acquire a lock), as opposed to a plain revert.  Doin=
g
so is going to put us into a 12 second retry loop in the common case where
a Windows account can only read the hotcopy source =E2=80=94 and that's als=
o going
to be a quite visible regression.

> My point here is that *occasional* failures in a functionality that peopl=
e
> might use for creating backups is a problem. For example, people running
> pack in their post-commit hooks may not notice the failure during testing=
,
> so they don't implement the redo fallback in their backup procedure. Or,
> they notice the failure and get that unsettling feeling ...

Is this argument for or against serializing hotcopy and pack?  Because if w=
e
serialize them, na=C3=AFve post-commit hooks that call 'svnadmin pack' sync=
hronously
could wreak much more havoc.  Hotcopy blocks pack, so, if an administrator =
is
currently performing a backup with 'svnadmin hotcopy', packing will only be=
gin
after the hotcopy is done.  Given that hotcopy could take a significant amo=
unt
of time, depending on how fast the storage works, users will probably witne=
ss
hanging 'svn commit' operations, as the execution of the post-commit hook i=
s
going to be blocked by the hotcopy.

The last, but not the least, hotcopy now blocks other hotcopies, so one can
no longer perform the backups in a parallel manner, e.g., to two different
backup destinations.

> Reverting the whole of r1589284 would be excessive. Simply not using the
> "with_pack_lock" call would make for a much smaller, less intrusive chang=
e.

Hmm, what could be less intrusive that just reverting a change?  I see it a=
s
that if we stop locking the hotcopy source, we don't require other relevant
bits of this change, i.e., the supplementary logic that made this locking
possible.  Why should we keep it?

Doing so would also allow us to get rid of the inconsistency in how repos a=
nd
fs layers currently perform the hotcopy that I mentioned in [1].  Currently=
,
if an API user calls svn_fs_hotcopy3() in the non-incremental mode, the tar=
get
filesystem is immediately openable, even while the hotcopy operation is sti=
ll
in progress).  This means that anyone calling svn_fs_open2() on this target
could be working with a filesystem stub without the lock tree or db/txn-cur=
rent
file, and I don't think there is anything good in that, quoting [1]:

[[[
The whole idea of the non-incremental hotcopy is that it *does not* make th=
e
destination visible until the hotcopy finishes. This is how the things work=
ed
prior to this changeset and this is how the svn_repos layer currently works=
.
By writing the format file before actually doing something we do not only
expose our internal stub to the world (which really is a stub without even =
r0),
but will also leave this internal stub openable in case the hotcopy fails.
This stub does not even have to be a normal filesystem and we only need
it to bootstrap the hotcopy process, but we now behave as if it was!

Finally, the 'repos' and 'fs' layers now behave inconsistently in these ter=
ms,
i.e. svn_repos_hotcopy3() still writes the format file only when everything=
 is
done.
]]]

What do you think?

[1] http://svn.haxx.se/dev/archive-2014-07/0124.shtml


Regards,
Evgeny Kotkov