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

Log the real reason for why posix_fadvise failed #430

Merged
merged 1 commit into from
May 3, 2024

Conversation

TedLyngmo
Copy link
Contributor

reclaimFilePageCache did not set errno but rdbSaveInternal which is logging the error assumed it did. This makes sure errno is set.

Fixes #429

`reclaimFilePageCache` did not set `errno` but `rdbSaveInternal` which
is logging the error assumed it did. This makes sure `errno` is set.

Fixes valkey-io#429

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 68.45%. Comparing base (5b1fd22) to head (babd006).

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable     #430   +/-   ##
=========================================
  Coverage     68.44%   68.45%           
=========================================
  Files           109      109           
  Lines         61671    61673    +2     
=========================================
+ Hits          42209    42216    +7     
+ Misses        19462    19457    -5     
Files Coverage Δ
src/util.c 69.49% <33.33%> (-0.22%) ⬇️

... and 11 files with indirect coverage changes

Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Makes sense to me, given https://man7.org/linux/man-pages/man2/posix_fadvise.2.html.

Did you encounter this issue while running in production or just reviewing code. This will influence whether or not we backport the fix.

@madolson madolson added the release-notes This issue should get a line item in the release notes label May 3, 2024
@madolson madolson merged commit 8f97448 into valkey-io:unstable May 3, 2024
17 checks passed
@TedLyngmo
Copy link
Contributor Author

TedLyngmo commented May 4, 2024

Did you encounter this issue while running in production or just reviewing code. This will influence whether or not we backport the fix.

I noticed it in our test environment when I tried tracking down reasons for misc. "Resource temporarily unavailable" issues - so I don't really know the real reason for why posix_fadvise failed.

I also added a PR in Redis: redis/redis#13246

@TedLyngmo TedLyngmo deleted the fix_429 branch May 4, 2024 15:37
@madolson
Copy link
Member

madolson commented May 4, 2024

OK, sounds like enough reason to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Wrong error logged when reclaimFilePageCache fails
2 participants