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

Make ext4 rootfs great again #3830

Merged
merged 8 commits into from
Apr 10, 2024
Merged

Conversation

rouming
Copy link
Contributor

@rouming rouming commented Mar 21, 2024

This set of patches fixes the ext4 rootfs generation. Main problem was in the image size difference between squashfs image and ext4 which leads to an incorrect GPT table. The solution is to calculate resulting image dynamically.

Patch makes creation of the ext4 rootfs possible and also has a size reduction for the installer.raw images:

                      before      after
     installer.raw     592Mb      296Mb

Unfortunately for the rootfs we have a hardcoded partition size equal to 512Mb, which can't be changed, otherwise upgrade logic fail when we try to fit bigger rootfs into smaller one partition. This means that upgrade from squashfs image to ext4 is currently impossible. But who cares.

Now live image can be generated as following:

  make ROOTFS_FORMAT=ext4 live

My motivation to generate ext4 rootfs is to have possibility to persist changes on eve while running.
Another great feature becomes possible is to "patch" resulting qemu image with changed files and not generate rootfs from scratch, which can take minutes of CPU time.

This patch changes the pkg/mkverification-raw-efi/make-raw, which almost a copy of the original one. I kindly ask @jsfakian to help me to review and to test the resulting verification images, that those work and nothing is broken because of my changes.

cc: @mikem-zed

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (7859da6) to head (c931159).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3830   +/-   ##
=======================================
  Coverage   17.51%   17.51%           
=======================================
  Files           3        3           
  Lines         805      805           
=======================================
  Hits          141      141           
  Misses        629      629           
  Partials       35       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rouming rouming changed the title Make ext4 generation again possible for the rootfs Make ext4 rootfs great again Mar 21, 2024
@rouming rouming force-pushed the rootfs-as-ext4 branch 5 times, most recently from 8297582 to 7d9372e Compare March 21, 2024 16:34
Makefile Show resolved Hide resolved
@github-actions github-actions bot requested a review from jsfakian March 22, 2024 09:21
Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the near-duplication of make-raw in both mkimage-raw-efi and mkverification-raw-efi, but that wasn't introduced here, so not your job to fix here. :-)

In any case, as long as this all works well and is tested, looks good for what it does. I like that it calculates size dynamically.

@deitch
Copy link
Contributor

deitch commented Mar 22, 2024

Another great feature becomes possible is to "patch" resulting qemu image with changed files and not generate rootfs from scratch, which can take minutes of CPU time

How does that work?

@github-actions github-actions bot requested a review from deitch March 22, 2024 13:20
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the impact on the current squashfs installer and live image from the dynamic size?
I can't tell whether the partition sizes will be dynamic or fixed with these changes.

@eriknordmark
Copy link
Contributor

What's the impact on the current squashfs installer and live image from the dynamic size? I can't tell whether the partition sizes will be dynamic or fixed with these changes.

Answering my own question, the partition size (at least in the live image) is the size of the image.
linuxkit-525400123456:~# lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINTS
sda 8:0 0 64G 0 disk
├─sda1 8:1 0 36M 0 part
├─sda2 8:2 0 232M 0 part /
├─sda3 8:3 0 232M 0 part
├─sda4 8:4 0 1M 0 part
└─sda9 8:9 0 63.5G 0 part /persist

That means it can't be used to update to a slightly larger image.

Can't we fix that to use the same fixed size for the partitions?

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cloned this workspace and tried what works on master, and it fails to boot the installer.

My invocation is:
make -e QEMU_MEMORY=16384 MEDIA_SIZE=65536 SSH_PORT=2345 installer-raw run-installer-raw run-target

A few minor things done here:

1. /mnt folder should be created, otherwise mount fails.
2. Proper round up on 1kb.
3. Remove unused variable ROOTFS_PART_SECTORS

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
@rouming
Copy link
Contributor Author

rouming commented Apr 2, 2024

@eriknordmark

That means it can't be used to update to a slightly larger image.
Can't we fix that to use the same fixed size for the partitions?

This is very good point, I forgot about upgrade. Actually without borrowing free space from the /persist we are screwed by default to upgrade to image greater than these hardcoded values. So in the future if image grows this has to be fixed by doing dangerous operations on the /persist. Now I can fix that by returning old values and taking max.

I cloned this workspace and tried what works on master, and it fails to boot the installer.

This was fixed just before your comment, here https://github.com/lf-edge/eve/compare/8ab4fde0385564de8f57a560a255a6d8bed92179..21b71376ddaa6789ee9303e52b63c737743f2c9d
Please checkout once again.

rouming added 2 commits April 2, 2024 11:55
Easy to show that long alignment equation can be simplified to
a shorter 1MB alignment:

    x + 2^10 - 1
    ------------  + 2^10 - 1      x + 2^20 - 1
        2^10                  =   ------------
    ------------------------          2^20
             2^10

Take the left side and do a simplification

    x + 2^10 - 1     2^10 - 1
    ------------  +  --------
        2^20           2^10

    x + 2^10 - 1 + 2^10(2^10 - 1)
  = -----------------------------
               2^20

    x + 2^10 - 1 + 2^20 - 2^10
  = --------------------------
               2^20

    x + 2^20 - 1
  = ------------
       2^20

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
! Patch covers two almost identical 'make-raw' files.

