On Sat, Mar 03, 2018 at 11:43:31AM +0100, Stefan Sperling wrote:
> On Fri, Mar 02, 2018 at 09:40:28PM +0000, Philip Martin wrote:
> > Philip Martin <phi...@codematters.co.uk> writes:
> >
> > > Again, 1.10 would be nearly twice as fast, but now rereading the authz
> > > removes most of that gain.
> >
> > I think I see the underlying problem: the authz code now incorporates a
> > cache based on the md5 checksum of the rules, so when the rules are
> > unchanged the cached value can be reused. This cache relies on the
> > caller being able to pass an svn_repos_t to svn_repos_authz_read3() and,
> > while svnserve does indeed pass such a pointer, mod_authz_svn is passing
> > NULL. That means mod_authz_svn does not take advantage of the new authz
> > cache.
> >
> > Stefan's pool patch helps, but I believe the authz rereading in
> > mod_authz_svn should be reverted from 1.10 unless we can make it take
> > advantage of the new authz cache.
>
> Yes, the problem seems to be that mod_authz_svn has no way of storing
> per-connection state at present. The memory usage problem happened
> because it kept adding new access_conf objects to the per-connection
> pool since it has no way of knowing whether a previous request did
> already create the same object.
>
> We could store per-connection data by using members of r->conn, such
> as r->conn->id or r->notes to index into a cache of svn_repos_t stored
> in r->conn's pool. But where could we store a pointer to this cache
> so that it could be read back from the request structure r?
>
> Maybe we could use a global cache of svn_repos_t objects which live
> throughout the lifetime of mod_authz_svn and are shared across connections?
> It probably won't grow out of bounds as the number of repositories is
> relatively constant. But it's unclear how to ensure it stays in sync
> with the on-disk state. Hmm...
I may have found a way to store an svn_repos_t object on the connection.
Does it help? (authz_tests pass)
Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 1825762)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -409,6 +409,7 @@ get_access_conf(request_rec *r, authz_svn_config_r
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err = SVN_NO_ERROR;
dav_error *dav_err;
+ svn_repos_t *repos;
dav_err = dav_svn_get_repos_path2(r, conf->base_path, &repos_path,
scratch_pool);
if (dav_err)
@@ -465,9 +466,24 @@ get_access_conf(request_rec *r, authz_svn_config_r
"Path to groups file is %s", groups_file);
}
+ /* Store an svn_repos_t on this connection to allow authz config caching. */
+ repos = ap_get_module_config(r->connection->conn_config, &authz_svn_module);
+ if (repos == NULL)
+ {
+ svn_err = svn_repos_open3(&repos, repos_path, NULL, r->connection->pool,
+ scratch_pool);
+ if (svn_err)
+ {
+ log_svn_error(APLOG_MARK, r, "Failed to open repository:",
+ svn_err, scratch_pool);
+ return NULL;
+ }
+ ap_set_module_config(r->connection->conn_config, &authz_svn_module,
repos);
+ }
+
svn_err = svn_repos_authz_read3(&access_conf,
access_file, groups_file,
- TRUE, NULL,
+ TRUE, repos,
result_pool, scratch_pool);
if (svn_err)
