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 pre-allocation when changing priority #7738

Merged
merged 2 commits into from
Sep 30, 2024
Merged

fix pre-allocation when changing priority #7738

merged 2 commits into from
Sep 30, 2024

Conversation

arvidn
Copy link
Owner

@arvidn arvidn commented Sep 29, 2024

based on @HanabishiRecca 's research and fix. set m_file_priority early, to make open_file() work as expected. If there's a failure, restore it.

#7733

…ly, to make open_file() work as expected. If there's a failure, restore it
@@ -161,15 +162,22 @@ error_code translate_error(std::error_code const& err, bool const write)

download_priority_t const old_prio = m_file_priority[i];
download_priority_t new_prio = prio[i];

m_file_priority[i] = new_prio;
Copy link
Contributor

@HanabishiRecca HanabishiRecca Sep 29, 2024

Choose a reason for hiding this comment

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

Again, I'm not a C++ guy, but this looks sketchy to me. Also considering that new_prio is not const.
Why do we need such intermediate variable?

Copy link
Owner Author

Choose a reason for hiding this comment

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

it's not necessary. but I like giving it an intuitive name. it probably should be const, but it's a copy either way.

@arvidn
Copy link
Owner Author

arvidn commented Sep 30, 2024

while working on a test for this, I realized that the allocate_files feature doesn't really make any difference on posix systems where files are sparse by default. on windows it makes the files be created without the sparse flag, so they are allocated. Maybe it would make sense to have the same behavior on posix.

@HanabishiRecca
Copy link
Contributor

Posix storage is not affected because it does not use this pref in the first place, and open_file does not access m_file_priority anywhere.
But for the sake of consistency, I guess the same change could be useful there too.

@arvidn
Copy link
Owner Author

arvidn commented Sep 30, 2024

I was wrong. files are actually allocated on non-windows when the sparse flag isn't set.
The test fails without the fix

@HanabishiRecca
Copy link
Contributor

HanabishiRecca commented Sep 30, 2024

I have completely lost the thread of discussion. What do you mean by posix systems?

File preallocation does work on Linux (using mmap storage), that's the platform I use and test things on.
Not setting the sparse flag leads to result similar to fallocate.

Comment on lines +856 to +863
/*
TORRENT_TEST(test_pre_allocate_posix)
{
test_pre_allocate<posix_storage>();
}
*/

Check notice

Code scanning / CodeQL

Commented-out code Note test

This comment appears to contain commented-out code.
@arvidn arvidn force-pushed the pre-alloc-fix branch 5 times, most recently from ae4003c to 5c24565 Compare September 30, 2024 14:59
@arvidn arvidn merged commit 5d5eeef into RC_2_0 Sep 30, 2024
42 checks passed
@arvidn arvidn deleted the pre-alloc-fix branch September 30, 2024 17:34
@arvidn
Copy link
Owner Author

arvidn commented Sep 30, 2024

thanks @HanabishiRecca !

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.

2 participants