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 Thu, Oct 27, 2022 at 8:22 AM Emmanuel Dreyfus 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
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 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 m...@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
On Wed, Oct 26, 2022 at 1:59 PM Emmanuel Dreyfus wrote: > On Tue, Oct 18, 2022 at 05:03:48PM +, 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
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
On Tue, Oct 18, 2022 at 05:03:48PM +, 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 m...@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
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 m...@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
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 > 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 acheive the same result. -- Emmanuel Dreyfus m...@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
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 m...@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
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&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
>> 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 > 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) {
>>
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&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
> 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 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
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&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 block if expressions are needed? Regards Rüdiger
