etc/systemd/zfs-mount-generator: serialise, handle keylocation=http[s]://

[nabijaczleweli]

Description

The first patch serialises the generator, see message for measurements. No output changes.

The second patch adds keylocation=http[s]:// by pulling in network-online.target but otherwise loading normally.

The third patch I would like The Reviewer's opinion on: it's slightly shorter and matches what's logically happening slightly better, but it could also go wrong easier in future if we have to allocate in that block or some code moves around and makes tofree no longer append-only (which is a good quality to have). I've looked at it too much to have a meaningful opinion.

How Has This Been Tested?

The first patch produces identical output on my test set. If FREE_STATICS is true, it's valgrind-clean (save for libshare, but what can you do).

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes. – none apply
• I have run the ZFS Test Suite with this change applied. – CI take my hand
• All commit messages are properly formatted and contain Signed-off-by.

[tonynguien]

@aerusso @InsanePrawn, would you be able to review these changes?

[rlaager]

This seems fine to me.

Even though I was the one that asked about it, I'm quite surprised that serializing the generator is faster. I guess the fork() overhead is high. That's why it's important to profile, I guess. :)

I could go either way on the third commit. On the one hand, it feels a little weird to remove something from the list of things to free. But on the other hand, that seems to be happening because ownership is transferred.

[aerusso]

Nice speedup! I especially like the reduction in nloc.

This is starting to become nontrivial enough that we should probably add tests for the generator. I don't think that's really a requirement for this MR, though.

[nabijaczleweli]

@rlaager I did profile the first version before posting it, and it spent 70% of its time in fork() (on relatively idle Westmere with a lot of free RAM, this (likely, I haven't measured it directly this time, but it falls out of some other tests) skyrockets on my work laptop test environment (laptop CPU (8th-gen, but still; it's not running cold or fast), single-channel RAM, QEMU on HAX on Windows, horribly slow virtual memory)), and 5%ish in str(c?)spn() (i.e. strtok()), but to me the simplified state and lifetime management still vastly outweighed the potential benefits of throwing away the version I tested extensively and which was already ~20x faster.
It also doesn't really show up for Normal amounts of datasets, because it's already quite short, but I run ZFS-backed Docker/k8s on that VM, which is 900+ datasets on a good day, so I don't realistically expect this speedup to affect too many set-ups.

As for the escape handling, seeing as you haven't a strong opinion, I've consulted some friends (this was a mistake, never ask programmers about anything), and the consensus there seems to be that *(tofree++) = (void *)mountfile;+*(--tofree) = NULL; has less potential to go bad, and if it does, it will do so much harder and more obviously, which is preferable, so I've squashed it into the first commit (previous HEAD at zomg-eleven-v1, should it be needed).

@aerusso I can dump my test dataset into ZTS in a later PR if you want, but it's difficult to test this other than with diff -r to pre-generated output (since the correct testing procedure would be to inject the units into a VM image, boot it, and observe how it behaves from within and without; this is doable, but in a definite "nah bro" state from me for the foreseeable future), which is the absolute worst kind of test, especially in perpetua and across multiple tens of files, since any changes to the output format just work out to regenerating the data, which, y'know.

[InsanePrawn]

I can dump my test dataset into ZTS in a later PR if you want, but it's difficult to test this other than with diff -r to pre-generated output [...] which is the absolute worst kind of test, especially in perpetua and across multiple tens of files, since any changes to the output format just work out to regenerating the data, which, y'know.

historically (at least during my involvement), only columns were added, and the added properties were optional/user props -> default to -; so if you handle a missing column == -, a certain downwards compatibility is achieved.
Having some sanity tests that generate units from a basic list.cache file and verify that certain entries in the unit are set correctly does seem feasible and defo better than nothing to me?
If we wanted to, we could even test for this downwards compatibility.
[To be clear: i'm not saying support and test arbitrary amounts of missing columns, I'm thinking about testing known older versions of the cache file (or more precisely, the set of columns it had) that we expect to be compatible, e.g. 0.8, 2.0, etc.; if we ever decide to break compat, the tests should reflect this.]

Testing the cache-list generator itself is another open point afaik.

[nabijaczleweli]

Rebased. AFAICT the review only points toward further expansion because both want* strings are effectively constant and there's a need_ flag for the network, so I don't think there's a blocker for this, and not allowing http[s]:// in the generator by default is kinda stinky and bit me a few weeks ago

[rlaager]

I think @aerusso makes a good point. I left a comment with an alternate suggestion that I think makes this slightly simpler.

[nabijaczleweli]

It did, and I've also applied it to RequiresMountsFor= below, since it's so much simpler

[nabijaczleweli]

Week bump – CI passes and reviews addressed

[rlaager]

From my perspective, this is good to go. I did actually compile and run it. I get sane results on my system.

[tonynguien]

Looks good to me.

[tonynguien]

@nabijaczleweli We are ready to merge. Would you mind squashing the commits?

[nabijaczleweli]

Done.