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

Fix nativeimgutil.ConvertToRaw() #2718

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Conversation

nirs
Copy link
Contributor

@nirs nirs commented Oct 10, 2024

  • Truncate before copying the data to eliminate the seeks during the
    copy. This also provides a hint to the file system that can minimize
    allocations and fragmentation of the file.

  • Avoid unneeded seeks during copy using WriteAt. This does not improve
    performance since practically all time is spent on reading from the
    source image.

  • Fix zero detection to handle short reads. Previously we would compare
    the entire buffer which can contain non-zero bytes from the previous
    read.

  • Fix write to handle short reads. Previously we would write the entire
    buffer including data from previous read, corrupting the image.

  • Fix error message for failed write. Looks like it was copied from the
    read branch.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Oct 11, 2024

Thanks, this seems to work, but the output is confusing

INFO[0000] Converting "/Users/suda/.lima/alpine/basedisk" (qcow2) to a raw disk "/Users/suda/.lima/alpine/diffdisk" 
INFO[0000] Expanding to 100GiB                          
212.00 MiB / 212.00 MiB [---------------------------------] 100.00% 115.06 MiB/s

It should be like

Expanding ...
Converting ...
[Progress bar]

@nirs
Copy link
Contributor Author

nirs commented Oct 11, 2024

Thanks, this seems to work, but the output is confusing

INFO[0000] Converting "/Users/suda/.lima/alpine/basedisk" (qcow2) to a raw disk "/Users/suda/.lima/alpine/diffdisk" 
INFO[0000] Expanding to 100GiB                          
212.00 MiB / 212.00 MiB [---------------------------------] 100.00% 115.06 MiB/s

It should be like

Expanding ...
Converting ...
[Progress bar]

But this does not work for the raw-to-raw case, which must expand the image as a second step. I think we should keep the same flow since it is simple, easy to follow, and make it easy to verify that the convert was correct before expanding.

So I plan to do this:

  1. truncate to source size
  2. convert using WriteAt avoiding the seeks
  3. expand

@AkihiroSuda
Copy link
Member

Do you plan to update this PR or open a follow-up PR separately ?

@nirs
Copy link
Contributor Author

nirs commented Oct 13, 2024

Do you plan to update this PR or open a follow-up PR separately ?

I can update it this weekend.

- Truncate before copying the data to eliminate the seeks during the
  copy. This also provides a hint to the file system that can minimize
  allocations and fragmentation of the file.

- Avoid unneeded seeks during copy using WriteAt. This does not improve
  performance since practically all time is spent on reading from the
  source image.

- Fix zero detection to handle short reads. Previously we would compare
  the entire buffer which can contain non-zero bytes from the previous
  read.

- Fix write to handle short reads. Previously we would write the entire
  buffer including data from previous read, corrupting the image.

- Fix error message for failed write. Looks like it was copied from the
  read branch.

Signed-off-by: Nir Soffer <[email protected]>
@nirs
Copy link
Contributor Author

nirs commented Oct 14, 2024

@AkihiroSuda new version keeps the same user interface:

% limactl create --tty=false --plain 
INFO[0000] Terminal is not available, proceeding without opening an editor 
INFO[0000] Attempting to download the image              arch=aarch64 digest="sha256:5ecac6447be66a164626744a87a27fd4e6c6606dc683e0a233870af63df4276a" location="https://cloud-images.ubuntu.com/releases/24.04/release-20240821/ubuntu-24.04-server-cloudimg-arm64.img"
Downloading the image (ubuntu-24.04-server-cloudimg-arm64.img)
551.31 MiB / 551.31 MiB [----------------------------------] 100.00% 57.27 MiB/s
INFO[0010] Downloaded the image from "https://cloud-images.ubuntu.com/releases/24.04/release-20240821/ubuntu-24.04-server-cloudimg-arm64.img" 
INFO[0010] Converting "/Users/nsoffer/.lima/default/basedisk" (qcow2) to a raw disk "/Users/nsoffer/.lima/default/diffdisk" 
3.50 GiB / 3.50 GiB [-------------------------------------] 100.00% 199.27 MiB/s
INFO[0028] Expanding to 100GiB                          
INFO[0028] Run `limactl start default` to start the instance.

% qemu-img info ~/.lima/default/diffdisk 
image: /Users/nsoffer/.lima/default/diffdisk
file format: raw
virtual size: 100 GiB (107374182400 bytes)
disk size: 1.71 GiB
Child node '/file':
    filename: /Users/nsoffer/.lima/default/diffdisk
    protocol type: file
    file length: 100 GiB (107374182400 bytes)
    disk size: 1.71 GiB

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 9705b66 into lima-vm:master Oct 15, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants