Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Fri, Jan 26, 2018 at 8:49 PM,  <ji...@apache.org> wrote:
> Author: jim
> Date: Fri Jan 26 19:49:04 2018
> New Revision: 1822341
>
> URL: http://svn.apache.org/viewvc?rev=1822341&view=rev
> Log:
> PR 62044: Force addition of generation number to shm filename on
> all platforms. Keep persisted filename as-was.
>
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
[]
>
> 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=1822341&r1=1822340&r2=1822341&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c (original)
> +++ httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c Fri Jan 26 19:49:04 2018
> @@ -71,31 +71,6 @@ static apr_pool_t *gpool = NULL;
>  #define DEFAULT_SLOTMEM_SUFFIX ".shm"
>  #define DEFAULT_SLOTMEM_PERSIST_SUFFIX ".persist"
>
> -/* Unixes (and Netware) have the unlink() semantic, which allows to
> - * apr_file_remove() a file still in use (opened elsewhere), the inode
> - * remains until the last fd is closed, whereas any file created with
> - * the same name/path will use a new inode.
> - *
> - * On windows and OS/2 ("\SHAREMEM\..." tree), apr_file_remove() marks
> - * the files for deletion until the last HANDLE is closed, meanwhile the
> - * same file/path can't be opened/recreated.
> - * Thus on graceful restart (the only restart mode with mpm_winnt), the
> - * old file may still exist until all the children stop, while we ought
> - * to create a new one for our new clear SHM.  Therefore, we would only
> - * be able to reuse (attach) the old SHM, preventing some changes to
> - * the config file, like the number of balancers/members, since the
> - * size checks (to fit the new config) would fail.  Let's avoid this by
> - * including the generation number in the SHM filename (obviously not
> - * the persisted name!)
> - */
> -#ifndef SLOTMEM_UNLINK_SEMANTIC
> -#if defined(WIN32) || defined(OS2)
> -#define SLOTMEM_UNLINK_SEMANTIC 0
> -#else
> -#define SLOTMEM_UNLINK_SEMANTIC 1
> -#endif
> -#endif

The "unlink semantic" is broken, I must have been on drugs when I wrote this :)

But this change (r1822341) means that SHMs are not reused on graceful
restart, unless persisted.
This was already the case for non-unix, nothing wrong with this
commit, but it was not really friendly in the first place...

So I wonder if we'd better:
1. have a constant file name (no generation suffix) for all systems,
2. not unlink/remove the SHM file on pconf cleanup,
2'. eventually unlink the ones unused in ++generation (e.g. retained
global list),
3. unlink all on pglobal cleanup.

Now we'd have a working shm_attach() not only for persisted slotmems,
while shm_create() would only be called for new SHMs which we know can
be pre-removed (to work around crashes leaving them registered
somewhere).
Also, if attach succeeds on startup (first generation) but the SHM is
not persisted (an old crash still), we can possibly pass the sizes
checks and re-create the SHM regardless.

WDYT?

PS: 2' may not be suitable in all cases since we could want to allow
transient disable of a slotmem (for some reason), while some next
graceful would re-enable it. We probably can let 3 alone for cleanup.

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Sat, Jan 27, 2018 at 5:29 PM, Mark Blackman <ma...@exonetric.com> wrote:
>
> On 27 Jan 2018, at 15:55, Yann Ylavic <yl...@gmail.com> wrote:
>
>> OK, v2 already, with correct usage of gpool (i.e. pconf), distinct
>> from ap_pglobal.
>> <mod_slotmem_shm-v2.patch>
>
> Thanks, do you recommend we test this version in our pre-production
> environments?

No of course, found some mistakes already.
I'll post something on the PR after some testing (none so far...)

Regards,
Yann.

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Mark Blackman <ma...@exonetric.com>]

