Stefan Fuhrmann <stefanfuhrmann_at_alice-dsl.de> writes:
> This branch adds support for HTTP2-style transactions to all backends.
> The feature is opt-in and supported in all 3 FS implementations.
>
> I'd like to merge this to /trunk next weekend, so that e.g. Bert can
> continue the work on the network layer. Because it is opt-in, removing
> the feature later should be simple - in case we find a major problem
> with it.
>
> Any objections?
I have several concerns about the particular design and implementation:
(1) Requiring twice more disk writes and space
Changes that don't fit into memory require twice more disk writes and space
in the parallel mode, because data is first spilled to a temporary file, and
then appended to the protorev file. Even if we negotiate the use of this
feature, and only enable it for HTTP/2 capable clients and servers, that
might still be slower for every case except when committing over a
high-latency network.
(2) Making the feature opt-in
Since the feature is opt-in and disabled by default, we would be adding
another layer to the amount of different code execution paths that we need
to support and test (FS formats, addressing, parallel / non-parallel writes).
And we cannot enable it by default due to (1).
(3) Opening a denial of service attack vector
In the parallel mode, a server holds up to 0.5 MB of data in memory per
every change. Combined with the fact that writes to the protorev files
are serialized via rev-lock, an evil doer could be able to exhaust all
available memory on the server by issuing a huge PUT request, waiting for
the moment when the rev-lock is acquired and spamming with a big amount of
smaller PUT requests. The latter requests would block until the rev-lock
file is released, but the associated data is going to be kept in memory.
Alternatively, this could lead to a starvation between httpd workers, where
most of them are blocked waiting for a single PUT to complete and are
locking out other clients.
(4) Being incompatible with released versions of Subversion
Support for parallel writes is implemented without a format bump, and can
result in failing commits or data corruption due to mismatching concurrency
settings. Consider an environment with a reverse proxy doing load balancing
between 1.9 and 1.10 mod_dav_svn servers that work with the same repository.
A client performs two parallel PUT requests, and each of them is rerouted to
each of the servers. Consequently, either the 1.9 server fails to acquire
a nonblocking rev-lock and the commit fails, or, in a racy scenario, the
file/offset manipulation logic in transaction.c:rep_write_contents_close()
results in a corrupted revision.
Regards,
Evgeny Kotkov
Received on 2016-01-29 22:35:42 CET