Skip to content

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
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 11, 2024
Without updating 'm' 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). 

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Theera K. <[email protected]>
Closes openzfs#16521
Closes openzfs#16546
ptr1337 pushed a commit to CachyOS/zfs that referenced this pull request Nov 14, 2024
Without updating 'm' 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). 

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Theera K. <[email protected]>
Closes openzfs#16521
Closes openzfs#16546
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Jan 26, 2025
Without updating 'm' 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). 

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Theera K. <[email protected]>
Closes openzfs#16521
Closes openzfs#16546
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