Stefan Fuhrmann <stefanfuhrm...@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
