Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fstrim: timer and service unit on fstab-less systems #1019

Closed
lucab opened this issue Apr 28, 2020 · 13 comments
Closed

fstrim: timer and service unit on fstab-less systems #1019

lucab opened this issue Apr 28, 2020 · 13 comments

Comments

@lucab
Copy link
Contributor

lucab commented Apr 28, 2020

This is a followup to the concern at #673 (comment).

Nowadays there exists systemd-based distributions which run without an /etc/fstab file, e.g. Container Linux and Fedora CoreOS. Over there, we are wondering how to get the same features provided by fstrim.timer and fstrim.service unit in a fstab-less way. #1016 is the first step into that direction, but it only silences the noisy service failure we experience today.

So far we had some quick brainstorming at coreos/fedora-coreos-tracker#468 and we came up with two alternative approaches:

  • reading the list of trimmable mounts from a volatile file (fstab-like) generated by some systemd-aware logic under /run.
  • introducing timer and service template units which can be instantiated per each trimmable mountpoint.

@karelzak what are your thoughts on this?

/cc @jlebon

@karelzak
Copy link
Collaborator

We originally had "fstrim --all" instead of "fstrim --fstab" in the service file. I think the option --all is still usable and you can use it in fstab-less systems.

The disadvantage is that it will try to trim also manually and temporarily connected media, but I don't think it's so big problem. All you need is to replace --fstab with --all in the service file on package install.

@dustymabe
Copy link
Contributor

Rather than overriding the complete ExecStart it might be easiler to override if we make an OPTIONS= (better naming ideas?) variable such that we can override the --fstab or --all easily without overriding all of ExecStart.

@karelzak
Copy link
Collaborator

I have no problem to accept whatever what will work for your use-case as well as for mainstream (fstab based) use-case ;-)

@lucab
Copy link
Contributor Author

lucab commented Apr 29, 2020

@karelzak yes, I think my original question was more about what is your preferred integration point for distros. That is, a sort of public API for systemd units.
If you prefer us to keep using your fstrim.service and just override the flag out it, we can proceed as @dustymabe suggested but revert #1016 first.

@karelzak
Copy link
Collaborator

I'm not sure if I understand @dustymabe's idea with OPTIONS=. Do you mean to define something like:

  Environment='OPTION=--fstab'
  ExecStart=/sbin/fstrim $OPTION --verbose --quiet-unsupported

would it be better to use sed s/--fstab/--all/ in your installation scripts?

Yes, #1016 seems unwanted.

@dustymabe
Copy link
Contributor

I'm not sure if I understand @dustymabe's idea with OPTIONS=. Do you mean to define something like:

  Environment='OPTION=--fstab'
  ExecStart=/sbin/fstrim $OPTION --verbose --quiet-unsupported

Right, and then we'd use an override to override OPTION to set it to OPTION=--all, or something else and probably also cancel out the ConditionPathExists=/etc/fstab.

would it be better to use sed s/--fstab/--all/ in your installation scripts?

I don't think so because it's nice to know where configuration comes from so we should leave what comes from util-linux in tact.

I think probably the bigger question we are trying to answer is what do we think is approprate to do for systems that don't have an fstab? If --all is suitable then we just use that. If not, then do we do something more complicated like what @lucab suggested in #1019 (comment):

  • reading the list of trimmable mounts from a volatile file (fstab-like) generated by some systemd-aware logic under /run.
  • introducing timer and service template units which can be instantiated per each trimmable mountpoint.

@karelzak
Copy link
Collaborator

karelzak commented May 4, 2020

From my point of view --all is good enough. The option --fstab has been introduced later to avoid removable or manually mounted filesystems (but these filesystems also need to trim, so --fstab is bad in some cases).

@karelzak
Copy link
Collaborator

I'd like to finalize this, what about to specify on fstrim command line multiple sources and the command will use the first usable, something like:

    fstrim --listed-in "/etc/fstab:/proc/self/mountinfo"

The result will be that you do not have to modify the service file at all.

karelzak added a commit that referenced this issue May 22, 2020
This new option works like --all but it allows to specify multiple
files with filesystems to make fstrim configuration more portable
between distributions. For example:

 fstrim --listed-in /etc/fstab:/proc/self/mountinfo

forces fstrim to try fstab and if unsuccessful than try mountinfo.

Addresses: #1019
Signed-off-by: Karel Zak <kzak@redhat.com>
@karelzak
Copy link
Collaborator

Please, test it in your environment. Thanks.

@karelzak
Copy link
Collaborator

karelzak commented Jun 1, 2020

@dustymabe and @lucab is --listen-in usable for you?

@lucab
Copy link
Contributor Author

lucab commented Jun 1, 2020

@karelzak sorry, I forgot to come back to this. Yes, I think that would do it. Do you still plan for distributions to be able to tune this? If so, you may consider to tweak the service unit as such:

[Service]
Environment="FSTRIM_TARGET=--listed-in /etc/fstab:/proc/self/mountinfo"
ExecStart=@sbindir@/fstrim ${FSTRIM_TARGET} --verbose --quiet-unsupported

That way distributions can simply adjust the -a vs -A vs --listed-in by setting their own FSTRIM_TARGET as it suits, without having to patch upstream content.

@karelzak
Copy link
Collaborator

karelzak commented Jun 4, 2020

Well, the idea of the --listed-in is that you do not have to modify the service file at all ;-) ... but if you believe that distros still need any changes than I can add FSTRIM_TARGET of course.

@lucab
Copy link
Contributor Author

lucab commented Jun 4, 2020

Ack, let's keep it as is for now then. We'll report back if we see any other issue. Thanks for all the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants