Horton

Thread

Commits

  1. Distinguish datacheckums worker invocations more reliably

  2. Minor cleanup around checking datachecksum worker result

  3. Avoid leaving DataChecksumState->worker_pid to an old value

  4. Misc cleanup in datachecksums_state.[ch]

  1. Little checksum worker cleanups

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-06-23T20:40:20Z

    Hi,

I had another fresh look at datachecksum_state.c while rebasing my 
"Interrupts vs signals" patch set, and spotted a few minor things that I 
think should be cleaned up. See commit messages for details.

Unless I'm missing something, the last patch fixes a bug, albeit a very 
theoretical one. The crux is that when the launcher exits, the worker 
might be left running; launcher_exit sends it SIGTERM but it might not 
exit instantly. If the launcher is restarted, and it launches a new 
worker while the old one is still running, the old launcher might set 
the worker's result field in shared memory, misleading the launcher to 
believe that the *new* worker succeeded.

That'd race condition would be really hard to hit in practice - I didn't 
even try to write a test - but it'd be nice to fix it. The patch adds a 
unique ID to each worker invocation to distinguish the old and new 
worker if both are running at the same time, ensuring that the old 
worker doesn't mess with the new worker's state.

- Heikki

  2. Re: Little checksum worker cleanups

    Daniel Gustafsson <daniel@yesql.se> — 2026-06-23T21:05:09Z

    > On 23 Jun 2026, at 22:40, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> I had another fresh look at datachecksum_state.c while rebasing my "Interrupts vs signals" patch set, and spotted a few minor things that I think should be cleaned up. See commit messages for details.

Thanks! I'll have a proper look at these in the morning with fresh eyes.

--
Daniel Gustafsson

  3. Re: Little checksum worker cleanups

    Daniel Gustafsson <daniel@yesql.se> — 2026-06-24T09:11:08Z

    > On 23 Jun 2026, at 22:40, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> I had another fresh look at datachecksum_state.c while rebasing my "Interrupts vs signals" patch set, and spotted a few minor things that I think should be cleaned up. See commit messages for details.

Thanks for the postcommit review, all the attached patches LGTM.

> Unless I'm missing something, the last patch fixes a bug, albeit a very theoretical one. The crux is that when the launcher exits, the worker might be left running; launcher_exit sends it SIGTERM but it might not exit instantly. If the launcher is restarted, and it launches a new worker while the old one is still running, the old launcher might set the worker's result field in shared memory, misleading the launcher to believe that the *new* worker succeeded.
> 
> That'd race condition would be really hard to hit in practice - I didn't even try to write a test - but it'd be nice to fix it. The patch adds a unique ID to each worker invocation to distinguish the old and new worker if both are running at the same time, ensuring that the old worker doesn't mess with the new worker's state.

+   uint64      worker_invocation_counter;
    ...
+   invocation = ++DataChecksumState->worker_invocation_counter;
+   DataChecksumState->worker_invocation = invocation;

This could safely be a uint32 without risking overflow in practice, but the
memory savings are fairly slim.  Perhaps we should add a comment stating why
there is no overflow protection to try and mitigate LLM/static analyzer
findings being reported?

> <0002-Clarify-StartDataChecksumsWorkerLauncher-function.patch>

+typedef enum DataChecksumsWorkerOperation
+{
+   ENABLE_DATACHECKSUMS,
+   DISABLE_DATACHECKSUMS,
+} DataChecksumsWorkerOperation;

Just as a note, this clearly looks like it could be made a boolean but it was
intentionally made to support a (still hypothetical) future
VERIFY_DATACHECKSUMS operation.

--
Daniel Gustafsson

  4. Re: Little checksum worker cleanups

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-06-24T12:40:56Z

    On 24/06/2026 12:11, Daniel Gustafsson wrote:
>> On 23 Jun 2026, at 22:40, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
>> I had another fresh look at datachecksum_state.c while rebasing my "Interrupts vs signals" patch set, and spotted a few minor things that I think should be cleaned up. See commit messages for details.
> 
> Thanks for the postcommit review, all the attached patches LGTM.

Committed, thanks for the review. (I squashed a few of the cosmetic ones)

- Heikki