Mailing List Archive

Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c
On 10/17/22 11: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.

Why do we need to use an Apache expression here? Wouldn't it be sufficient to have
DAVLockDiscovery as a flag (On/Off) with default to On that can be placed in an
<If > block if expressions are needed?

Regards

RĂ¼diger
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
Did you run any tests to observe the alleged contention?

The dbm database is very fast. I'd be surprised that contention occurs in
any typical workload.

Cheers,
-g


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
> */
>
>
>
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
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
>> */
>>
>>
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Mon, Oct 17, 2022 at 12:05 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 10/17/22 11:48 AM, ylavic@apache.org wrote:
> >
> > 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).
>
> Why do we need to use an Apache expression here? Wouldn't it be sufficient to have
> DAVLockDiscovery as a flag (On/Off) with default to On that can be placed in an
> <If > block if expressions are needed?

Yes, that'd work equally and is probably more inline with other directives.
I can change it that way after we're done discussing the suitability
of the patch..


Regards;
Yann.
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Mon, Oct 17, 2022 at 05:38:37AM -0500, Greg Stein wrote:
> Did you run any tests to observe the alleged contention?

I was the victim of it, with a server showing processes awaiting
for fcntl() to give a lock on DAVLockDB, and users complaining
anything takes ages.

> The dbm database is very fast. I'd be surprised that contention occurs in
> any typical workload.

dbm is fast once you have it open. mod_dav_fs opens DAVLockDB on each
HTTP request, then it acquire a filesystem level lock on it. This is
where contenton occurs.


--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Mon, Oct 17, 2022 at 12:04:55PM +0200, Ruediger Pluem wrote:
> Why do we need to use an Apache expression here? Wouldn't it be sufficient to have
> DAVLockDiscovery as a flag (On/Off) with default to On that can be placed in an
> <If > block if expressions are needed?

Yes, that would be fine too. I was too focused on a specific client's client
behavior that expr on User-Agent and remote IP seemed critical to me,
but indeed <If> acheive the same result.



--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Mon, Oct 17, 2022 at 12:04:55PM +0200, Ruediger Pluem wrote:
> Why do we need to use an Apache expression here? Wouldn't it be sufficient to have
> DAVLockDiscovery as a flag (On/Off)

I posted a patch for that change, along with documentation, on
https://bz.apache.org/bugzilla/show_bug.cgi?id=66313

Is it fine for you?

--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Tue, Oct 18, 2022 at 05:03:48PM +0000, Emmanuel Dreyfus wrote:
> dbm is fast once you have it open. mod_dav_fs opens DAVLockDB on each
> HTTP request, then it acquire a filesystem level lock on it. This is
> where contenton occurs.

I have been thinking about how Apache could open DAVLockDB once, instead
of for each HTTP request. The workers should share a file descriptor
on the file, and a mutex to avoid concurent access.

That does not fit well with the prefork model. Opending DAVLockDB
and creating the mutex (a sysV mutex?) should be done in the master
process. Should it be done when processing the configuration directive?

We would also need to take care of closing the previous file descriptor on
reloads.


--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On 10/26/22 8:51 PM, Emmanuel Dreyfus wrote:
> On Mon, Oct 17, 2022 at 12:04:55PM +0200, Ruediger Pluem wrote:
>> Why do we need to use an Apache expression here? Wouldn't it be sufficient to have
>> DAVLockDiscovery as a flag (On/Off)
>
> I posted a patch for that change, along with documentation, on
> https://bz.apache.org/bugzilla/show_bug.cgi?id=66313
>
> Is it fine for you?
>


Looks good. Thanks.

Regards

RĂ¼diger
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Wed, Oct 26, 2022 at 1:59 PM Emmanuel Dreyfus <manu@netbsd.org> wrote:

> On Tue, Oct 18, 2022 at 05:03:48PM +0000, Emmanuel Dreyfus wrote:
> > dbm is fast once you have it open. mod_dav_fs opens DAVLockDB on each
> > HTTP request, then it acquire a filesystem level lock on it. This is
> > where contenton occurs.
>

Gotcha. Yes, that makes total sense.


> I have been thinking about how Apache could open DAVLockDB once, instead
> of for each HTTP request. The workers should share a file descriptor
> on the file, and a mutex to avoid concurent access.
>
> That does not fit well with the prefork model. Opending DAVLockDB
> and creating the mutex (a sysV mutex?) should be done in the master
> process. Should it be done when processing the configuration directive?
>
> We would also need to take care of closing the previous file descriptor on
> reloads.
>

That's gonna get very tricky across the various MPMs. dbm wasn't really
designed for this.

When I wrote mod_dav way back when, SQLite was not available. That is where
I'd go today for the lock database (and properties). It handles
multi-process, multi-thread, and is very efficient. If you are sufficiently
motivated, that would be an excellent path forward.

With that said, I'm not a fan of [DAV or svn] locks. Anything that can be
done to avoid a workflow that encompasses locks would be ideal.

Cheers,
-g
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Thu, Oct 27, 2022 at 01:58:58AM -0500, Greg Stein wrote:
> With that said, I'm not a fan of [DAV or svn] locks. Anything that can be
> done to avoid a workflow that encompasses locks would be ideal.

For DAV filesystem, we cannot spare locks when clients use LOCK/UNLOCK
methods. Lock discovery by PROPFIND is another story, I cannot see
a use case for that.

If you want to see less locks, then you must be great fan of my
DAVLockDiscovery contribution, especially now it has been updated
as a flag directive. Any chance we get it into 2.4 branch?

--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1904638 - in /httpd/httpd/trunk: changes-entries/DAVLockDiscovery.txt modules/dav/main/mod_dav.c modules/dav/main/mod_dav.h modules/dav/main/props.c [ In reply to ]
On Thu, Oct 27, 2022 at 8:22 AM Emmanuel Dreyfus <manu@netbsd.org> wrote:

> On Thu, Oct 27, 2022 at 01:58:58AM -0500, Greg Stein wrote:
> > With that said, I'm not a fan of [DAV or svn] locks. Anything that can be
> > done to avoid a workflow that encompasses locks would be ideal.
>
> For DAV filesystem, we cannot spare locks when clients use LOCK/UNLOCK
> methods. Lock discovery by PROPFIND is another story, I cannot see
> a use case for that.


Agree.


> If you want to see less locks, then you must be great fan of my
> DAVLockDiscovery contribution, especially now it has been updated
> as a flag directive.


Yeup!


> Any chance we get it into 2.4 branch?
>

I'll let others throw in on this. I haven't worked on httpd for quite a
long time.

Cheers,
-g