-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
downgrade systemd-tmpfiles failure on unsupported chattr() flags to a non-fatal warning #560
Comments
Hmmm. This apparently is not an issue on ext4... I have the suspicion that this should be fixed in the kernel so that the ioctl behaves the same on all file systems. |
On XFS, I get a slightly different error.
The manual page for chattr has the following note:
I'm honestly a bit suprised that it doesn't generate an error on ext4. If it doesn't support COW, I guess it just silently throws this flag away? |
Just for clearness, the problem exists after an upgrade to systemd-222 and the new file "/usr/lib/tmpfiles.d/journal-nocow.conf". Setting nocow on BTRFS sounds like an good idea, but make this the default? As @poettering says, the ioctl should be fixed in kernel for ext4 but in a way that this FS also trow an error as the other FS. But if this gets fixed this FS is another one on which the systemd-tmpfiles-setup.service will fail and in the end the system shows an degraded state. |
On XFS,
|
I am pretty sure the behaviour of ext[234] should be considered the gold standard how file systems should react. If the other, more exotic file systems disagree with that, then they should be fixed in the kernel. In general, if a file system supports chattr at all then I am sure it should store all bits, even the ones it doesn't support, so that file trees become portable between file systems, even if some of the options don't always have an effect. Of course, this means there's no way then for userspace to detect which bits are actually supported on a specific file system, but to cover that I guess there should be a new kernel ioctl that returns the mask that the file system supports at all. Hence, please file a kernel bz bug asking to adjust xfs and reiserfs behaviour for these ioctls, to expose the same behaviour as ext234 here. That all said, I am happy to take a patch that downgrades the error to a warning, that shows up in the logs if this happens, but does not result in unclean exit. That way, people running xfs/reiserfs will see a log message about this until the kernel gets fixed, but it will not result in service failure. |
@floppym hmm, does reiserfs support file attributes at all? i.e. are +i and attributes like that supported on reiserfs at all? |
(I am asking this since on file systems that support no attributes at all we should probably downgrade the error all the way to "debug", so that it is not normally seen at all.) |
…ttributes This downgrades errors from setting file attributes via tmpfiles to warnings and makes them non-fatal. Also, as a special case, if a file system does not support file attributes at all, then the message is downgraded to debug, so that it is not seen at all. With this change reiserfs should not see any messages at all anymore (since it apparently does not implement file attributes at all), but XFS will still get a warning but no failure. The warning is something the XFS kernel folks should fix though, by adjusting their file attributes behaviour to be identical to ext234's. Fixes systemd#560.
I implemented what I proposed above now in PR #663. Please still file the kernel bug against XFS though, it really should be fixed to mimic ext234's behaviour on this. As long as this is not fixed in XFS you'll still get a warning there. The patch will downgrade the message to debug on reiserfs, since reiserfs returns ENOTTY, suggesting it doesn't support the ioctl at all. |
While I agree that it would be desireable to store and keep file attributes, even if the fs doesn't support them, i think the behaviour of ext* is wrong here, as it just throws away the attribute without reporting an error. |
Well, the kernel API is fucked up, no doubt. But the API is the API and everything that doesn't follow ext234's currently fucked up API behaviour here is breaking API. Fixing the API would mean adding a ioctl or two to query which bits are stored/have effect. |
Ok @poettering, thats your point of view. So lets look at a few file systems:
So in my point of view i wouldn't blame the Kernel API or implement new ioctl, fixing ext and jfs to behave like the other FS which doesn't support that feature is an better approach as then it is clearly shown in userspace that this is not supported (anyway i understand your argument with the portability). |
@MorphBonehunter ok, so summarizing your findings, after #663 has been merged now we are now good on all, but xfs, exfat, ntfs. Given that they are all more exotic than ext234 (and that the ioctl is originlly from ext234) they should be fixed to follow its semantics. Right now they return three different error codes however. That's just fucked up. Please file bugs against the three file systems, asking them to closely follow ext234's behaviour if they implement the ioctl. |
@gionniboy We have to rely on a consistent kernel API across different file system types in order to write tools that are versatile enough to work with either one of them. Don't you agree? Now, in systemd, we have the rule to not work around bugs in other components, unless the other component officially refuses to fix something. Because when done at the right place, fixes also help other projects. Makes sense? |
There is no way to fail it at all. as it said in the commit *The journal write pattern is problematic on btrfs file systems as it results in badly fragmented files when copy-on-write (COW) is used: the performances decreases substantially over time." |
OK my Archlinux just upgraded the systemd to 223. journal-nocow.conf is there again. What should I do? Read the commit history of the last week to see what have changed in this everyone-can-comprehend system? Delete the journal-nocow.conf again or not? You don't even provide a release note. Ohh my friends... |
@wuxb45 /usr is territory of the package manager, you are not supposed to delete/modify files there, only the package manager should. If you want to disable it, simply symlink /dev/null to /etc/tmpfiles.d/journal-nocow.conf. This will mask the file, as tmpfiles.d follows the usual rule that files in /etc override similar named files in /usr/lib and can be masked away by symlinking /dev/null over it. See tmpfiles.d(5) for more info. |
@poettering Thanks! It would work for my system for a long time as I don't use btrfs for the journaling subsystem. |
systemd-tmpfiles seems to exit with a non-zero status when it fails to set the (invalid) nocow attr on a file on reiserfs.
This also fails when you call chattr +C.
This was originally reported on Gentoo Linux.
https://bugs.gentoo.org/show_bug.cgi?id=554530
The text was updated successfully, but these errors were encountered: