Skip to content

units: pull in udev from systemd-vconsole-setup.service#11377

Open
keszybz wants to merge 1 commit intosystemd:mainfrom
keszybz:vconsole-setup-ordering
Open

units: pull in udev from systemd-vconsole-setup.service#11377
keszybz wants to merge 1 commit intosystemd:mainfrom
keszybz:vconsole-setup-ordering

Conversation

@keszybz
Copy link
Copy Markdown
Member

@keszybz keszybz commented Jan 10, 2019

Fixes #10019:

At the time the initial transaction is constructed, s-vc-setup.service is not
part of it. Therefore any service which only has After=s-vc-setup.service as
ordering constraint can be scheduled at any time. And if PID1 decides to
schedule it before udev then the service will be run before
s-vc-setup.service even if the later will finally be run during the process.

Actually even if the service is run after udev is, there's no guarantee that
s-vc-setup.service will run before.

Let's pull in systemd-udevd.service and order ourselves after it.

@keszybz keszybz added the units label Jan 10, 2019
@fbuihuu
Copy link
Copy Markdown
Contributor

fbuihuu commented Jan 10, 2019

Sorry but I stil can't see how it could work: s-vc-s.service is started asynchronously only by udev so ordering this service against udev has no effects in practice.

@bl33pbl0p
Copy link
Copy Markdown
Contributor

Right, After= on this service will only have effect as long it has a job in queue. Since this service is activated by udev, and has Wants= and After= on udev, it still isn't part of the initial transaction (unlike the service that has After= declared on it which would be pulled by some target), so ordering has no effect (as their is no job).

@yuwata
Copy link
Copy Markdown
Member

yuwata commented Jan 10, 2019

If I remember correctly, we had a discussion about dropping the unit. Though, I cannot find where we discussed that...

@keszybz
Copy link
Copy Markdown
Member Author

keszybz commented Jan 10, 2019

My idea was that if s-vc-setup.service is pulled in by any means, it can only function correctly if it's after udev. But @fbuihuu and @bl33pbl0p are right, this isn't enough. We only recommend a After=s-vc-setup.service dependency. We also needs Wants= to make sure that both s-vc-setup.service and systemd-udev.service as pulled in and ordered properly. I'll update.

@keszybz keszybz force-pushed the vconsole-setup-ordering branch from 8072072 to 963704c Compare January 10, 2019 12:10
@keszybz
Copy link
Copy Markdown
Member Author

keszybz commented Jan 10, 2019

Updated.

@keszybz
Copy link
Copy Markdown
Member Author

keszybz commented Jan 10, 2019

If I remember correctly, we had a discussion about dropping the unit. Though, I cannot find where we discussed that...

IIRC, previously it was pulled in unconditionally, and the outcome of that discussion was status quo that it is only pulled in by an udev rule.

Documentation=man:systemd-vconsole-setup.service(8) man:vconsole.conf(5)
DefaultDependencies=no
Wants=systemd-udevd.service
After=systemd-udevd.service
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand the point of these deps: the fact that udevd has been started doesn't mean that the vconsole underlying device is ready to be used. In theory the kernel can send the corresponding event at any time and there's no way for a unit to be ordered against this event (unless a dedicated device unit is created).

As the vconsole setup is not mandatory for using the console, I think s-vc-s.service shouldn't have any impact on the scheduling of the services depending on it. Those services should just try to run s-vc-s.service before they're run and if the console can be setup then fine otherwise just proceed anyway.

@poettering
Copy link
Copy Markdown
Member

not sure i grok this? what you are looking for is a way to ensure s-vc-s.s is included in the initial transaction, no? But this PR doesn't do that, it just pulls in udev from s-vc-s, but that doesn't really change anything this udevd is the one pulled in anyway, and s-vc-s is the one that typically is not.

i am not sure how the VC subsystem works: is it possible that the VC doesn't exist initially but is added later? if so, i don't see how we could ever fix this: if we don't know whether a VC shows up we cannot possibly delay things until after it has been initialized, because we can't distuingish between "shows up late" and "shows up never"...

If otoh the VC is either compiled in and hence always available, or not compiled in and hence never available, then why don't we just statically neable s-vc-s.s and rely on ConditionXYZ= to bypass execution if the VC is actually not there?

@keszybz keszybz force-pushed the vconsole-setup-ordering branch from 963704c to 1fe6fc6 Compare March 19, 2020 12:32
…-setup.service

For systemd#10019:
> At the time the initial transaction is constructed, s-vc-setup.service is not
> part of it. Therefore any service which only has After=s-vc-setup.service as
> ordering constraint can be scheduled at any time. And if PID1 decides to
> schedule it before udev then the service will be run before
> s-vc-setup.service even if the later will finally be run during the process.
>
> Actually even if the service is run after udev is, there's no guarantee that
> s-vc-setup.service will run before.

Recommend the Wants= dependency to make sure that the service is pulled in even
if not part of the initial transaction. Without that the After= dependency
could have no effect. s-vc-setup.service is very cheap if there are no consoles
to set up, so the downsides of pulling it in if not strictly necessary are
negligible.
@keszybz keszybz force-pushed the vconsole-setup-ordering branch from 1fe6fc6 to b6ba0ba Compare March 19, 2020 12:35
@keszybz
Copy link
Copy Markdown
Member Author

keszybz commented Mar 19, 2020

Update to have just the man page tweak. That change is still good, even if it does not solve the issue completely.

Copy link
Copy Markdown
Contributor

@fbuihuu fbuihuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps one way to completely fix this is to make s-vc-s remember which VCs it initialized during each invocation so we're sure that it won't screw already configured VCs which might be already in used.

@keszybz keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed needs-review labels Dec 9, 2020
Base automatically changed from master to main January 21, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks units vconsole

Development

Successfully merging this pull request may close these issues.

No reliable ways to wait for the end of the init of virtual terminals

6 participants