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

Unify arc_prune_async() code #15456

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Unify arc_prune_async() code #15456

merged 1 commit into from
Oct 30, 2023

Conversation

amotin
Copy link
Member

@amotin amotin commented Oct 26, 2023

There is no sense to have separate implementations for FreeBSD and Linux. Make Linux code shared as more functional and just register FreeBSD-specific prune callback with arc_add_prune_callback() API.

Aside of code cleanup this should fix excessive pruning on FreeBSD:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274698

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:

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Oct 26, 2023
@amotin
Copy link
Member Author

amotin commented Oct 26, 2023

@mjguzik @markjdb Would you take a look please.

There is no sense to have separate implementations for FreeBSD and
Linux.  Make Linux code shared as more functional and just register
FreeBSD-specific prune callback with arc_add_prune_callback() API.

Aside of code cleanup this should fix excessive pruning on FreeBSD:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274698

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@mjguzik
Copy link
Contributor

mjguzik commented Oct 26, 2023

If you are trying to do clean ups I think first step is to remove obsolete compat for older FreeBSD versions, if present in affected code.

12.4 is EOLing later this year, 13.1 is already EOLed.

13.2 ships with 1302001, so any test for a value lower than that can be just whacked.

@amotin
Copy link
Member Author

amotin commented Oct 26, 2023

@mjguzik ZFS release notes tell that all releases since 12.2-RELEASE are supported. It does not hurt much to keep it. Some day -- sure.

@grahamperrin

This comment was marked as resolved.

@amotin
Copy link
Member Author

amotin commented Oct 27, 2023

Should MFC flags apply?

@grahamperrin Sure, it could be merged into OpenZFS 2.2 branch and may be even 2.1, respectively FreeBSD 14 and may be 13.

Blocking 14.0r, or not relevant? https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=14.0r

I've never seen the problem myself, and it is not fatal, but I'll leave it to somebody who has workload triggering it to speak. IIRC 14.0 is in the last RC stages, so I am not eager to cause too much disruption unless there is something else.

@markjdb
Copy link
Contributor

markjdb commented Oct 27, 2023

I've never seen the problem myself, and it is not fatal, but I'll leave it to somebody who has workload triggering it to speak. IIRC 14.0 is in the last RC stages, so I am not eager to cause too much disruption unless there is something else.

It's not fatal, but it's somewhat disruptive. If I'm running a CPU intensive workload, a user thread may be assigned to the CPU where arc_prune_task() is running, but then it never gets to run. The problem resolves itself when ULE's load balancer runs, but that can take a few seconds. It can cause my desktop to hang occasionally.

It would be nice to have this in 14.0 but it's too late. I can take care of issuing an EN.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 30, 2023
@behlendorf behlendorf merged commit 799e09f into openzfs:master Oct 30, 2023
25 checks passed
amotin added a commit to amotin/zfs that referenced this pull request Nov 6, 2023
There is no sense to have separate implementations for FreeBSD and
Linux.  Make Linux code shared as more functional and just register
FreeBSD-specific prune callback with arc_add_prune_callback() API.

Aside of code cleanup this should fix excessive pruning on FreeBSD:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274698

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Mark Johnston <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15456
@amotin amotin deleted the prune branch November 6, 2023 18:28
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 20, 2023
There is no sense to have separate implementations for FreeBSD and
Linux.  Make Linux code shared as more functional and just register
FreeBSD-specific prune callback with arc_add_prune_callback() API.

Aside of code cleanup this should fix excessive pruning on FreeBSD:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274698

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Mark Johnston <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15456
@mmatuska
Copy link
Contributor

@amotin shouldn't this be a candidate for zfs-2.1-release as well?

lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
There is no sense to have separate implementations for FreeBSD and
Linux.  Make Linux code shared as more functional and just register
FreeBSD-specific prune callback with arc_add_prune_callback() API.

Aside of code cleanup this should fix excessive pruning on FreeBSD:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274698

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Mark Johnston <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15456
@OlCe2
Copy link
Contributor

OlCe2 commented Apr 9, 2024

Excessive ARC pruning is still a very annoying problem in FreeBSD 13 (see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=275594).

@amotin Can this change be backported to OpenZFS 2.1.x? In the meantime, I'm planning to soon commit the simpler change: https://reviews.freebsd.org/D44170 to FreeBSD's copy, which alternatively could be committed to OpenZFS 2.1.x if you find it less disruptive than the one in this pull request.

@mmatuska I assume you're going to continue to merge new releases from the OpenZFS 2.1.x branch to FreeBSD's stable/13, even if no new release is cut from the latter?

Thanks.

@amotin
Copy link
Member Author

amotin commented Apr 11, 2024

@OlCe2 Here: #16083

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.

7 participants