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 Mon, Oct 17, 2022 at 12:05 PM Ruediger Pluem wrote: > > On 10/17/22 11:48 AM, yla...@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 > 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
On Mon, Oct 17, 2022 at 12:39 PM Greg Stein 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 wrote:
>>
>> Author: ylavic
>> Date: Mon Oct 17 09:48:11 2022
>> New Revision: 1904638
>>
>> URL: http://svn.apache.org/viewvc?rev=1904638=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
>> 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=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 > 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=1904637=1904638=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, [5],
>> +
>> AP_EXPR_FLAG_DONT_VARY,
>> + , 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,
>> )) != NULL) {
>> if (lockdb != NULL)
>>
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
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 wrote:
> Author: ylavic
> Date: Mon Oct 17 09:48:11 2022
> New Revision: 1904638
>
> URL: http://svn.apache.org/viewvc?rev=1904638=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
> 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=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 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=1904637=1904638=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, [5],
> +
> AP_EXPR_FLAG_DONT_VARY,
> + , 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,
> )) != 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 @@
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, yla...@apache.org wrote: > Author: ylavic > Date: Mon Oct 17 09:48:11 2022 > New Revision: 1904638 > > URL: http://svn.apache.org/viewvc?rev=1904638=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 block if expressions are needed? Regards Rüdiger
Re: mod_dav_fs performances
On Mon, Oct 17, 2022 at 9:20 AM Emmanuel Dreyfus wrote: > > I submitted a proposed fix in > https://bz.apache.org/bugzilla/show_bug.cgi?id=66313 I Thanks for this, checked in trunk (r1904638). Regards; Yann.
mod_dav_fs performances
Hello I have been badly hit by a performance problem in mod_dav_fs. After a few users updated to (RaiDrive/2022.6.56.0), all users reported terrible performance, with files taking age to open. Investigating the problem, we discovered lock contention of the DavLockDB file. Too many clients looking up locks, too often. A log analysis show that RaiDrive/2022.6.56.0 does much more PROPFIND than other clients. PROPFIND involves lock discovery, and lock discovery needs serialized access to DavLockDB. I submitted a proposed fix in https://bz.apache.org/bugzilla/show_bug.cgi?id=66313 I This introduces a DAVLockDiscovery option to optionaly disable lock discovery. It uses an apache expession so that the thing can be fine tuned, e.g. disabling it for specific UserAgent that exhibit an abusive behavior. When lock discovery is disabled, PROPFIND just returns an empty lock discovery section. I have been testing disabling lock discovery for a week, it works well. Performances are good again, and no regression appeared. -- Emmanuel Dreyfus m...@netbsd.org
