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.

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

Jump to bottom

cp: fix possible OOM and partial write with large files #6694

Merged
merged 1 commit into from
Oct 15, 2024
Merged

cp: fix possible OOM and partial write with large files #6694

merged 1 commit into from
Oct 15, 2024

Conversation

neyo8826
Copy link
Contributor

There are 2 fixes:

  • pwrite does not guarantee that all bytes will be written, write_all_at is a proper wrapper over it
  • sparse_copy_without_hole (triggered on ZFS) can eat RAM if file is large and there are not enough holes; now the write happens at max 16 MiB chunks (as far as I tested, it saturates SSD anyways without much CPU)

I have tested it with checking sha256sum of a 4 GiB file.

sylvestre
sylvestre reviewed Sep 13, 2024
View reviewed changes
src/uu/cp/src/platform/linux.rs Outdated
@@ -141,6 +141,8 @@ where
}
let src_fd = src_file.as_raw_fd();
let mut current_offset: isize = 0;
let step = std::cmp::min(size, 16 * 1024 * 1024) as usize; // 16 MiB
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment to explain why you are doing this

sylvestre
sylvestre reviewed Sep 13, 2024
View reviewed changes
src/uu/cp/src/platform/linux.rs
current_offset.try_into().unwrap(),
)
};
for i in (0..len).step_by(step) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same, please document this a bit more :) (i know it wasn't documented before :)

@sylvestre
Copy link
Contributor

thanks
did you do some benchmarking to see the impact on perf?
https://github.com/sharkdp/hyperfine/ is great for this

@neyo8826
Copy link
Contributor Author

More docs :)

I have "benchmarked" it with my large file (technically not sparse, but heuristic triggered it I suppose).
I was slower even before implementing the chunking because now it actually writes the whole file (thank god for the checksumming in my build tool, that's how I discovered the bug).
After adding the chunking, the performance stayed the same, it was IO saturated (GCP high perf SSD)
For small files it should behave the same with 1 read and write call.

@sylvestre
Copy link
Contributor

any idea of the impact on slow harddrive ?

@neyo8826
Copy link
Contributor Author

Well, if the copy is between 2 HDDs, then it shouldnt matter.
If they are on the same, there will be more head movement, and the write cache will matter a lot (8MB to 1GB?)
We do not sync manually, so even the FS cache will help.
I would like to see a proper benchmark with both consumer and enterprise HDDs, but it is not possible for me right now.

16MB seems a lot, it should balance out anything with the FS help (just my opinion)

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/cp/sparse-2. tests/cp/sparse-2 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cp/sparse-extents-2. tests/cp/sparse-extents-2 is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
GNU test failed: tests/cp/sparse-extents-2. tests/cp/sparse-extents-2 is passing on 'main'. Maybe you have to rebase?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 6, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@sylvestre
Copy link
Contributor

do you know how I could reproduce the OOM ? thanks

@neyo8826
Copy link
Contributor Author

neyo8826 commented Oct 7, 2024

do you know how I could reproduce the OOM ? thanks

create a file and copy it with sparse_copy_without_hole
on my system it was automatically triggered by zfs it seems
make the file larger than system memory + swap without any actual holes
then let mut buf: Vec<u8> = vec![0x0; len as usize]; wants to be allocated (former line 161)

BenWiederhake
BenWiederhake approved these changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

LGTM!

Bonus points if you can come up with a way to test this. For example: Create a 500 MB "hole-only" file, and copy it while a ulimit -m 300000 applies.

Please let me know if you intend to write such a test or not.

@neyo8826
Copy link
Contributor Author

A hole-only file would not trigger any actual reads or writes because of SEEK_DATA. As for a technically hole-less file it should copy it by 16 MiB chunks. I have no idea how to test them in a filesystem independent way. Maybe a mock fs? (out of scope for me)

BenWiederhake
BenWiederhake approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Ideally someone should eventually™ come up with a way to test this (without introducing CI flakes), but this is good enough for now. Thanks! :)

@BenWiederhake BenWiederhake merged commit 1781c3e into uutils:main Oct 15, 2024
68 checks passed
@neyo8826 neyo8826 deleted the copy_fix branch October 17, 2024 05:59
@BrewTestBot BrewTestBot mentioned this pull request Nov 16, 2024
Merged
@moonfruit moonfruit mentioned this pull request Nov 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

@BenWiederhake BenWiederhake BenWiederhake approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@neyo8826 @sylvestre @BenWiederhake