Stefan Fuhrmann <stefan.fuhrmann_at_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 — i.e., you cannot write or lock a file without 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 denied.
I cannot say that I feel confident about the other proposed option (handling
EACCES after trying to acquire a lock), as opposed to a plain revert. Doing
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 — and that's also going
to be a quite visible regression.
> My point here is that *occasional* failures in a functionality that people
> 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 we
serialize them, naïve post-commit hooks that call 'svnadmin pack' synchronously
could wreak much more havoc. Hotcopy blocks pack, so, if an administrator is
currently performing a backup with 'svnadmin hotcopy', packing will only begin
after the hotcopy is done. Given that hotcopy could take a significant amount
of time, depending on how fast the storage works, users will probably witness
hanging 'svn commit' operations, as the execution of the post-commit hook is
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 change.
Hmm, what could be less intrusive that just reverting a change? I see it as
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 and
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 target
filesystem is immediately openable, even while the hotcopy operation is still
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-current
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 the
destination visible until the hotcopy finishes. This is how the things worked
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 terms,
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
Received on 2015-06-19 14:27:49 CEST