On Mon, Oct 17, 2022 at 12:39 PM Greg Stein <gst...@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 <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. >> >> >> 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 >> */ >> >>
