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

[btrfs] add new FSMount subclass for BTRFS (#1926892) #930

Conversation

michel-slm
Copy link
Contributor

@michel-slm michel-slm commented Feb 9, 2021

We need to use this to allow overriding the filesystem mount options
from the default. Ideally this can be overridden in configuration,
but short of having such a mechanism we can hardcode it for now.

Context: https://fedoraproject.org/wiki/Changes/BtrfsTransparentCompression

Signed-off-by: Michel Alexandre Salim michel@michel-slm.name


This change is Reviewable

@blivetghoul
Copy link

Can one of the admins verify this patch?

We need to use this to allow overriding the filesystem mount options
from the default. Ideally this can be overridden in configuration,
but short of having such a mechanism we can hardcode it for now.

Signed-off-by: Michel Alexandre Salim <michel@michel-slm.name>
@michel-slm michel-slm changed the title [btrfs] add new FSMount subclass for BTRFS [btrfs] add new FSMount subclass for BTRFS (#1926892) Feb 9, 2021
@vojtechtrefny
Copy link
Member

Jenkins, ok to test.

@vojtechtrefny
Copy link
Member

I've two problems with this:

  1. This should be Anaconda specific. Blivet is used also outside of Anaconda and I'm not sure we want to add the compress option by default everywhere.
  2. This affects all mounts of btrfs devices so if someone mounts a btrfs device using blivet it will be mounted with this option. Even if we limit the change to Anaconda, we mount all filesystems in Anaconda. Shouldn't this be somehow limited only to fstab entries created during installation?

If the change can't be done only in Anaconda (I still think, Anaconda should be able to use the mountopts parameter for this), we'll probably need a special flag to limit this behaviour only when running from the installer. I'll try to think of a better way to do this, overriding FSMount doesn't seems to be the best option.

@cmurf
Copy link

cmurf commented Feb 11, 2021

Shouldn't this be somehow limited only to fstab entries created during installation?

Ideally it's applied to the install time mount command, so that the installation itself is compressed. A fallback position would be to just add it to /etc/fstab where it would take effect on first boot. In that case the system becomes compressed over time as rpms are updated.

In fact, there might be an advantage to having separate install time and run time values. For production installations, compress=zstd:5 might make sense, i.e. a bit more aggressive. For Btrfs based images, like the workstation ARM images and possibly ISO's in lieu of squashfs, compress-force=11. We don't expect to have to implement this part in F34 (bonus if it's possible), merely going for an /etc/fstab entry would achieve the immediate need.

Further, it may be preferred to not apply to /boot/ when on (a separate) Btrfs, just to keep things simpler.

In practice, we still mount ro during boot, and the / fstab line's mount options are in effect remount options. The compress option works whether initial or remount, and applies file system wide even when combined with the subvol mount option. So yeah it's not a big loss to only apply to /etc/fstab and it just takes effect at first boot.

@cmurf
Copy link

cmurf commented Feb 11, 2021

Just for fun, tested this patch twice, as written and with compress=zstd. The initial mount does use compress=zstd:1 or compress=zstd but not the install time mount. I'm not sure why it only works once. And /etc/fstab lacks the compress option.

/tmp/storage.log:WARNING:blivet:Failed to reset SElinux context for newly mounted filesystem root directory to default.
/tmp/storage.log:INFO:program:Running... umount /tmp/btrfs-tmp.1262dyfgvtd
/tmp/storage.log:DEBUG:blivet:                          BTRFS._pre_setup: type: btrfs ; device: /dev/vda2 ; mountpoint: /tmp/btrfs-tmp.126xv89etme ;
/tmp/storage.log:INFO:program:Running... mount -t btrfs -o compress=zstd /dev/vda2 /tmp/btrfs-tmp.126xv89etme
/tmp/storage.log:WARNING:blivet:Failed to reset SElinux context for newly mounted filesystem root directory to default.
/tmp/storage.log:INFO:program:Running... umount /tmp/btrfs-tmp.126xv89etme
/tmp/storage.log:DEBUG:blivet:              BTRFS._pre_setup: type: btrfs ; device: /dev/vda2 ; mountpoint: / ;
/tmp/storage.log:INFO:program:Running... mount -t btrfs -o subvol=root /dev/vda2 /mnt/sysimage

@michel-slm
Copy link
Contributor Author

Abandoning this in favor of a pykickstart MountData change, that way every mounts that Anaconda would do should have the right options.

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

Successfully merging this pull request may close these issues.

None yet

4 participants