Skip to content

Filesystem optimizations #67

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Filesystem optimizations #67

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c02e552
upd(unikernel): reduce initrd image size
matthewjmarangoni Sep 8, 2025
aa6153b
Merge branch 'main' into upd-fs-optimizations
matthewjmarangoni Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion images/chromium-headful/build-unikernel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ docker rm cnt-"$app_name" || true
docker create --platform linux/amd64 --name cnt-"$app_name" "$IMAGE" /bin/sh
docker cp cnt-"$app_name":/ ./.rootfs
rm -f initrd || true
sudo mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs
sudo mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs
Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Logic

The -x 0 parameter being added here should be documented or explained. The -x flag in mkfs.erofs typically controls xattr features, and setting it to 0 disables extended attribute support. This could impact applications that rely on extended attributes for security contexts, capabilities, or other metadata. Consider documenting why this is being disabled and whether it's intentional for the optimization goals.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Performance

The compression block size of 131072 (128KB) is quite large and may increase memory usage during filesystem operations. Consider documenting the minimum EROFS-utils version required (1.6+) for the -C and -z flags, or add version validation to prevent build failures on older systems.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Low Style

The parameter ordering could be improved for readability. Consider grouping related parameters together, e.g., placing compression-related options (-C, -z) adjacent to each other: sudo mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -z lz4 -x 0 initrd ./.rootfs
Agent: 🤖 General


# Package the unikernel (and the new initrd) to KraftCloud
kraft pkg \
Expand Down
2 changes: 1 addition & 1 deletion images/chromium-headless/build-unikernel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ docker rm cnt-"$app_name" || true
docker create --platform linux/amd64 --name cnt-"$app_name" "$IMAGE" /bin/sh
docker cp cnt-"$app_name":/ ./.rootfs
rm -f initrd || true
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs
Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Logic

Inconsistency detected: This command doesn't use sudo while the equivalent command in images/chromium-headful/build-unikernel.sh does use sudo. Both commands create the same initrd file and use identical parameters. This inconsistency could lead to permission issues or different behavior between headful and headless builds. Consider standardizing the approach - either both should use sudo or neither should, depending on the execution environment and file permissions needed.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Low Performance

The compression block size -C 131072 (128KB) is quite large and may not be optimal for all use cases. While this can improve compression ratios for large files, it can hurt performance for small file access patterns common in web applications. The PR mentions this is experimental and needs further testing - consider adding a comment explaining the rationale for this specific block size choice and making it configurable via an environment variable for easier testing of different values.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Logic

Inconsistency detected: The headful version uses 'sudo' for mkfs.erofs but this headless version doesn't. This could lead to permission issues depending on the build environment. Consider aligning the privilege requirements between both scripts.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Logic

Consider adding error handling to validate that the EROFS-utils version supports the newer flags (-C, -z) before executing. The build will fail silently on systems with EROFS-utils < 1.6, making debugging difficult.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Logic

Consider adding error handling for the mkfs.erofs command. Since this is a critical step in the build process and the new parameters could potentially fail with older EROFS versions, adding || { echo "EROFS filesystem creation failed"; exit 1; } would provide better error reporting.
Agent: 🤖 General

Copy link

@mesa-dot-dev mesa-dot-dev bot Sep 10, 2025

Choose a reason for hiding this comment

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

Medium Logic

Inconsistency: The headful version uses sudo for mkfs.erofs while this headless version doesn't. This could lead to different permission behaviors or failures depending on the execution environment. Consider either adding sudo here or documenting why it's not needed for the headless build.
Agent: 🤖 General


kraft pkg \
--name $UKC_INDEX/$IMAGE \
Expand Down
Loading