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

Evicting too many bytes from MFU metadata #16546

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

tkittich
Copy link
Contributor

Motivation and Context

I am not so sure about this PR. Consider this as a small bug report. ^^"
From reading the code, it seems some bytes has been evicted from MRU metadata so it probably should be subtracted from m.

How Has This Been Tested?

Has not been tested.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I think you are right here. My fault. Without updating m there we evict from MFU metadata all that we wanted to evict from all metadata, including already evicted MRU metadata (m is the total amount of metadata we had at the beginning, and w is the total amount of metadata we want to have).

Since you already have reproduction for the issue, could you test that it behaves better?

@behlendorf behlendorf self-requested a review September 19, 2024 19:40
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 19, 2024
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Good find, yes I believe you're right. If you could provide some of your test results that would be very helpful.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 20, 2024
@behlendorf behlendorf merged commit d40d409 into openzfs:master Sep 24, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants