[WIP] Fix: avoid using deprecated valloc & frequent aligned alloc #134

wenningerk · 2021-09-06T14:49:34Z

switch to using a single aligned buffer that stays allocated.

free to use allocation mechanism that doesn't allow to
use free() to hand back (not needed for current posix_memalign)
avoid memory fragmentation risk (and thus risk to run out of
hogged memory) due to frequent alloc/free of aligned memory

Switching from forking to simple should probably be
thought over again as sbd is doing quite some checks that
should be done before systemd should assume sbd as started
and thus start pacemaker.
Nothing severe should happen though since we have startup-
synchronization between the daemons meanwhile.
To leave legacy forking startup behind and still have the
behavior that pacemaker is just started by systemd when sbd
has checked config, watchdog-device & disks notify is
probably the only way.
More important is that pacemaker is atm using the sbd-pid-file to
detect a running sbd-daemon and thus assume it should
synchronize with it. More robust would probably be to ask
systemd (if compiled with systemd-support) if sbd is enabled
and it would work without a pid-file.

kgaillot

Looks good to me

kgaillot · 2021-09-07T19:16:21Z

src/sbd-md.c

@@ -108,6 +109,7 @@ open_device(const char* devname, int loglevel)

 	if (io_setup(1, &st->ioctx) != 0) {
 		cl_perror("io_setup failed");
+		free(st->buffer);


This isn't necessary since st is allocated just above here, and buf hasn't been set yet

But doesn't hurt either ;-)
Added when I was still undecided what to do first.
Anyway whole thing wasn't meant to be reviewed yet - just wanted to see posix_memalign being handled by all the targets CI builds against.

kgaillot · 2021-09-07T19:17:23Z

src/sbd-md.c

@@ -120,6 +122,7 @@ open_device(const char* devname, int loglevel)
 		} else {
 			cl_log(loglevel, "Opening device %s failed.", devname);
 		}
+		free(st->buffer);


kgaillot · 2021-09-07T19:40:53Z

coverity seems to be complaining about a pre-existing memory leak in header_get()

wenningerk · 2021-09-08T06:16:51Z

coverity seems to be complaining about a pre-existing memory leak in header_get()

Yeah allocation method changed so that preexisting baseline doesn't match anymore.
Overall I'm still undecided if I want to go this minimalistic way or go with a pool of buffers
and not have copying - with the drawback that code has to be modified to kind of
track further processing of sector read.
Probably the amount of data handled by sbd is small enough that investing in avoiding
copying isn't really worth it.
And when we're anyway copying for keeping around the header it might be cleaner to
not keep a full sector.
I'm as well not sure if we couldn't improve sector-size being used by e.g. using
LCM(BLKBSZGET, BLKSSZGET). That would avoid the read-out, modify, write
on e.g. a physically 4K disk. This is critical for pre-existing disks of course but
we might go with the logical sector size, read the header and go with a potentially
larger sector size just after reading it from the header (or start with the larger
and reduce on readout - should both equally work). So it would just affect newly
created sbd-disks.

kgaillot · 2021-09-08T13:49:13Z

Overall I'm still undecided if I want to go this minimalistic way or go with a pool of buffers
and not have copying - with the drawback that code has to be modified to kind of
track further processing of sector read.
Probably the amount of data handled by sbd is small enough that investing in avoiding
copying isn't really worth it.

That's what I was thinking too

And when we're anyway copying for keeping around the header it might be cleaner to
not keep a full sector.
I'm as well not sure if we couldn't improve sector-size being used by e.g. using
LCM(BLKBSZGET, BLKSSZGET). That would avoid the read-out, modify, write
on e.g. a physically 4K disk. This is critical for pre-existing disks of course but
we might go with the logical sector size, read the header and go with a potentially
larger sector size just after reading it from the header (or start with the larger
and reduce on readout - should both equally work). So it would just affect newly
created sbd-disks.

Sounds reasonable

switch to using a single aligned buffer that stays allocated. - free to use allocation mechanism that doesn't allow to use free() to hand back (not needed for current posix_memalign) - avoid memory fragmentation risk (and thus risk to run out of hogged memory) due to frequent alloc/free of aligned memory

kgaillot · 2021-09-23T20:02:58Z

Looks good after a quick review without getting too deep into it

If we're going to introduce a compatibility break with pacemaker, I'm wondering if we have enough information requirements to make a public library worthwhile. Pacemaker SBD support could depend on the availability of the library, which would provide equivalents of panic_sbd(), pcmk__locate_sbd(), pcmk__get_sbd_timeout(), and pcmk__get_sbd_sync_resource_startup(). We could possibly drop the requirement that SBD and pacemaker be built with the same sync default, instead pacemaker could adapt to SBD's default.

wenningerk · 2021-09-24T08:30:45Z

...

If we're going to introduce a compatibility break with pacemaker, I'm wondering if we have enough information requirements to make a public library worthwhile.
...

You mean a library coming from the sbd-package?
If yes we're getting a nasty circular build-dependency.
A possibility would then be making the pacemaker-working a separate binary
that is coming from the pacemaker-package.
Nice thing about that approach would be that build-dependencies would
become the same as runtime-dependencies. First corosync, then (or at
the same time for runtime) corosync and finally pacemaker.
Have to think about further implications this might have apart from maybe
a more complex upgrade-path.
For now I guess I'm gonna revert this PR to it's initial purpose - taking care
of the memory-management and disk-access.
Especially as I found that the issue that made me start getting startup to
state-of-the-art was actually coming from improper use - since ever -
of aio.

kgaillot · 2021-09-24T14:32:52Z

...

If we're going to introduce a compatibility break with pacemaker, I'm wondering if we have enough information requirements to make a public library worthwhile.
...

You mean a library coming from the sbd-package?

Yes

If yes we're getting a nasty circular build-dependency.
A possibility would then be making the pacemaker-working a separate binary
that is coming from the pacemaker-package.

I wasn't thinking of putting anything pacemaker-related in sbd's public API, since that's just for sbd's internal use. The public API would just have low-level stuff for external code to check on sbd. It should be segregated in a separate directory and not use any pacemaker APIs.

wenningerk · 2021-09-24T15:33:44Z

I wasn't thinking of putting anything pacemaker-related in sbd's public API, since that's just for sbd's internal use. The public API would just have low-level stuff for external code to check on sbd. It should be segregated in a separate directory and not use any pacemaker APIs.

That wasn't my point. sbd already has pacemaker as build-time requirement.
So building a library as part of sbd-build that pacemaker is supposed to build
against creates some kind of build-order-issue.

kgaillot · 2021-09-24T16:36:09Z

I wasn't thinking of putting anything pacemaker-related in sbd's public API, since that's just for sbd's internal use. The public API would just have low-level stuff for external code to check on sbd. It should be segregated in a separate directory and not use any pacemaker APIs.

That wasn't my point. sbd already has pacemaker as build-time requirement.
So building a library as part of sbd-build that pacemaker is supposed to build
against creates some kind of build-order-issue.

Ah right, of course. Hmm.

Isolating the pacemaker-related stuff into the pacemaker project or even its own project could be worthwhile. I believe at one point we had discussed making the interface more generic so the corosync/pacemaker stuff wasn't so tightly integrated. I'm not sure if that would open any demand for other sbd "plugins" but it could be interesting. The question is whether it's worth the effort ...

wenningerk · 2021-09-27T14:19:36Z

As said moved the start-type things to a separate PR to concentrate on aligned-allocation and fix of use of the AIO API with this PR.

kgaillot · 2021-09-27T15:17:00Z

looks good to me

wenningerk · 2021-10-01T06:40:42Z

@gao-yan:

Any comments/ideas?
I mean for the memory-management and the API-fix ...
Of course ideas for the start-type and related stuff are welcome as well in #135

gao-yan · 2021-10-01T16:04:35Z

If we're going to introduce a compatibility break with pacemaker, I'm wondering if we have enough information requirements to make a public library worthwhile. Pacemaker SBD support could depend on the availability of the library, which would provide equivalents of panic_sbd(), pcmk__locate_sbd(), pcmk__get_sbd_timeout(), and pcmk__get_sbd_sync_resource_startup(). We could possibly drop the requirement that SBD and pacemaker be built with the same sync default, instead pacemaker could adapt to SBD's default.

