bliss@tigris.org writes:
> Modified:
> trunk/subversion/mod_dav_svn/liveprops.c
> Log:
> Fix the horrible ra_dav->get_dir performance introduced in 1.1.0-rc4.
>
> ...also to be known as...
>
> Fix the horrible ls performance over ra_dav introduced in 1.1.0-rc4.
>
> If svn_repos_fs_revision_prop is supplied a path read authz function,
> it will run that function on all changed paths for the revision to
> verify that the revision is readable. That is a very expensive
> operation, but the best we can do when we simply want to read a
> revision property.
>
> The liveprops code in ra_dav already has a path and wants to read a
> couple of revision properties for the last changed revision of those
> paths. By letting mod_dav_svn checking the path first, and not
> letting svn_repos_fs_revision_prop do any authz we save an enormous
> amount of work.
>
> One test case (which I'm too lazy to describe how to set up here, but
> the client operation was a non-recursive ls of a directory with 412
> files) went down from six minutes to three seconds with this change.
>
> * subversion/mod_dav_svn/liveprops.c
> (dav_svn_insert_prop, dav_svn_get_last_modified_time): Call
> dav_svn_authz_read on our path, and pass NULL as path read authz
> callback to svn_repos_fs_revision_prop.
Quick sanity check: the reason this is okay is because of the
paragraph in section 5 of notes/authz_policy.txt, that says:
* If a revision has a mixture of readable/unreadable
changed-paths, then all revprops are unreadable, except for
svn:author and svn:date. All revprops are unwritable.
right? The only properties we're grabbing here are svn:author and
svn:date, exactly the two that are allowed even if there are some
other unreadable paths in the revision. Therefore all we need to know
is that *at least* one path is readable, and we do know that.
Am I on the right track?
Code comments below:
> --- trunk/subversion/mod_dav_svn/liveprops.c (original)
> +++ trunk/subversion/mod_dav_svn/liveprops.c Sun Oct 3 13:54:53 2004
> @@ -117,7 +117,6 @@
> apr_pool_t *p = resource->info->pool;
> const dav_liveprop_spec *info;
> int global_ns;
> - dav_svn_authz_read_baton arb;
> svn_error_t *serr;
>
> /*
> @@ -131,10 +130,6 @@
> if (!resource->exists)
> return DAV_PROP_INSERT_NOTSUPP;
>
> - /* Build a baton for path-based authz function (dav_svn_authz_read) */
> - arb.r = resource->info->r;
> - arb.repos = resource->info->repos;
> -
> /* ### we may want to respond to DAV_PROPID_resourcetype for PRIVATE
> ### resources. need to think on "proper" interaction with mod_dav */
>
> @@ -188,6 +183,8 @@
> {
> svn_revnum_t committed_rev = SVN_INVALID_REVNUM;
> svn_string_t *last_author = NULL;
> + dav_svn_authz_read_baton arb;
> + svn_boolean_t allowed;
>
> /* ### for now, our global VCC has no such property. */
> if (resource->type == DAV_RESOURCE_TYPE_PRIVATE
> @@ -222,13 +219,32 @@
> {
> return DAV_PROP_INSERT_NOTSUPP;
> }
> -
> - /* Get the date property of the created revision. */
Heh, it appears the old comment said "date" where it meant "author",
but...
> +
> + /* Check if we have access to this path and return NOTDEF if
> + we don't. */
> + arb.r = resource->info->r;
> + arb.repos = resource->info->repos;
> + serr = dav_svn_authz_read(&allowed,
> + resource->info->root.root,
> + resource->info->repos_path,
> + &arb, p);
> + if (serr)
> + {
> + /* ### what to do? */
> + svn_error_clear(serr);
> + value = "###error###";
> + break;
> + }
> + if (! allowed)
> + return DAV_PROP_INSERT_NOTDEF;
> +
> + /* Get the date property of the created revision. The authz is
> + already performed, so we don't need to do it here too. */
> serr = svn_repos_fs_revision_prop(&last_author,
> resource->info->repos->repos,
> committed_rev,
> SVN_PROP_REVISION_AUTHOR,
> - dav_svn_authz_read, &arb, p);
> + NULL, NULL, p);
...your new comment also says "date" when in fact we're fetching the
author.
The comment should maybe also state the at-least-one-readable-path
policy for author, to remind the programmer of why this is okay.
> if (serr != NULL)
> {
> /* ### what to do? */
> @@ -667,13 +683,27 @@
> svn_error_t *serr;
> apr_time_t timeval_tmp;
> dav_svn_authz_read_baton arb;
> -
> - arb.r = resource->info->r;
> - arb.repos = resource->info->repos;
> + svn_boolean_t allowed;
>
> if ((datestring == NULL) && (timeval == NULL))
> return 0;
>
> + /* Check if we have access to this path and return NOTDEF if we
> + don't. */
> + arb.r = resource->info->r;
> + arb.repos = resource->info->repos;
> + serr = dav_svn_authz_read(&allowed,
> + resource->info->root.root,
> + resource->info->repos_path,
> + &arb, pool);
> + if (serr)
> + {
> + svn_error_clear(serr);
> + return 1;
> + }
> + if (! allowed)
> + return 1;
> +
This is exactly the same code as before, right? Maybe worth
abstracting?
> if (resource->baselined && resource->type == DAV_RESOURCE_TYPE_VERSION)
> {
> /* A baseline URI. */
> @@ -698,12 +728,13 @@
> return 1;
> }
>
> - /* Get the svn:date property of the CR */
> + /* Get the svn:date property of the CR. The authz is already
> + performed, so we don't need to do it here too. */
> serr = svn_repos_fs_revision_prop(&committed_date,
> resource->info->repos->repos,
> committed_rev,
> SVN_PROP_REVISION_DATE,
> - dav_svn_authz_read, &arb,
> + NULL, NULL,
> pool);
> if (serr != NULL)
> {
Here "date" is correct :-).
But this comment also could maybe state the at-least-one-readable-path
policy for dates, as a reminder. Most people won't know the authz
policy by heart.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Oct 4 19:20:31 2004