[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: btrfs compression by default



On Tue, 2021-02-16 at 15:05 +0100, Vendula Poncova wrote:
> On Tue, Feb 16, 2021 at 3:16 AM Michel Alexandre Salim
> <michel michel-slm name> wrote:
> > 
> > Hi Jiří, Vendula, all,
> > 
> > On Tue, 2021-02-09 at 12:12 +0100, Vendula Poncova wrote:
> > > On Tue, Feb 9, 2021 at 10:42 AM <jkonecny redhat com> wrote:
> > > > 
> > > > Hi everyone,
> > > > 
> > > > I see a few options for this. First is to add this directly to
> > > > blivet
> > > > library as you pointed out already. However, I don't think
> > > > blivet
> > > > developers would be happy about that because they are trying to
> > > > be as
> > > > much as possible general purpose library and this change is
> > > > really
> > > > just
> > > > about Anaconda.
> > > > 
> > > > Another option seems to be to modify Anaconda code directly.
> > > > Unfortunately, I can't tell from top of my mind how hard that
> > > > would
> > > > be
> > > > but still seems like the easiest solution.
> > > > 
> > > > However, the best approach which I can think of is to add
> > > > something
> > > > like that to our configuration files. Benefit would be that
> > > > another
> > > > modifications like that could be done easily in the future.
> > > > That is
> > > > of
> > > > course for a discussion for the Anaconda team because it could
> > > > be
> > > > pretty hard to implemented this in some common form.
> > > > 
> > > > Vendy, Vojta, do you have some better ideas or could you tell
> > > > us
> > > > something more about my ideas above?
> > > > 
> > > > Best Regards,
> > > > Jirka
> > > > 
> > > > On Tue, 2021-02-09 at 01:18 -0500, Neal Gompa wrote:
> > > > > On Tue, Feb 9, 2021 at 1:09 AM Michel Alexandre Salim
> > > > > <michel michel-slm name> wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Mon, 2021-02-08 at 21:02 -0700, Chris Murphy wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > This is in regards to this Fedora 34 change:
> > > > > > > https://fedoraproject.org/wiki/Changes/BtrfsTransparentCompression#Scope
> > > > > > > 
> > > > > > > The gist is to do 'mount -o compress=zstd:1' anytime
> > > > > > > Btrfs is
> > > > > > > used,
> > > > > > > whether Destination Installation>Automatic or Custom or
> > > > > > > Advanced-Custom. Apply it during the installation, and
> > > > > > > add it
> > > > > > > to
> > > > > > > /etc/fstab.
> > > > > > > 
> > > > > > > Somehow I got confused thinking that autopart supports --
> > > > > > > fsoptions,
> > > > > > > and that would be the way to do this. But (a) --fsoptions
> > > > > > > isn't
> > > > > > > supported with autopart, and (b) it wouldn't be a
> > > > > > > universal
> > > > > > > approach
> > > > > > > anyway. And we want this to be consistent. Now I'm
> > > > > > > thinking it
> > > > > > > needs
> > > > > > > to go somewhere in:
> > > > > > > 
> > > > > > > https://github.com/storaged-project/blivet/blob/3.4-devel/blivet/devices/btrfs.py
> > > > > > > 
> > > > > > > Am I on the right track, or does it need to go somewhere
> > > > > > > else,
> > > > > > > or
> > > > > > > in
> > > > > > > addition to that?
> > > > > > > 
> > > > > > (I'm one of the change owners, and trying to figure this
> > > > > > out with
> > > > > > Chris).
> > > > > > 
> > > > > > Ideally we have a solution that is configurable - i.e. this
> > > > > > is
> > > > > > exposed
> > > > > > via a kickstart command (and probably entailing changes in
> > > > > > Anaconda
> > > > > > and
> > > > > > pykickstart), but if that is not possible, or require a lot
> > > > > > of
> > > > > > rework
> > > > > > (which we can try to work on), if we are willing to carry a
> > > > > > patch
> > > > > > for
> > > > > > blivet or some other component to override the behavior
> > > > > > just for
> > > > > > Fedora
> > > > > > 34, what's the best way of achieving that?
> > > > > > 
> > > > > > Btrfs is probably the only filesystem with built-in
> > > > > > compression
> > > > > > that we
> > > > > > potentially care about, so designing an interface to expose
> > > > > > this
> > > > > > functionality might be an overkill -- but we're not sure.
> > > > > > 
> > > > > 
> > > > > Eventually, VDO might get integrated into the mainline tree,
> > > > > so
> > > > > having
> > > > > the interface which could be used for LVM compression through
> > > > > VDO
> > > > > wouldn't be too bad.
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > > Hello!
> > > 
> > > The change says: "On variants using btrfs as the default
> > > filesystem,
> > > enable transparent compression using zstd." It means that the
> > > compression has to be configurable per product, so we need to add
> > > a
> > > new configuration option to the Anaconda configuration files
> > > (something like btrfs_compression_enabled = True). I guess you
> > > planned
> > > to use interactive-defaults.ks, but we are going to replace this
> > > file
> > > with the configuration files anyway.
> > > 
> > > I have checked the Blivet's code and we might be able to redefine
> > > the
> > > _mount_class attribute of the BTRFS class. The mount class
> > > defines
> > > default mount options. That should be enough to fix all
> > > partitioning
> > > methods. What do you think, Vojta?
> > > 
> > I'm trying to figure this out, and am currently a bit stuck:
> > 
> > - patching devicetree/fsset.py's `fstab` to ensure fstab contains
> > compress=zstd:1 works, but the change only affects fstab on the
> > installed system, so during installation compression is not enabled
> > - patching devicetree/handler.py's `mount_options` to inject
> > compress=zstd:1 does not seem to do anything
> > https://github.com/rhinstaller/anaconda/compare/master...michel-slm:f34-34.23.1-btrfs-compression?expand=1
> > 
> > - patching devicetree/handler.py's `get_device_mount_options` to
> > ensure
> > btrfs filesystems area always mounted with compress=zstd:1 does not
> > work as expected (patching `set_device_mount_options` does not do
> > anything either):
> > https://github.com/rhinstaller/anaconda/compare/master...michel-slm:f34-34.23.1-btrfs1?expand=1
> > 
> > 
> > Since I don't really want to hardcode this long-term (hardcoding in
> > a
> > proof-of-concept, or for the F34 release only, is not too bad) --
> > is
> > Blivet the place to do this? What I had in mind is:
> > 
> > - Some product definition in data/product.d specifies compression
> > should be enabled
> > - When that is enabled, Anaconda overrides the _mount_class of the
> > BTRFS class it gets from Blivet
> > 
> > ^ where in Anaconda's codebase can I do this? Trying to follow this
> > in
> > the code but it's not easy especially since there does not seem to
> > be
> > an example of another filesystem conditionally overriding the
> > default
> > mount options.
> 
> Hello,
> 
> I have talked to Vojta and we suggest the following solution:
> 
> 1. Open a pull request with [0] in the upstream.
> 2. Add a new flag to blivet.flags for enabling the compression of
> btrfs. It should be False by default. Modify [0] to respect the flag.
> 3. Open a pull request that sets up the new flag in Anaconda.
> Anaconda
> sets the Blivet's flags in [1].
> 4. If you want, add a new option to the [Storage] section of the
> Anaconda configuration file that will control the new flag. It should
> be False by default. Set this option in the relevant product
> configuration files located at [2].
> 
> [0] https://src.fedoraproject.org/rpms/python-blivet/pull-request/8
> [1] 
> https://github.com/rhinstaller/anaconda/blob/master/pyanaconda/modules/storage/initialization.py#L50
> [2] 
> https://github.com/rhinstaller/anaconda/tree/master/data/product.d
> 
> Vendy
> 
Thanks Vendy!

Per the discussion in IRC, going to follow the plan you outlined:
1. I'll modify my patch to make it accept a flag - btrfs_compression,
which can be either False (or a falsy value) or a string with the
compression option
2. I'll work on an Anaconda patch to set that flag
3. Test, if it works, submit both as PRs (hopefully today so it can be
reviewed tomorrow CET)
4. Agreed that exposing this in Anaconda configuration can be done
later, after the first two patches are in first. Will try this as a
stretch goal if we have time

Thanks all,

-- 
Michel Alexandre Salim
profile: https://keyoxide.org/michel michel-slm name
chat via email: https://delta.chat/
GPG key: 5DCE 2E7E 9C3B 1CFF D335 C1D7 8B22 9D2F 7CCC 04F2

Attachment: signature.asc
Description: This is a digitally signed message part


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]