You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2013/12/17 13:48:39 UTC
Performance of "svn lock *"
One of WANdisco's customers brought the performance of "svn lock *" to
my attention. There is a network issue over HTTP and a filesystem issue
in the repository.
When locking multiple files over HTTP the client sends a separate LOCK
request for each file and the round trip delays add up. Also the
bandwidth overhead, all the HTTP headers, is high: using one request per
path it is not an efficient way to transport paths.
On the repository side creating or removing a lock involves writing an
index file for each parent directory in addition to handling the lock
file itself. To lock N separate files in a directory '/A/B/C' involves
writing N times the index files for '/', '/A', '/A/B' and '/A/B/C' as
well as handling the N lock files. To lock N files at depth D we do
O(N*D) writes but only modify O(N+D) distinct files; it doesn't scale
very well.
We already pass multiple paths into svn_ra_lock so we could address part
of the network problem by rewriting some serf code to make it pipeline
the LOCK requests. That would have the advantage of working with older
servers but to solve all the problems we need to make HTTP more like the
svn protocol: send a single request (perhaps POST instead of LOCK?) for
the repository root and pass all the paths in the body of the request.
Once we have all the paths arriving at the server in one request we can
add new FS APIs to lock/unlock multiple paths, then sort the paths and
write each index file only once.
I'm not quite sure how hooks would behave. We would need to run all the
pre-lock hooks first and some could fail. We could drop the paths that
fail and pass the rest to the FS layer, or perhaps fail the whole
operation if any pre-lock fails. Either way the FS layer may fail to
lock some of the paths for various reasons (non-existant, already
locked, etc.) and so the final set of locks could be smaller than the
set of paths. Finally we run post-lock for the subset of paths that are
locked.
There is also an FS atomic issue to consider. The current single path
API can be interrupted between writing index files and handling the lock
file, but the result is the single path is locked or unlocked with any
"broken" index files being invisible to the user (athough the "broken"
index files may cause more work for the server). A multiple path API
could result in user visible changes to a subset of the paths. I think
that would be OK but it needs a bit more thought.
I've also noticed that we don't fsync any files when writing locks into
the repository. I'm not sure if this is deliberate or not but if we
were to start calling fsync then the filesystem issue would become more
important.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Performance of "svn lock *"
Posted by Philip Martin <ph...@wandisco.com>.
Bert Huijben <be...@qqmail.nl> writes:
> All those options work, but you could also just follow the Ra code and
> implement a callback with the result. The Ra layers can then batch or
> stream the result as they wish.
Reusing the RA design is worth considering. The interaction between the
repos hooks and FS write-lock makes it bit more difficult. This new FS
API will have a corresponding repos API that runs pre-lock and
post-lock. The repos layer would run all pre-locks before invoking FS
and the FS callback would invoke the repos callback. The repos callback
would run the post-lock hooks but we don't want to do that while holding
the FS write-lock. So the FS callbacks have to happen after the
write-lock is released.
svn_repos_lock(paths, server_callback)
for path in paths:
run pre-lock
svn_fs_lock(paths, repos_callback)
take write-lock
for path in paths:
create lock
release write-lock
for path in paths:
callback-to-repos
repos_callback
if lock
run post-lock
callback-to-server
server_callback
...
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Performance of "svn lock *"
Posted by Bert Huijben <be...@qqmail.nl>.
All those options work, but you could also just follow the Ra code and implement a callback with the result. The Ra layers can then batch or stream the result as they wish.
Bert
Sent from Windows Mail
From: Philip Martin
Sent: Thursday, December 19, 2013 3:28 PM
To: Subversion Development
Philip Martin <ph...@wandisco.com> writes:
> Once we have all the paths arriving at the server in one request we can
> add new FS APIs to lock/unlock multiple paths, then sort the paths and
> write each index file only once.
I'm wondering what sort of API would be suitable here if we were to do
this. We currently have
svn_error_t *
svn_fs_lock(svn_lock_t **lock,
svn_fs_t *fs,
const char *path,
const char *token,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
svn_revnum_t current_rev,
svn_boolean_t steal_lock,
apr_pool_t *pool);
Suppose we were to pass an array of paths and tokens, and return an
array of locks:
svn_error_t *
svn_fs_lock(apr_array_header_t **locks,
svn_fs_t *fs,
apr_array_header_t *paths,
apr_array_header_t *tokens,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
svn_revnum_t current_rev,
svn_boolean_t steal_lock,
apr_pool_t *pool);
That could be implemented but the tricky bit is error handling when some
paths can be locked and others cannot. When the single path function is
invoked multiple times the result is some paths locked while others
generate warnings such as:
svn: warning: apr_err=SVN_ERR_FS_NOT_FOUND
svn: warning: W160013: Path '/A/B/C/h' doesn't exist in HEAD revision
svn: warning: apr_err=SVN_ERR_FS_PATH_ALREADY_LOCKED
svn: warning: W160035: Path '/A/B/C/g' is already locked by user 'pm' in filesystem 'd79b647c-16e3-4f8b-8e91-65796fcf634a'
A multiple path function would probably want to preserve the behaviour
that allows locking and unlocking a partial set of paths, after all we
rely on this in out own code. The locks array would be OK, we could
return NULL elements for paths that are not locked, but how do we return
the corresponding errors?
Do we chain the errors together in the single return error? Do we
return an array of errors as well as an array of locks?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Performance of "svn lock *"
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Once we have all the paths arriving at the server in one request we can
> add new FS APIs to lock/unlock multiple paths, then sort the paths and
> write each index file only once.
I'm wondering what sort of API would be suitable here if we were to do
this. We currently have
svn_error_t *
svn_fs_lock(svn_lock_t **lock,
svn_fs_t *fs,
const char *path,
const char *token,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
svn_revnum_t current_rev,
svn_boolean_t steal_lock,
apr_pool_t *pool);
Suppose we were to pass an array of paths and tokens, and return an
array of locks:
svn_error_t *
svn_fs_lock(apr_array_header_t **locks,
svn_fs_t *fs,
apr_array_header_t *paths,
apr_array_header_t *tokens,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
svn_revnum_t current_rev,
svn_boolean_t steal_lock,
apr_pool_t *pool);
That could be implemented but the tricky bit is error handling when some
paths can be locked and others cannot. When the single path function is
invoked multiple times the result is some paths locked while others
generate warnings such as:
svn: warning: apr_err=SVN_ERR_FS_NOT_FOUND
svn: warning: W160013: Path '/A/B/C/h' doesn't exist in HEAD revision
svn: warning: apr_err=SVN_ERR_FS_PATH_ALREADY_LOCKED
svn: warning: W160035: Path '/A/B/C/g' is already locked by user 'pm' in filesystem 'd79b647c-16e3-4f8b-8e91-65796fcf634a'
A multiple path function would probably want to preserve the behaviour
that allows locking and unlocking a partial set of paths, after all we
rely on this in out own code. The locks array would be OK, we could
return NULL elements for paths that are not locked, but how do we return
the corresponding errors?
Do we chain the errors together in the single return error? Do we
return an array of errors as well as an array of locks?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Performance of "svn lock *"
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/17/2013 10:06 AM, C. Michael Pilato wrote:
> Just piping in on this one point. POST is perfectly acceptable here,
> and is not in conflict with the DAV spec in the least. Just because we
> add a new route for locking/unlocked multiple paths (via POST) doesn't
> mean we'll stop supporting the DAV LOCK/UNLOCK one-path-at-a-time route.
> That's all the compatibility -- with old Subversion clients and generic
> WebDAV clients alike -- that we need to fuss with.
By the way, I have no fewer than *six* patches with various stages of
aborted attempts at this approach for this very problem. :-(
Re: Performance of "svn lock *"
Posted by Ben Reser <be...@reser.org>.
On 12/17/13 7:06 AM, C. Michael Pilato wrote:
> Just piping in on this one point. POST is perfectly acceptable here,
> and is not in conflict with the DAV spec in the least. Just because we
> add a new route for locking/unlocked multiple paths (via POST) doesn't
> mean we'll stop supporting the DAV LOCK/UNLOCK one-path-at-a-time route.
> That's all the compatibility -- with old Subversion clients and generic
> WebDAV clients alike -- that we need to fuss with.
I agree with this. We've already started down the path of implementing our own
more efficient protocol with HTTPv2. While I think we should limit where we
deviate, if there's a good reason then we should do so. The only alternative
in my opinion is to start writing RFCs to update DAV. Maybe that's a good
thing to do anyway. But at this point I doubt there's much interest in DAV let
alone DeltaV.
The changes we've made so far with HTTPv2 were to save round trips. While
collapsing LOCK requests into a single request wouldn't be as beneficial as the
HTTPv2 changes I don't really see a reason to say we shouldn't do this if
someone wants to do the work.
Re: Performance of "svn lock *"
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/17/2013 09:35 AM, Branko Čibej wrote:
> I can't see us using POST instead of LOCK. AFAIU that's in conflict with
> the DAV spec; we already know that we could have writen a much more
> efficient custom HTTP protocol, and decided against it for good reasons.
> Pipelining LOCK requests sounds like the way to go; people can upgrade
> their servers if they really want this.
Just piping in on this one point. POST is perfectly acceptable here,
and is not in conflict with the DAV spec in the least. Just because we
add a new route for locking/unlocked multiple paths (via POST) doesn't
mean we'll stop supporting the DAV LOCK/UNLOCK one-path-at-a-time route.
That's all the compatibility -- with old Subversion clients and generic
WebDAV clients alike -- that we need to fuss with.
Re: Performance of "svn lock *"
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> On 17.12.2013 13:48, Philip Martin wrote:
>> On the repository side creating or removing a lock involves writing an
>> index file for each parent directory in addition to handling the lock
>> file itself. To lock N separate files in a directory '/A/B/C' involves
>> writing N times the index files for '/', '/A', '/A/B' and '/A/B/C' as
>> well as handling the N lock files. To lock N files at depth D we do
>> O(N*D) writes but only modify O(N+D) distinct files; it doesn't scale
>> very well.
>
> I don't understand what the index file is for ... could this be
> something we could just stop writing? And what about backwards compat?
Operations like delete need to know if there are any locks in a given
tree. The index files are necessary to provide a quick answer to
questions like: "are there any locks in the tree '/A/B'. Without the
index files we would need to examine every path in the tree or every
lock in the repository.
>> We already pass multiple paths into svn_ra_lock so we could address part
>> of the network problem by rewriting some serf code to make it pipeline
>> the LOCK requests. That would have the advantage of working with older
>> servers but to solve all the problems we need to make HTTP more like the
>> svn protocol: send a single request (perhaps POST instead of LOCK?) for
>> the repository root and pass all the paths in the body of the request.
>
> I can't see us using POST instead of LOCK. AFAIU that's in conflict with
> the DAV spec; we already know that we could have writen a much more
> efficient custom HTTP protocol, and decided against it for good reasons.
> Pipelining LOCK requests sounds like the way to go; people can upgrade
> their servers if they really want this.
Pipelining doesn't allow us to solve the N*M write problem in the
repository. We could do it for svn: which already sends a "lock-many"
request but not for HTTP. Perhaps that problem isn't important?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Performance of "svn lock *"
Posted by Branko Čibej <br...@wandisco.com>.
On 17.12.2013 13:48, Philip Martin wrote:
> One of WANdisco's customers brought the performance of "svn lock *" to
> my attention. There is a network issue over HTTP and a filesystem issue
> in the repository.
My gut feeling is to just forget it. If someone wants to lock the whole
working copy, they can deal with the performance penalty as well.
> When locking multiple files over HTTP the client sends a separate LOCK
> request for each file and the round trip delays add up. Also the
> bandwidth overhead, all the HTTP headers, is high: using one request per
> path it is not an efficient way to transport paths.
>
> On the repository side creating or removing a lock involves writing an
> index file for each parent directory in addition to handling the lock
> file itself. To lock N separate files in a directory '/A/B/C' involves
> writing N times the index files for '/', '/A', '/A/B' and '/A/B/C' as
> well as handling the N lock files. To lock N files at depth D we do
> O(N*D) writes but only modify O(N+D) distinct files; it doesn't scale
> very well.
I don't understand what the index file is for ... could this be
something we could just stop writing? And what about backwards compat?
> We already pass multiple paths into svn_ra_lock so we could address part
> of the network problem by rewriting some serf code to make it pipeline
> the LOCK requests. That would have the advantage of working with older
> servers but to solve all the problems we need to make HTTP more like the
> svn protocol: send a single request (perhaps POST instead of LOCK?) for
> the repository root and pass all the paths in the body of the request.
I can't see us using POST instead of LOCK. AFAIU that's in conflict with
the DAV spec; we already know that we could have writen a much more
efficient custom HTTP protocol, and decided against it for good reasons.
Pipelining LOCK requests sounds like the way to go; people can upgrade
their servers if they really want this.
I'll skip the rest as it relies on us deciding to actually find this
scenario important.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com