Skip to content

mount/generators: do not make unit wanted by its device unit#11373

Merged
poettering merged 2 commits intosystemd:masterfrom
tomty89:auto
Feb 15, 2019
Merged

mount/generators: do not make unit wanted by its device unit#11373
poettering merged 2 commits intosystemd:masterfrom
tomty89:auto

Conversation

@tomty89
Copy link
Copy Markdown
Contributor

@tomty89 tomty89 commented Jan 9, 2019

As device units will be reloaded by systemd whenever the corresponding device generates a "changed" event, if the mount unit / cryptsetup service is wanted by its device unit, the former can be restarted by systemd unexpectedly after the user stopped them explicitly. It is not sensible at all and can be considered dangerous. Neither is the behaviour conventional (as auto in fstab should only affect behaviour on boot and mount -a) or ever documented at all (not even in systemd, see systemd.mount(5) and crypttab(5)).

@bl33pbl0p
Copy link
Copy Markdown
Contributor

bl33pbl0p commented Jan 9, 2019

Note: device units are not actually reloaded (reload isnt't applicable to them), but a JOB_NOP is enqueued for them, and then the manager generates a transaction for all dependencies that defines 'reload from' dependencies using the JOB_NOP job as an anchor job (and then activates the transaction).

@boucman
Copy link
Copy Markdown
Contributor

boucman commented Jan 9, 2019

so wouldn't the correct fix to break the ReloadPropagates dependency ?

@tomty89
Copy link
Copy Markdown
Contributor Author

tomty89 commented Jan 10, 2019

First of all, I'm very poor at writing commit message, so it may not reflect what the problem actually is very accurately.

Then for the "reloaded" part, it's a direct copy from systemd.device(5), I haven't really checked whether the statement is accurate.

But the problem seems pretty simple and straight-forward to me: anything that triggers a "start" equivalence of a device unit would trigger "start" of its mount units / cryptsetup services (with auto), and the "start" equivalence of the device unit can be triggered on many occasions (in other words, its definition is pretty vague)

But then that may not even be the core of the problem. To me it's simply because whoever wrote that code wasn't having a clear mind and think "that's what auto can also do for us!" without seeing the whole picture.

For example, if you have multiple mount units with auto requiring the same cryptsetup service (so same device as well), you stopped them all, then just restarting one of the mount unit or the service will cause all the other mount units be restarted.

In fact the dependency makes no sense or point at all except when people want to abuse auto and make systemd work like a udisks frontend (the "auto,nofail" "plug and mount" crap). Truth is they are simply relying on a side effect of this bug.

@keszybz
Copy link
Copy Markdown
Member

keszybz commented Jan 10, 2019

Yeah. It's bothered me in the past, but never enough to really investigate ;)

To summarize: the functionality is to automatically mount the device as soon as it is plugged in. It it enabled automatically for all non-noauto mount point in /etc/fstab. This would be useful, except that all those points are already part of local-fs.target or remote-fs.target. There are two possibilities:

  • we are in a normal boot transaction and local-fs.target/remote-fs.target pull in the .mount unit. In this case pulling in the .mount unit from the .device unit is not necessary and makes no difference.
  • we are outside of the normal transaction (for example because something failed). In this case the dependency has an effect, but I'm not sure if it a good one. We will attempt to mount things automatically, which could make some things easier, but on the other hand, it can confuse the admin.

Then there's the case you list, where a mount unit is stopped manually and is restarted when the device emits a change event. This effect is pretty bad and I remember having issues with it in the past for example when trying to run fsck and such.

So yeah, I think your patch looks good. I'll wait for @poettering's review. Maybe there's some other motivation for this that we both missed.

@keszybz keszybz requested a review from poettering January 10, 2019 10:55
@poettering
Copy link
Copy Markdown
Member

i have the suspicion this is going to break lots of things.. But it's ok to merge i guess.

@keszybz keszybz added this to the v242 milestone Jan 15, 2019
@keszybz keszybz added the pid1 label Jan 15, 2019
@poettering poettering added postponed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Jan 21, 2019
As device units will be reloaded by systemd whenever the corresponding device generates a "changed" event, if the mount unit / cryptsetup service is wanted by its device unit, the former can be restarted by systemd unexpectedly after the user stopped them explicitly. It is not sensible at all and can be considered dangerous. Neither is the behaviour conventional (as `auto` in fstab should only affect behaviour on boot and `mount -a`) or ever documented at all (not even in systemd, see systemd.mount(5) and crypttab(5)).
@poettering poettering merged commit 7ca9289 into systemd:master Feb 15, 2019
@keszybz keszybz removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed postponed labels Mar 20, 2019
@mbiebl
Copy link
Copy Markdown
Contributor

mbiebl commented Oct 7, 2019

Related to this PR is this downstream bug report:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=941758

Having the .device unit declare a wants dependency on the .mount unit pretty much broke StopWhenUnneeded=true for .mount units.

@keszybz
Copy link
Copy Markdown
Member

keszybz commented Oct 8, 2019

So... you're saying that we did good, i.e. for once, not a bug report, but a fix report. Yay!

@arooks-ppo
Copy link
Copy Markdown

Related to @poettering's pre-merge comment:

i have the suspicion this is going to break lots of things.. But it's ok to merge i guess.

This did break things for us. I think there are lots of people who -- mostly unknowingly -- were depending on this side-effect of systemd to implement "plug-and-mount crap" without realizing it was a side effect. There are embedded (i.e. headless) applications where an interactive tool based on udisks isn't an option, and it's convenient to have a USB disk get mounted at plug-in.

There are many posts that describe how to do this using /etc/fstab and systemd; the most commonly-quoted one that I've seen is Dominique Dumont's Automount usb devices with systemd blog post. Those will no longer work because they are based on the side effect of the older systemd.

Note that I'm not advocating for backing this change out. I can get my old behaviour back using a udev rule to match my device's partition and then set SYSTEMD_WANTS to the appropriate mount point. Then it works the same as before. I might be able to run something using udisks2 to achieve a similar result, but I don't want to use a /media mount point and the udev rules seems simpler.

Just a heads-up that there are probably many others depending on mounts happening when devices are plugged in. If anyone wants the details of my udev rules, feel free to ask.

Oh, and a ping to @mbiebl who pulled this into Debian 10.2's systemd; I got bitten by the change of behaviour between a (not-updated) Debian 10.0 system (systemd 241-5) and a 10.3 install (systemd 241-7-deb10u3). Both of those versions of systemd announce that they are version 241, so I didn't notice at first that they are different.

@keszybz
Copy link
Copy Markdown
Member

keszybz commented Mar 3, 2020

If anyone wants the details of my udev rules

Sure, please post them here, so it can serve as reference.

@arooks-ppo
Copy link
Copy Markdown

I have an entry in /etc/fstab like this (this hasn't changed from older systemd; was all that was needed for device to mount at plugin, previously - that is, systemd 241 or Debian 10.0):
LABEL=usb-stick1 /mnt/usb1 ext4 nofail,users 0 2

I've added this udev rule, in the file /etc/udev/rules.d/98-usb-stick1.rules:
ACTION=="add", SUBSYSTEM=="block", KERNEL=="sd*", SUBSYSTEMS=="usb", ENV{DEVTYPE}=="partition", IMPORT{builtin}="blkid", ENV{ID_FS_TYPE}=="ext4", ENV{ID_FS_LABEL_ENC}=="usb-stick1", ENV{SYSTEMD_WANTS}+="mnt-usb1.mount"

This can be adapted in obvious ways to mount by UUID or with a specific device node like /dev/sdc1.

@fingolfin00
Copy link
Copy Markdown

fingolfin00 commented Feb 15, 2023

Sorry if I revive this thread.

I'm using @arooks-ppo solution (thank you btw) to automount of an external USB disk with systemd 244. It works, but only if launch a .service that "Wants" the corresponding .mount unit. If I set ENV{SYSTEMD_WANTS}+="mount-point-path.mount", it doesn't work and produces no output/error.

So I need a udev rule:

ACTION=="add", SUBSYSTEM=="block", KERNEL=="sd*", SUBSYSTEMS=="usb", ENV{DEVTYPE}=="partition", IMPORT{builtin}="blkid", ENV{ID_FS_TYPE}=="ext4", ENV{ID_FS_LABEL_ENC}=="data-ssd", ENV{SYSTEMD_WANTS}+="dummy.service"

And the dummy.service:

[Unit]
Description=Dummy service
Wants=opt-data\x2dssd.mount

[Service]
Type=simple
ExecStart=/bin/echo "I'm dummy"

[Install]
WantedBy=local-fs.target

My /etc/fstab contains the following line:

LABEL=data-ssd  /opt/data-ssd  ext4  defaults,nofail,users  0  2

Shouldn't it be possible to launch the mount unit directly from the udev rule?

Also may be worth mentioning that if I query with udevadm, ENV{SYSTEMD_WANTS} is printed out only with in the non-working case (.mount unit launched directly form the udev rule):

# udevadm info --query=property --path=/sys/class/block/sda1
[...]
SYSTEMD_WANTS=opt-data\x2dssd.mount
[...]

whereas it disappears if launching the dummy.service first.

Thanks

@poettering
Copy link
Copy Markdown
Member

Guys, this is not a support forum. Never has been. It's very bad style to ask question on unrelated bug reports.

https://lists.freedesktop.org/mailman/listinfo/systemd-devel

(Almost any udev rule with ACTION="add" is broken. You want ACTION!="remove" otherwise your prop will be immediately lost again once a "change" event is see. But anyway, further question to the ML, not here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

8 participants