The purpose of the change is to access those variables earlier
from functions, which will appear in following patches.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
rouming added 5 commits April 2, 2024 11:55
! Patch covers two almost identical 'make-raw' files.

Check for IMX8 files repeats several times. Introduce a separate 'imx8_exists()'
function for this purpose.

Note! The 'make-raw' in the verification package is a bit outdated, so IMX8
blobs are copied to the /efifs folder. Originally this was introduced in
the following commit:

  b8f73a8 ("arm: Add support to install EVE image on eMMC for i.MX8MP devices")

but was not ported to the verification 'make-raw'. This patch does that.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
! Patch covers two almost identical 'make-raw' files.

The 'make-raw' script expects the resulting image being created by the
caller. The main problem of this approach is that the caller should
create an image in advance without knowing exactly the size of
partitions, which will be created lately. This causes a corruption of
the image with the ext4 rootfs, which is bigger than usual squashfs
and does not fit ~500Mb.

This patch fixes the problem by calculating resulting image size
by iterating over each requested partitions, e.g. 'imga' or 'imgb'
partitions are created by requesting size of the actual rootfs binary
image.

Having this calculations in place a caller does not need to predict
the resulting image size, but simply pass the '-C' argument, which
means the image file will be created according to the size of the
partitions.

Keeping the upgrade logic in mind we can't unfortunately change the
squashfs (default) rootfs partition size, so introduce minimum rootfs
size, which is equal to the default size of 512Mb.

We always use the maximum:

    max(512MB, file_size $ROOTFS_IMG)

so that default squashfs size won't be change, but ext4 will grow
(and yes, unfortunately you can't upgrade from squashfs to ext4,
 because ext4 partition is a way bigger)

Patch makes creation of the ext4 rootfs possible and also has a
size reduction for the installer.raw image:

                  before      after
 installer.raw     592Mb      296Mb

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
…tion

This patch removes creation of the image in the 'makeflash.sh' by passing
'-C' option to the 'mkimage-raw-efi' and 'mkverification-raw-efi' in order
to use dynamic image calculation.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
In order to generate live/verification-raw/installer-raw images
with the ext4 as rootfs call as the following:

   make ROOTFS_FORMAT=ext4 live
   (replace 'live' with 'verification-raw' or 'installer-raw')

Also disable warning for ext4 if image size exceeds 250Mb.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Reflect '-C' option changes: now image size is calculted dynamically,
so in order to ask mkimage-raw-efi to create an image pass '-C' only.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
@rouming
Copy link
Contributor Author

rouming commented Apr 2, 2024

@eriknordmark @jsfakian
Folks, now installer and verification images should work. I would appreciate if you offer me a hand in testing this.

Also the rootfs partition size is kept as is, i.e. 512Mb, so upgrade won't fail. This is for the imga, imgb partitions only, the installer/verificator keeps rootfs as a file on fat32, so we don't keep this max value.

@rouming rouming mentioned this pull request Apr 4, 2024
@rouming
Copy link
Contributor Author

rouming commented Apr 4, 2024

Another great feature becomes possible is to "patch" resulting qemu image with changed files and not generate rootfs from scratch, which can take minutes of CPU time

How does that work?

@deitch here #3842. Can we continue improving? Still a lot of room for improvements, but help of the linuxkit is required.

@deitch
Copy link
Contributor

deitch commented Apr 4, 2024

This needs to be approved and merged before #3842

@deitch
Copy link
Contributor

deitch commented Apr 4, 2024

Also the rootfs partition size is kept as is, i.e. 512Mb, so upgrade won't fail. This is for the imga, imgb partitions only, the installer/verificator keeps rootfs as a file on fat32, so we don't keep this max value.

So this is ready to go?

Still a lot of room for improvements, but help of the linuxkit is required

For this PR? Or for next steps you want in #3842? Or for something else beyond that?

@rouming
Copy link
Contributor Author

rouming commented Apr 4, 2024

Also the rootfs partition size is kept as is, i.e. 512Mb, so upgrade won't fail. This is for the imga, imgb partitions only, the installer/verificator keeps rootfs as a file on fat32, so we don't keep this max value.

So this is ready to go?

This is ready to go. There was some important concern from @eriknordmark , I fixed that. If he does not have any objections or anyone else, this can be merged. Also would be great if @jsfakian can help me with verification image. Last time he spotted one thing, which breaks the image. This is also fixed.

Still a lot of room for improvements, but help of the linuxkit is required

For this PR? Or for next steps you want in #3842? Or for something else beyond that?

The #3842 goes on top of the ext4 (this PR). Nothing more. The #3842 requires more eyes and testing and doc.

@deitch
Copy link
Contributor

deitch commented Apr 4, 2024

Yeah, I see @eriknordmark still has changes pending. Let's let him sign off and merge it in. Hats off @rouming

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but i haven't had time to retry my make test

@eriknordmark eriknordmark merged commit 3688da0 into lf-edge:master Apr 10, 2024
29 of 32 checks passed
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.

4 participants