> On 27 Jan 2018, at 15:55, Yann Ylavic <yl...@gmail.com> wrote:
> 
> On Sat, Jan 27, 2018 at 4:41 PM, Yann Ylavic <ylavic.dev@gmail.com <ma...@gmail.com>> wrote:
>> On Sat, Jan 27, 2018 at 4:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Sat, Jan 27, 2018 at 3:14 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>>> 
>>>> So I wonder if we'd better:
>>>> 1. have a constant file name (no generation suffix) for all systems,
>>>> 2. not unlink/remove the SHM file on pconf cleanup,
>>>> 2'. eventually unlink the ones unused in ++generation (e.g. retained
>>>> global list),
>>>> 3. unlink all on pglobal cleanup.
>>>> 
>>>> Now we'd have a working shm_attach() not only for persisted slotmems,
>>>> while shm_create() would only be called for new SHMs which we know can
>>>> be pre-removed (to work around crashes leaving them registered
>>>> somewhere).
>>>> Also, if attach succeeds on startup (first generation) but the SHM is
>>>> not persisted (an old crash still), we can possibly pass the sizes
>>>> checks and re-create the SHM regardless.
>>>> 
>>>> WDYT?
>>> 
>>> Something like the attached patch (it talks better sometimes...).
>>> Not even tested of course :)
>> 
>> Oops, more than needed in there, here is the good one.
> 
> OK, v2 already, with correct usage of gpool (i.e. pconf), distinct
> from ap_pglobal.
> <mod_slotmem_shm-v2.patch>

Thanks, do you recommend we test this version in our pre-production environments?

Regards,
Mark

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Sat, Jan 27, 2018 at 4:41 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Jan 27, 2018 at 4:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Sat, Jan 27, 2018 at 3:14 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> So I wonder if we'd better:
>>> 1. have a constant file name (no generation suffix) for all systems,
>>> 2. not unlink/remove the SHM file on pconf cleanup,
>>> 2'. eventually unlink the ones unused in ++generation (e.g. retained
>>> global list),
>>> 3. unlink all on pglobal cleanup.
>>>
>>> Now we'd have a working shm_attach() not only for persisted slotmems,
>>> while shm_create() would only be called for new SHMs which we know can
>>> be pre-removed (to work around crashes leaving them registered
>>> somewhere).
>>> Also, if attach succeeds on startup (first generation) but the SHM is
>>> not persisted (an old crash still), we can possibly pass the sizes
>>> checks and re-create the SHM regardless.
>>>
>>> WDYT?
>>
>> Something like the attached patch (it talks better sometimes...).
>> Not even tested of course :)
>
> Oops, more than needed in there, here is the good one.

OK, v2 already, with correct usage of gpool (i.e. pconf), distinct
from ap_pglobal.

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Sat, Jan 27, 2018 at 4:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Jan 27, 2018 at 3:14 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> So I wonder if we'd better:
>> 1. have a constant file name (no generation suffix) for all systems,
>> 2. not unlink/remove the SHM file on pconf cleanup,
>> 2'. eventually unlink the ones unused in ++generation (e.g. retained
>> global list),
>> 3. unlink all on pglobal cleanup.
>>
>> Now we'd have a working shm_attach() not only for persisted slotmems,
>> while shm_create() would only be called for new SHMs which we know can
>> be pre-removed (to work around crashes leaving them registered
>> somewhere).
>> Also, if attach succeeds on startup (first generation) but the SHM is
>> not persisted (an old crash still), we can possibly pass the sizes
>> checks and re-create the SHM regardless.
>>
>> WDYT?
>
> Something like the attached patch (it talks better sometimes...).
> Not even tested of course :)

Oops, more than needed in there, here is the good one.

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Sat, Jan 27, 2018 at 3:14 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> So I wonder if we'd better:
> 1. have a constant file name (no generation suffix) for all systems,
> 2. not unlink/remove the SHM file on pconf cleanup,
> 2'. eventually unlink the ones unused in ++generation (e.g. retained
> global list),
> 3. unlink all on pglobal cleanup.
>
> Now we'd have a working shm_attach() not only for persisted slotmems,
> while shm_create() would only be called for new SHMs which we know can
> be pre-removed (to work around crashes leaving them registered
> somewhere).
> Also, if attach succeeds on startup (first generation) but the SHM is
> not persisted (an old crash still), we can possibly pass the sizes
> checks and re-create the SHM regardless.
>
> WDYT?

