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

ignore return code of fstrim #721

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

nopeslide
Copy link
Contributor

devices (i.e. virtualized) may not support fstrim operations.

@behrmann
Copy link
Contributor

Thanks for the contribution!

I guess this should be unproblematic.

@DaanDeMeyer
Copy link
Contributor

Can we ignore a specific return code instead of ignoring all return codes? It would be even better if we could check if fstrim is supported before running fstrim but I guess detecting if it's supported might not be trivial.

@keszybz
Copy link
Member

keszybz commented May 24, 2021

--quiet-unsupported instead?

@poettering
Copy link
Member

Yes, please rework this to just use fstrim's --quiet-unsupported switch, which will just make the unsupported error go away, but all other errors will still be fatal and be logged.

@nopeslide
Copy link
Contributor Author

@poettering Tried the flag first, but this seems to only surpress the output not the error code.

@nopeslide
Copy link
Contributor Author

nopeslide commented May 27, 2021

See https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/sys-utils/fstrim.c#n544

rc = fstrim_filesystem(&ctl, path, NULL);
if (rc == 1 && !ctl.quiet_unsupp)
	warnx(_("%s: the discard operation is not supported"), path);

return rc == 0 ? EXIT_SUCCESS : EXIT_FAILURE;

@nopeslide nopeslide force-pushed the fstrim-ignore-unsupported branch 2 times, most recently from b40e534 to 940c05a Compare May 27, 2021 16:15
@nopeslide
Copy link
Contributor Author

removed the non-existing co-author

devices (i.e. virtualized) may not support fstrim operations.
@DaanDeMeyer
Copy link
Contributor

Sorry for taking so long on this. Can we run the process (without the flag) and check the returncode and the output to figure out if the failure is due to fstrim being unsupported? If it's due to any other error, we can call die to make sure we don't continue if fstrim failed for another reason.

@poettering
Copy link
Member

@karelzak shouldn't fstrim suppress not only the warning but also the unclean exit code when --quiet-unsupported it specified? i.e. isn't this something to fix in the fstrim tool?

@poettering
Copy link
Member

poettering commented Jun 4, 2021

Id probably merge this PR as it is though, the fstrim stuff is best-effort anyway, I doubt we need to put too much effort into working around the fact that we can't recognize the error cause properly here... I mean, even if @karelzak changes fstrim to return a clean exit code on EOPNOTSUPP, we should probably just work with released versions of fstrim too.

@DaanDeMeyer DaanDeMeyer merged commit 24b0d18 into systemd:main Jun 13, 2021
karelzak added a commit to util-linux/util-linux that referenced this pull request Jul 1, 2021
This feature is already supported for -a and -A. Let's support it also
when FS specified on command line.

Addresses: systemd/mkosi#721
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak added a commit to util-linux/util-linux that referenced this pull request Jul 20, 2021
This feature is already supported for -a and -A. Let's support it also
when FS specified on command line.

Addresses: systemd/mkosi#721
Signed-off-by: Karel Zak <kzak@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants