On Mon, Oct 17, 2022 at 12:39 PM Greg Stein <gstein@gmail.com> wrote:
>
> Did you run any tests to observe the alleged contention?
No I didn't (not a user of mod_dav myself), but Emmanuel (CCed) who
submitted the patch seems to encounter issues in some environment(s),
per PR 66313.
>
> The dbm database is very fast. I'd be surprised that contention occurs in any typical workload.
Dunno, I'll let Emmanuel argue here..
Cheers;
Yann.
>
> On Mon, Oct 17, 2022 at 4:48 AM <ylavic@apache.org> wrote:
>>
>> Author: ylavic
>> Date: Mon Oct 17 09:48:11 2022
>> New Revision: 1904638
>>
>> URL: http://svn.apache.org/viewvc?rev=1904638&view=rev
>> Log:
>> mod_dav: Allow to disable lock discovery via an DAVLockDiscovery expression.
>>
>> mod_dav-fs scales badly when a few clients run PROPFIND requests to discover
>> directory content. Each PROPFIND involves lockdiscovery, which in turn waits
>> for a locked access to the file containing the lock database. Performances
>> quickly drop because of lock contention on this file.
>>
>> Add a DAVLockDiscovery configuration directive that allows lockdiscovery to be
>> disabled. Its argument is an Apache expression so that flexible configuration
>> are possible (per-request).
>>
>> When lock discovery is disabled, an empty lockdiscovery property is returned on
>> POPRFIND methods, just like if no lock was set on the object. That should cause
>> no regression, since a client cannot rely on lockdiscovery to decide when a
>> file should be accessed, the LOCK methood must be used.
>>
>> If DAVLockDiscovery is not specified, the behavior is unchanged.
>>
>>
>> PR 66313.
>> Submitted by: Emmanuel Dreyfus <manu netbsd.org>
>> Reviewed by: ylavic
>>
>>
>> Added:
>> httpd/httpd/trunk/changes-entries/DAVLockDiscovery.txt
>> Modified:
>> httpd/httpd/trunk/modules/dav/main/mod_dav.c
>> httpd/httpd/trunk/modules/dav/main/mod_dav.h
>> httpd/httpd/trunk/modules/dav/main/props.c
>>
>> Added: httpd/httpd/trunk/changes-entries/DAVLockDiscovery.txt
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/DAVLockDiscovery.txt?rev=1904638&view=auto
>> ==============================================================================
>> --- httpd/httpd/trunk/changes-entries/DAVLockDiscovery.txt (added)
>> +++ httpd/httpd/trunk/changes-entries/DAVLockDiscovery.txt Mon Oct 17 09:48:11 2022
>> @@ -0,0 +1,2 @@
>> + *) mod_dav: Allow to disable lock discovery via an DAVLockDiscovery
>> + expression (per-request). PR 66313. [Emmanuel Dreyfus <manu netbsd.org>]
>>
>> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1904638&r1=1904637&r2=1904638&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
>> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Mon Oct 17 09:48:11 2022
>> @@ -83,6 +83,7 @@ typedef struct {
>> const char *dir;
>> int locktimeout;
>> int allow_depthinfinity;
>> + ap_expr_info_t *allow_lockdiscovery;
>>
>> } dav_dir_conf;
>>
>> @@ -203,6 +204,8 @@ static void *dav_merge_dir_config(apr_po
>> newconf->dir = DAV_INHERIT_VALUE(parent, child, dir);
>> newconf->allow_depthinfinity = DAV_INHERIT_VALUE(parent, child,
>> allow_depthinfinity);
>> + newconf->allow_lockdiscovery = DAV_INHERIT_VALUE(parent, child,
>> + allow_lockdiscovery);
>>
>> return newconf;
>> }
>> @@ -301,6 +304,30 @@ static const char *dav_cmd_davdepthinfin
>> }
>>
>> /*
>> + * Command handler for the DAVLockDiscovery directive, which is TAKE1.
>> + */
>> +static const char *dav_cmd_davlockdiscovery(cmd_parms *cmd, void *config,
>> + const char *arg)
>> +{
>> + dav_dir_conf *conf = (dav_dir_conf *)config;
>> +
>> + if (strncasecmp(arg, "expr=", 5) == 0) {
>> + const char *err;
>> + if ((arg[5] == '\0'))
>> + return "missing condition";
>> + conf->allow_lockdiscovery = ap_expr_parse_cmd(cmd, &arg[5],
>> + AP_EXPR_FLAG_DONT_VARY,
>> + &err, NULL);
>> + if (err)
>> + return err;
>> + } else {
>> + return "error in condition clause";
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +/*
>> * Command handler for DAVMinTimeout directive, which is TAKE1
>> */
>> static const char *dav_cmd_davmintimeout(cmd_parms *cmd, void *config,
>> @@ -1450,7 +1477,7 @@ static dav_error *dav_gen_supported_live
>> }
>>
>> /* open the property database (readonly) for the resource */
>> - if ((err = dav_open_propdb(r, lockdb, resource, 1, NULL,
>> + if ((err = dav_open_propdb(r, lockdb, resource, DAV_PROPDB_RO, NULL,
>> &propdb)) != NULL) {
>> if (lockdb != NULL)
>> (*lockdb->hooks->close_lockdb)(lockdb);
>> @@ -2045,6 +2072,8 @@ static void dav_cache_badprops(dav_walke
>> static dav_error * dav_propfind_walker(dav_walk_resource *wres, int calltype)
>> {
>> dav_walker_ctx *ctx = wres->walk_ctx;
>> + dav_dir_conf *conf;
>> + int flags = DAV_PROPDB_RO;
>> dav_error *err;
>> dav_propdb *propdb;
>> dav_get_props_result propstats = { 0 };
>> @@ -2068,6 +2097,20 @@ static dav_error * dav_propfind_walker(d
>> return NULL;
>> }
>>
>> + conf = ap_get_module_config(ctx->r->per_dir_config, &dav_module);
>> + if (conf && conf->allow_lockdiscovery) {
>> + const char *errstr = NULL;
>> + int eval = ap_expr_exec(ctx->r, conf->allow_lockdiscovery, &errstr);
>> + if (errstr) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(00623)
>> + "Failed to evaluate expression (%s) - ignoring",
>> + errstr);
>> + } else {
>> + if (!eval)
>> + flags |= DAV_PROPDB_DISABLE_LOCKDISCOVERY;
>> + }
>> + }
>> +
>> /*
>> ** Note: ctx->doc can only be NULL for DAV_PROPFIND_IS_ALLPROP. Since
>> ** dav_get_allprops() does not need to do namespace translation,
>> @@ -2077,7 +2120,7 @@ static dav_error * dav_propfind_walker(d
>> ** the resource, however, since we are opening readonly.
>> */
>> err = dav_popen_propdb(ctx->scratchpool,
>> - ctx->r, ctx->w.lockdb, wres->resource, 1,
>> + ctx->r, ctx->w.lockdb, wres->resource, flags,
>> ctx->doc ? ctx->doc->namespaces : NULL, &propdb);
>> if (err != NULL) {
>> /* ### do something with err! */
>> @@ -2463,7 +2506,8 @@ static int dav_method_proppatch(request_
>> return dav_handle_err(r, err, NULL);
>> }
>>
>> - if ((err = dav_open_propdb(r, NULL, resource, 0, doc->namespaces,
>> + if ((err = dav_open_propdb(r, NULL, resource,
>> + DAV_PROPDB_NONE, doc->namespaces,
>> &propdb)) != NULL) {
>> /* undo any auto-checkout */
>> dav_auto_checkin(r, resource, 1 /*undo*/, 0 /*unlock*/, &av_info);
>> @@ -5220,6 +5264,11 @@ static const command_rec dav_cmds[] =
>> ACCESS_CONF|RSRC_CONF,
>> "allow Depth infinity PROPFIND requests"),
>>
>> + /* per directory/location, or per server */
>> + AP_INIT_TAKE1("DAVLockDiscovery", dav_cmd_davlockdiscovery, NULL,
>> + ACCESS_CONF|RSRC_CONF,
>> + "allow lock discovery by PROPFIND requests"),
>> +
>> { NULL }
>> };
>>
>>
>> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.h
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.h?rev=1904638&r1=1904637&r2=1904638&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/dav/main/mod_dav.h (original)
>> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.h Mon Oct 17 09:48:11 2022
>> @@ -1691,12 +1691,15 @@ struct dav_hooks_locks
>>
>> typedef struct dav_propdb dav_propdb;
>>
>> +#define DAV_PROPDB_NONE 0
>> +#define DAV_PROPDB_RO 1
>> +#define DAV_PROPDB_DISABLE_LOCKDISCOVERY 2
>>
>> DAV_DECLARE(dav_error *) dav_open_propdb(
>> request_rec *r,
>> dav_lockdb *lockdb,
>> const dav_resource *resource,
>> - int ro,
>> + int flags,
>> apr_array_header_t *ns_xlate,
>> dav_propdb **propdb);
>>
>> @@ -1705,7 +1708,7 @@ DAV_DECLARE(dav_error *) dav_popen_propd
>> request_rec *r,
>> dav_lockdb *lockdb,
>> const dav_resource *resource,
>> - int ro,
>> + int flags,
>> apr_array_header_t *ns_xlate,
>> dav_propdb **propdb);
>>
>>
>> Modified: httpd/httpd/trunk/modules/dav/main/props.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/props.c?rev=1904638&r1=1904637&r2=1904638&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/dav/main/props.c (original)
>> +++ httpd/httpd/trunk/modules/dav/main/props.c Mon Oct 17 09:48:11 2022
>> @@ -185,6 +185,8 @@ struct dav_propdb {
>>
>> dav_buffer wb_lock; /* work buffer for lockdiscovery property */
>>
>> + int flags; /* ro, disable lock discovery */
>> +
>> /* if we ever run a GET subreq, it will be stored here */
>> request_rec *subreq;
>>
>> @@ -351,6 +353,11 @@ static dav_error * dav_insert_coreprop(d
>> switch (propid) {
>>
>> case DAV_PROPID_CORE_lockdiscovery:
>> + if (propdb->flags & DAV_PROPDB_DISABLE_LOCKDISCOVERY) {
>> + value = "";
>> + break;
>> + }
>> +
>> if (propdb->lockdb != NULL) {
>> dav_lock *locks;
>>
>> @@ -522,17 +529,18 @@ static dav_error *dav_really_open_db(dav
>>
>> DAV_DECLARE(dav_error *)dav_open_propdb(request_rec *r, dav_lockdb *lockdb,
>> const dav_resource *resource,
>> - int ro,
>> + int flags,
>> apr_array_header_t * ns_xlate,
>> dav_propdb **p_propdb)
>> {
>> - return dav_popen_propdb(r->pool, r, lockdb, resource, ro, ns_xlate, p_propdb);
>> + return dav_popen_propdb(r->pool, r, lockdb, resource,
>> + flags, ns_xlate, p_propdb);
>> }
>>
>> DAV_DECLARE(dav_error *)dav_popen_propdb(apr_pool_t *p,
>> request_rec *r, dav_lockdb *lockdb,
>> const dav_resource *resource,
>> - int ro,
>> + int flags,
>> apr_array_header_t * ns_xlate,
>> dav_propdb **p_propdb)
>> {
>> @@ -559,6 +567,8 @@ DAV_DECLARE(dav_error *)dav_popen_propdb
>>
>> propdb->lockdb = lockdb;
>>
>> + propdb->flags = flags;
>> +
>> /* always defer actual open, to avoid expense of accessing db
>> * when only live properties are involved
>> */
>>
>>