Specifically about SBD_SYNC_RESOURCE_STARTUP, maybe we could just let pacemaker decide the default and export it for example in a header/pkgconfig so that sbd could comply?

kgaillot · 2021-10-01T20:17:30Z

Specifically about SBD_SYNC_RESOURCE_STARTUP, maybe we could just let pacemaker decide the default and export it for example in a header/pkgconfig so that sbd could comply?

That's more relevant to #135, which was stripped out of this one so it could focus on the memory alignment and async I/O usage issues

gao-yan

The switch to posix_memalign() and the relevant changes/fixes look sensible and thorough to me. Nice work, thanks!

Fix: avoid using deprecated valloc & frequent aligned alloc

kgaillot reviewed Sep 7, 2021

View reviewed changes

wenningerk force-pushed the aligned_alloc branch from 74a4449 to f9bf272 Compare September 8, 2021 05:50

wenningerk force-pushed the aligned_alloc branch from f9bf272 to 3c7a550 Compare September 8, 2021 14:06

wenningerk force-pushed the aligned_alloc branch 2 times, most recently from d500274 to bdd28f9 Compare September 9, 2021 04:38

wenningerk force-pushed the aligned_alloc branch 2 times, most recently from 8a004d2 to dd3004a Compare September 23, 2021 11:30

wenningerk added 3 commits September 27, 2021 16:05

Refactor: sbd-md: open_device, close_device

b3b822c

Fix: sbd-md: properly free one-time-allocations of sector-buffers

8d19d37

Fix: sbd-md: properly destroy io-context

9e6cbba

wenningerk force-pushed the aligned_alloc branch from dd3004a to 9e6cbba Compare September 27, 2021 14:10

wenningerk mentioned this pull request Sep 27, 2021

[WIP] Modernize start method #135

Open

gao-yan reviewed Oct 4, 2021

View reviewed changes

wenningerk merged commit 9d1fd44 into ClusterLabs:master Oct 4, 2021

wenningerk added a commit that referenced this pull request Oct 5, 2021

Merge pull request #134 from wenningerk/aligned_alloc

5ed9fd2

Fix: avoid using deprecated valloc & frequent aligned alloc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix: avoid using deprecated valloc & frequent aligned alloc #134

[WIP] Fix: avoid using deprecated valloc & frequent aligned alloc #134

wenningerk commented Sep 6, 2021 •

edited

Loading

kgaillot left a comment

kgaillot Sep 7, 2021

wenningerk Sep 8, 2021

kgaillot Sep 7, 2021

kgaillot commented Sep 7, 2021

wenningerk commented Sep 8, 2021

kgaillot commented Sep 8, 2021

kgaillot commented Sep 23, 2021

wenningerk commented Sep 24, 2021

kgaillot commented Sep 24, 2021

wenningerk commented Sep 24, 2021

kgaillot commented Sep 24, 2021

wenningerk commented Sep 27, 2021

kgaillot commented Sep 27, 2021

wenningerk commented Oct 1, 2021

gao-yan commented Oct 1, 2021

kgaillot commented Oct 1, 2021

gao-yan left a comment

[WIP] Fix: avoid using deprecated valloc & frequent aligned alloc #134

[WIP] Fix: avoid using deprecated valloc & frequent aligned alloc #134

Conversation

wenningerk commented Sep 6, 2021 • edited Loading

kgaillot left a comment

Choose a reason for hiding this comment

kgaillot Sep 7, 2021

Choose a reason for hiding this comment

wenningerk Sep 8, 2021

Choose a reason for hiding this comment

kgaillot Sep 7, 2021

Choose a reason for hiding this comment

kgaillot commented Sep 7, 2021

wenningerk commented Sep 8, 2021

kgaillot commented Sep 8, 2021

kgaillot commented Sep 23, 2021

wenningerk commented Sep 24, 2021

kgaillot commented Sep 24, 2021

wenningerk commented Sep 24, 2021

kgaillot commented Sep 24, 2021

wenningerk commented Sep 27, 2021

kgaillot commented Sep 27, 2021

wenningerk commented Oct 1, 2021

gao-yan commented Oct 1, 2021

kgaillot commented Oct 1, 2021

gao-yan left a comment

Choose a reason for hiding this comment

wenningerk commented Sep 6, 2021 •

edited

Loading