You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2018/05/03 19:14:31 UTC
Re: svn commit: r1830800 - in /httpd/httpd/trunk: CHANGES
modules/slotmem/mod_slotmem_shm.c
I seem to be having issues with this patch via this scenario:
o Using the balancer-manager, make changes.
o Do a grace restart some times.
o Make more changes.
o Now shutdown.
o Now restart. The changes are erased...
It looks like maybe something to do with the generation number... ??
> On May 3, 2018, at 4:32 AM, ylavic@apache.org wrote:
>
> Author: ylavic
> Date: Thu May 3 08:32:42 2018
> New Revision: 1830800
>
> URL: http://svn.apache.org/viewvc?rev=1830800&view=rev
> Log:
> mod_slomem_shm: Handle a generation number when the slotmem size changes.
>
> Modifying the number of proxy balancers or balancer members on restart
> could have prevented the server to load, notably on Windows. PR 62308.
>
> The generation number integrated in the SHM filename allows to create a
> new/resized SHM while the previous is still in use by previous generation
> gracefully shutting down (Windows prevents SHM/file to be removed in this
> case, but even on Unix(es) an unlinked file might not be re-openable while
> an inode exists). The generation number is added/incremented only if the
> size requirement changed, such that unrelated restarts continue to share
> SHMs between generations.
>
> The cleanup handling is also simplified because both the parent process and
> the Windows child process need to cleanup everything on exit. This translates
> to cleanup_slotmem() being always registered but in the dry load state
> (AP_SQ_MS_CREATE_PRE_CONFIG), for both cases still.
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1830800&r1=1830799&r2=1830800&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Thu May 3 08:32:42 2018
> @@ -1,6 +1,11 @@
> -*- coding: utf-8 -*-
> Changes with Apache 2.5.1
>
> + *) mod_slomem_shm: Handle a generation number when the slotmem size changes,
> + modifying the number of proxy balancers or balancer members on restart
> + could have prevented the server to load, notably on Windows. PR 62308.
> + [Yann Ylavic]
> +
> *) mod_proxy_html: Fix variable interpolation and memory allocation failure
> in ProxyHTMLURLMap. [Ewald Dieterich <ewald mailbox.org>]
>
>
> Modified: httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c?rev=1830800&r1=1830799&r2=1830800&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c (original)
> +++ httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c Thu May 3 08:32:42 2018
> @@ -26,6 +26,7 @@
> #include "httpd.h"
> #include "http_main.h"
> #include "http_core.h"
> +#include "ap_mpm.h"
>
> #define AP_SLOTMEM_IS_PREGRAB(t) (t->desc->type & AP_SLOTMEM_TYPE_PREGRAB)
> #define AP_SLOTMEM_IS_PERSIST(t) (t->desc->type & AP_SLOTMEM_TYPE_PERSIST)
> @@ -42,7 +43,8 @@ typedef struct {
> #define AP_UNSIGNEDINT_OFFSET (APR_ALIGN_DEFAULT(sizeof(unsigned int)))
>
> struct ap_slotmem_instance_t {
> - char *name; /* file based SHM path/name */
> + char *name; /* file based SHM name (immutable) */
> + char *fname; /* file based SHM path/name */
> char *pname; /* persisted file path/name */
> int fbased; /* filebased? */
> void *shm; /* ptr to memory segment (apr_shm_t *) */
> @@ -287,41 +289,37 @@ static APR_INLINE int is_child_process(v
> #endif
> }
>
> -static apr_status_t cleanup_slotmem(void *is_startup)
> +static apr_status_t cleanup_slotmem(void *nil)
> {
> int is_exiting = (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_EXITING);
> + int is_child = is_child_process();
> ap_slotmem_instance_t *mem;
>
> - /* When restarting or stopping we want to flush persisted data, and in the
> - * stopping case we also want to unlink the SHMs. On the first (dry-)load
> - * though (is_startup != NULL), the retained list is not used yet and SHMs
> - * contents were untouched (so they don't need to be persisted); unlink
> - * everything before the real loading (from scratch).
> + /* When restarting or stopping we want to flush persisted data, and in
> + * the stopping case we also want to unlink the SHMs. A MPM winnt child
> + * process is always exiting here, we don't want it to care about persisted
> + * data but it still has to (try to) cleanup SHMs since it may be the last
> + * one to use them, and Windows prevent anything but the last user to
> + * effectively destroy/remove an open SHM/file.
> */
> for (mem = globallistmem; mem; mem = mem->next) {
> - int unlink;
> - if (is_startup) {
> - unlink = mem->fbased;
> + if (AP_SLOTMEM_IS_PERSIST(mem) && !is_child) {
> + store_slotmem(mem);
> }
> - else {
> - if (AP_SLOTMEM_IS_PERSIST(mem)) {
> - store_slotmem(mem);
> - }
> - unlink = is_exiting;
> - }
> - if (unlink) {
> - /* Some systems may require the descriptor to be closed before
> - * unlink, thus call destroy() first.
> + if (is_exiting) {
> + /* Some systems require the descriptor to be closed before
> + * unlinked, thus call destroy() first.
> */
> apr_shm_destroy(mem->shm);
> - apr_shm_remove(mem->name, mem->pool);
> + apr_shm_remove(mem->fname, mem->pool);
> }
> }
>
> - if (is_startup || is_exiting) {
> + if (is_exiting) {
> *retained_globallistmem = NULL;
> }
> else {
> + /* Restarting, save list for next run */
> *retained_globallistmem = globallistmem;
> }
>
> @@ -361,25 +359,48 @@ static int check_slotmem(ap_slotmem_inst
>
> /* check size */
> if (apr_shm_size_get(mem->shm) != size) {
> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, ap_server_conf, APLOGNO(02599)
> - "existing shared memory for %s could not be used "
> + ap_log_error(APLOG_MARK, APLOG_INFO, 0, ap_server_conf, APLOGNO(02599)
> + "existing shared memory for %s could not be reused "
> "(failed size check)",
> - mem->name);
> + mem->fname);
> return 0;
> }
>
> desc = apr_shm_baseaddr_get(mem->shm);
> if (desc->size != item_size || desc->num != item_num) {
> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, ap_server_conf, APLOGNO(02600)
> - "existing shared memory for %s could not be used "
> + ap_log_error(APLOG_MARK, APLOG_INFO, 0, ap_server_conf, APLOGNO(02600)
> + "existing shared memory for %s could not be reused "
> "(failed contents check)",
> - mem->name);
> + mem->fname);
> return 0;
> }
>
> return 1;
> }
>
> +static apr_status_t slotmem_attach_shm(apr_shm_t **shm, const char **fname,
> + apr_uint32_t num, apr_pool_t *pool)
> +{
> + char *name = NULL;
> + apr_size_t size = 0, offset = 0;
> +
> + for (; num; --num) {
> + if (!name) {
> + name = apr_psprintf(pool, "%s.%u", *fname, APR_UINT32_MAX);
> + offset = strlen(*fname) + 1;
> + size = offset + strlen(name + offset) + 1;
> + }
> + apr_snprintf(name + offset, size - offset, "%u", num);
> +
> + if (apr_shm_attach(shm, name, pool) == APR_SUCCESS) {
> + *fname = name;
> + return APR_SUCCESS;
> + }
> + }
> +
> + return apr_shm_attach(shm, *fname, pool);
> +}
> +
> static apr_status_t slotmem_create(ap_slotmem_instance_t **new,
> const char *name, apr_size_t item_size,
> unsigned int item_num,
> @@ -397,20 +418,33 @@ static apr_status_t slotmem_create(ap_sl
> apr_size_t size = AP_SLOTMEM_OFFSET + AP_UNSIGNEDINT_OFFSET +
> (item_num * sizeof(char)) + basesize;
> int persist = (type & AP_SLOTMEM_TYPE_PERSIST) != 0;
> + int generation = 0;
> apr_status_t rv;
> apr_pool_t *p;
>
> *new = NULL;
> + ap_mpm_query(AP_MPMQ_GENERATION, &generation);
>
> if (slotmem_filenames(pool, name, &fname, persist ? &pname : NULL)) {
> /* first try to attach to existing slotmem */
> if (next) {
> ap_slotmem_instance_t *prev = NULL;
> for (;;) {
> - if (strcmp(next->name, fname) == 0) {
> + if (strcmp(next->name, name) == 0) {
> *new = next; /* either returned here or reused finally */
> if (!check_slotmem(next, size, item_size, item_num)) {
> + /* Can't reuse this SHM, a new one is needed with its
> + * own filename (including generation number) because
> + * the previous one may still be used by the previous
> + * generation. Persisted file (if any) can't be reused
> + * either.
> + */
> apr_shm_destroy(next->shm);
> + apr_shm_remove(next->fname, pool);
> + fname = apr_psprintf(pool, "%s.%u", fname,
> + (apr_uint32_t)generation);
> + persist = 0;
> +
> next = next->next;
> if (prev) {
> prev->next = next;
> @@ -426,7 +460,7 @@ static apr_status_t slotmem_create(ap_sl
> }
> /* we already have it */
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02603)
> - "create found %s in global list", fname);
> + "create found %s in global list", next->fname);
> return APR_SUCCESS;
> }
> if (!next->next) {
> @@ -437,7 +471,7 @@ static apr_status_t slotmem_create(ap_sl
> }
> }
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02602)
> - "create didn't find %s in global list", fname);
> + "create didn't find %s in global list", name);
> }
> else {
> fbased = 0;
> @@ -446,8 +480,7 @@ static apr_status_t slotmem_create(ap_sl
>
> /* first try to attach to existing shared memory */
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02300)
> - "create %s: %"APR_SIZE_T_FMT"/%u", fname, item_size,
> - item_num);
> + "create %s: %"APR_SIZE_T_FMT"/%u", name, item_size, item_num);
>
> {
> if (fbased) {
> @@ -456,7 +489,7 @@ static apr_status_t slotmem_create(ap_sl
> * parent exist in the children already; attach them.
> */
> if (is_child_process()) {
> - rv = apr_shm_attach(&shm, fname, gpool);
> + rv = slotmem_attach_shm(&shm, &fname, generation, gpool);
> }
> else {
> apr_shm_remove(fname, pool);
> @@ -509,10 +542,11 @@ static apr_status_t slotmem_create(ap_sl
> res = *new;
> if (res == NULL) {
> res = apr_pcalloc(p, sizeof(ap_slotmem_instance_t));
> - res->name = apr_pstrdup(p, fname);
> + res->name = apr_pstrdup(p, name);
> res->pname = apr_pstrdup(p, pname);
> *new = res;
> }
> + res->fname = apr_pstrdup(p, fname);
> res->fbased = fbased;
> res->shm = shm;
> res->persist = (void *)ptr;
> @@ -547,29 +581,33 @@ static apr_status_t slotmem_attach(ap_sl
> ap_slotmem_instance_t *res;
> ap_slotmem_instance_t *next = globallistmem;
> sharedslotdesc_t *desc;
> + int generation = 0;
> const char *fname;
> apr_shm_t *shm;
> apr_status_t rv;
>
> + *new = NULL;
> + ap_mpm_query(AP_MPMQ_GENERATION, &generation);
> +
> if (!slotmem_filenames(pool, name, &fname, NULL)) {
> return APR_ENOSHMAVAIL;
> }
>
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02301)
> - "attach looking for %s", fname);
> + "attach looking for %s", name);
>
> /* first try to attach to existing slotmem */
> if (next) {
> for (;;) {
> - if (strcmp(next->name, fname) == 0) {
> + if (strcmp(next->name, name) == 0) {
> /* we already have it */
> *new = next;
> *item_size = next->desc->size;
> *item_num = next->desc->num;
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
> APLOGNO(02302)
> - "attach found %s: %"APR_SIZE_T_FMT"/%u", fname,
> - *item_size, *item_num);
> + "attach found %s: %"APR_SIZE_T_FMT"/%u",
> + next->fname, *item_size, *item_num);
> return APR_SUCCESS;
> }
> if (!next->next) {
> @@ -580,7 +618,7 @@ static apr_status_t slotmem_attach(ap_sl
> }
>
> /* next try to attach to existing shared memory */
> - rv = apr_shm_attach(&shm, fname, pool);
> + rv = slotmem_attach_shm(&shm, &fname, generation, pool);
> if (rv != APR_SUCCESS) {
> return rv;
> }
> @@ -591,7 +629,8 @@ static apr_status_t slotmem_attach(ap_sl
>
> /* For the chained slotmem stuff */
> res = apr_pcalloc(pool, sizeof(ap_slotmem_instance_t));
> - res->name = apr_pstrdup(pool, fname);
> + res->name = apr_pstrdup(pool, name);
> + res->fname = apr_pstrdup(pool, fname);
> res->fbased = 1;
> res->shm = shm;
> res->persist = (void *)ptr;
> @@ -608,8 +647,8 @@ static apr_status_t slotmem_attach(ap_sl
> *item_num = desc->num;
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
> APLOGNO(02303)
> - "attach found %s: %"APR_SIZE_T_FMT"/%u", fname,
> - *item_size, *item_num);
> + "attach found %s: %"APR_SIZE_T_FMT"/%u",
> + res->fname, *item_size, *item_num);
> return APR_SUCCESS;
> }
>
> @@ -821,7 +860,6 @@ static const ap_slotmem_provider_t *slot
> static int pre_config(apr_pool_t *pconf, apr_pool_t *plog,
> apr_pool_t *ptemp)
> {
> - void *is_startup = NULL;
> const char *retained_key = "mod_slotmem_shm";
>
> retained_globallistmem = ap_retained_data_get(retained_key);
> @@ -830,30 +868,23 @@ static int pre_config(apr_pool_t *pconf,
> ap_retained_data_create(retained_key,
> sizeof *retained_globallistmem);
> }
> - globallistmem = *retained_globallistmem;
>
> - if (!is_child_process()) {
> - /* For the first (dry-)load, create SHMs on pconf and let them be
> - * cleaned up before the real loading. Otherwise SHMs shall have the
> - * same lifetime as the retained list (ap_pglobal). The cleanup is to
> - * update the retained list on restart, or to unlink the SHMs on stop
> - * or after the first (dry-)load (is_startup != NULL).
> - */
> - if (ap_state_query(AP_SQ_MAIN_STATE) != AP_SQ_MS_CREATE_PRE_CONFIG) {
> - gpool = ap_pglobal;
> - }
> - else {
> - is_startup = (void *)1;
> - gpool = pconf;
> - }
> -
> - apr_pool_cleanup_register(pconf, is_startup, cleanup_slotmem,
> + /* For the first (dry-)load, create SHMs on pconf and let them be
> + * cleaned up with it before the real loading. In any other case, SHMs
> + * shall have the same lifetime as the retained list (ap_pglobal), so
> + * the cleanup is to update the retained list on restart, and to unlink
> + * the SHMs on stop. This also works for MPM winnt child process which
> + * should rebuild the list first (i.e. attach SHMs, see slotmem_create),
> + * and try to cleanup/remove SHMs before exiting (see cleanup_slotmem).
> + */
> + if (ap_state_query(AP_SQ_MAIN_STATE) != AP_SQ_MS_CREATE_PRE_CONFIG) {
> + apr_pool_cleanup_register(pconf, NULL, cleanup_slotmem,
> apr_pool_cleanup_null);
> + globallistmem = *retained_globallistmem;
> + gpool = ap_pglobal;
> }
> else {
> - /* For MPMs which (re-)run pre_config we don't need to retain slotmems
> - * in children, so use pconf and let it detach SHMs when children exit.
> - */
> + globallistmem = NULL;
> gpool = pconf;
> }
>
>
>
Re: svn commit: r1830800 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, May 7, 2018 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Hi Jim,
>
> thanks for testing!
>
> I tried your scenario (if understood correctly, see below), and it
> seems to work for me.
>
>> I seem to be having issues with this patch via this scenario:
>>
>> o Using the balancer-manager, make changes.
>
> BalancerPersist on, right?
> Also, I made changes to balancers/members parameters only, since there
> doesn't seem to have a way to add balancers or members via the UI. Is
> there a way or do you have scripts/cURLs scenario to do this?
I could add balancer members by setting the growth= parameter
appropriately in the first place, and the changes were preserved too
after shutdown.
>
>> o Do a grace restart some times.
>> o Make more changes.
>
> I made changes through balancer-manager still, I think configuration
> changes could indeed reset manager changes, but that's expected no (I
> think it worked like this before already)?
>
>> o Now shutdown.
>> o Now restart. The changes are erased...
>>
>> It looks like maybe something to do with the generation number... ??
>
> Persisted files don't have a generation number, so it should work.. At
> least it worked for me with parameters changes only, same parameters
> values before/after shutdown/restart.
>
> Possibly you could elaborate a bit on the kind of changes issued in
> between so that I can reproduce?
>
>
> Regards,
> Yann.
Re: svn commit: r1830800 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Jim,
thanks for testing!
I tried your scenario (if understood correctly, see below), and it
seems to work for me.
> I seem to be having issues with this patch via this scenario:
>
> o Using the balancer-manager, make changes.
BalancerPersist on, right?
Also, I made changes to balancers/members parameters only, since there
doesn't seem to have a way to add balancers or members via the UI. Is
there a way or do you have scripts/cURLs scenario to do this?
> o Do a grace restart some times.
> o Make more changes.
I made changes through balancer-manager still, I think configuration
changes could indeed reset manager changes, but that's expected no (I
think it worked like this before already)?
> o Now shutdown.
> o Now restart. The changes are erased...
>
> It looks like maybe something to do with the generation number... ??
Persisted files don't have a generation number, so it should work.. At
least it worked for me with parameters changes only, same parameters
values before/after shutdown/restart.
Possibly you could elaborate a bit on the kind of changes issued in
between so that I can reproduce?
Regards,
Yann.