Something like the attached patch (it talks better sometimes...).
Not even tested of course :)

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Mon, Jan 29, 2018 at 12:49 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sun, Jan 28, 2018 at 8:23 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>>
>>> On Jan 27, 2018, at 9:14 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> But this change (r1822341) means that SHMs are not reused on graceful
>>> restart, unless persisted.
>>
>> If that's the case, then that's a non-starter. Are you sure?
>
> Quite yes (tested actually), a new filename is generated at each
> restart, so there is nothing to reuse in the list nor an SHM to attach
> to. We could use the name instead of the filename to identify them in
> the list, but anyway the key is to persist the list accross restarts
> (ap_pglobal and so on...).
>
> I proposed a patch in bz (seems to work from my testing), not as light
> as I would have hoped, but not completely insane either IMHO :)
> WDYT?

Some testing on non-unixes (e.g. Windows) would be appreciated too..

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

Thanks, r1822509 seems to work for me.

On Mon, Jan 29, 2018 at 2:46 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> OK, I'll hold off on any patching since it looks like
> you are working on this.
>
>> On Jan 28, 2018, at 6:49 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Sun, Jan 28, 2018 at 8:23 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>>
>>>
>>>> On Jan 27, 2018, at 9:14 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>> But this change (r1822341) means that SHMs are not reused on graceful
>>>> restart, unless persisted.
>>>
>>> If that's the case, then that's a non-starter. Are you sure?
>>
>> Quite yes (tested actually), a new filename is generated at each
>> restart, so there is nothing to reuse in the list nor an SHM to attach
>> to. We could use the name instead of the filename to identify them in
>> the list, but anyway the key is to persist the list accross restarts
>> (ap_pglobal and so on...).
>>
>> I proposed a patch in bz (seems to work from my testing), not as light
>> as I would have hoped, but not completely insane either IMHO :)
>> WDYT?
>

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Jim Jagielski <ji...@jaguNET.com>]

OK, I'll hold off on any patching since it looks like
you are working on this.

> On Jan 28, 2018, at 6:49 PM, Yann Ylavic <yl...@gmail.com> wrote:
> 
> On Sun, Jan 28, 2018 at 8:23 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> 
>> 
>>> On Jan 27, 2018, at 9:14 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> But this change (r1822341) means that SHMs are not reused on graceful
>>> restart, unless persisted.
>> 
>> If that's the case, then that's a non-starter. Are you sure?
> 
> Quite yes (tested actually), a new filename is generated at each
> restart, so there is nothing to reuse in the list nor an SHM to attach
> to. We could use the name instead of the filename to identify them in
> the list, but anyway the key is to persist the list accross restarts
> (ap_pglobal and so on...).
> 
> I proposed a patch in bz (seems to work from my testing), not as light
> as I would have hoped, but not completely insane either IMHO :)
> WDYT?

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Yann Ylavic <yl...@gmail.com>]

On Sun, Jan 28, 2018 at 8:23 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>
>> On Jan 27, 2018, at 9:14 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> But this change (r1822341) means that SHMs are not reused on graceful
>> restart, unless persisted.
>
> If that's the case, then that's a non-starter. Are you sure?

Quite yes (tested actually), a new filename is generated at each
restart, so there is nothing to reuse in the list nor an SHM to attach
to. We could use the name instead of the filename to identify them in
the list, but anyway the key is to persist the list accross restarts
(ap_pglobal and so on...).

I proposed a patch in bz (seems to work from my testing), not as light
as I would have hoped, but not completely insane either IMHO :)
WDYT?

Re: svn commit: r1822341 - in /httpd/httpd/trunk: CHANGES modules/slotmem/mod_slotmem_shm.c

[Jim Jagielski <ji...@jaguNET.com>]

> On Jan 27, 2018, at 9:14 AM, Yann Ylavic <yl...@gmail.com> wrote:
> But this change (r1822341) means that SHMs are not reused on graceful
> restart, unless persisted.

If that's the case, then that's a non-starter. Are you sure?