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.

Sign up for GitHub

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

Jump to bottom

Skip dnode handles use when not needed #16374

Closed
wants to merge 2 commits into from
Closed

Skip dnode handles use when not needed #16374

wants to merge 2 commits into from

Conversation

amotin
Copy link
Member

@amotin amotin commented Jul 22, 2024

Neither FreeBSD nor Linux currently implement kmem_cache_set_move(), which means dnode_move() is never called. In such situation use of dnode handles with respective locking to access dnode from dbuf is a waste of time for no benefit.

This patch implements optional simplified code for such platforms, saving at least 3 dnode lock/dereference/unlock per dbuf life cycle. Originally I hoped to drop the handles completely to save memory, but they are still used in dnodes allocation code, so left for now.

Before this change in CPU profiles of some workloads I saw 4-20% of CPU time spent in zrl_add_impl()/zrl_remove(), which are gone now.

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 added 2 commits July 21, 2024 21:04
@amotin
Cleanup DB_DNODE() macros usage
d33e2ca 
 - Use the macros in few places it was missed.
 - Reduce scope of DB_DNODE_ENTER/EXIT() and inline some DB_DNODE()
uses to make it more obvious what exactly is protected there and
make unprotected accesses by mistake more difficult.
 - Make use of zrl_owner().

Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
@amotin
Skip dnode handles use when not needed
64341ea 
Neither FreeBSD nor Linux currently implement kmem_cache_set_move(),
which means dnode_move() is never called.  In such situation use of
dnode handles with respective locking to access dnode from dbuf is
a waste of time for no benefit.

This patch implements optional simplified code for such platforms,
saving at least 3 dnode lock/dereference/unlock per dbuf life cycle.
Originally I hoped to drop the handles completely to save memory,
but they are still used in dnodes allocation code, so left for now.

Before this change in CPU profiles of some workloads I saw 4-20% of
CPU time spent in zrl_add_impl()/zrl_remove(), which are gone now.

Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
robn
robn approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

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

Looks good. Impressive performance improvements! I wonder then if its worth removing the support for dnode_move() entirely; leaving aside that we don't have support for any other platforms, is there ever going to be a scenario where the ongoing locking overhead is going to be worth whatever we get back from being able to defragment memory?

@amotin
Copy link
Member Author

amotin commented Jul 22, 2024 •
edited
Loading

@robn I was thinking about removing it completely, since I honestly not very believe in its usefulness, but it was in ZFS forever, and killing it would be much easier than restoring it back later, while giving no immediate benefit now. May be we could simplify dnode allocation code by using some simpler and more standard primitive instead of zrl locks, but that is much less busy code path I haven't looked much into.

@robn
Copy link
Member

robn commented Jul 22, 2024

Makes sense, reasonable tradeoff. Good work :)

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jul 22, 2024
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 29, 2024
behlendorf pushed a commit that referenced this pull request Jul 29, 2024
@amotin @behlendorf
Skip dnode handles use when not needed
ed87d45 
Neither FreeBSD nor Linux currently implement kmem_cache_set_move(),
which means dnode_move() is never called.  In such situation use of
dnode handles with respective locking to access dnode from dbuf is
a waste of time for no benefit.

This patch implements optional simplified code for such platforms,
saving at least 3 dnode lock/dereference/unlock per dbuf life cycle.
Originally I hoped to drop the handles completely to save memory,
but they are still used in dnodes allocation code, so left for now.

Before this change in CPU profiles of some workloads I saw 4-20% of
CPU time spent in zrl_add_impl()/zrl_remove(), which are gone now.

Reviewed-by: Rob Wing <rob.wing@klarasystems.com
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Allan Jude <allan@klarasystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:  Alexander Motin <mav@FreeBSD.org>
Sponsored by:   iXsystems, Inc.
Closes #16374
@amotin amotin deleted the no_handle branch August 2, 2024 20:55
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@amotin @lundman
Cleanup DB_DNODE() macros usage
889b897 
 - Use the macros in few places it was missed.
 - Reduce scope of DB_DNODE_ENTER/EXIT() and inline some DB_DNODE()
uses to make it more obvious what exactly is protected there and
make unprotected accesses by mistake more difficult.
 - Make use of zrl_owner().

Reviewed-by: Rob Wing <rob.wing@klarasystems.com
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Allan Jude <allan@klarasystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#16374
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@amotin @lundman
Skip dnode handles use when not needed
85e7607 
Neither FreeBSD nor Linux currently implement kmem_cache_set_move(),
which means dnode_move() is never called.  In such situation use of
dnode handles with respective locking to access dnode from dbuf is
a waste of time for no benefit.

This patch implements optional simplified code for such platforms,
saving at least 3 dnode lock/dereference/unlock per dbuf life cycle.
Originally I hoped to drop the handles completely to save memory,
but they are still used in dnodes allocation code, so left for now.

Before this change in CPU profiles of some workloads I saw 4-20% of
CPU time spent in zrl_add_impl()/zrl_remove(), which are gone now.

Reviewed-by: Rob Wing <rob.wing@klarasystems.com
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Allan Jude <allan@klarasystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:  Alexander Motin <mav@FreeBSD.org>
Sponsored by:   iXsystems, Inc.
Closes openzfs#16374
robn pushed a commit to robn/zfs that referenced this pull request Nov 5, 2024
@amotin @tonyhutter
Cleanup DB_DNODE() macros usage
56608da 
 - Use the macros in few places it was missed.
 - Reduce scope of DB_DNODE_ENTER/EXIT() and inline some DB_DNODE()
uses to make it more obvious what exactly is protected there and
make unprotected accesses by mistake more difficult.
 - Make use of zrl_owner().

Reviewed-by: Rob Wing <rob.wing@klarasystems.com
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Allan Jude <allan@klarasystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#16374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyhutter tonyhutter tonyhutter approved these changes

@allanjude allanjude allanjude approved these changes

@behlendorf behlendorf behlendorf approved these changes

@robn robn robn approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amotin @robn @behlendorf @allanjude @tonyhutter