allow callers to allocate and provide the abd_t struct #11439
Conversation
|
This is a nice improvement to the interface! I'm still working my way through it but a few quick comments. PR #11397 has just been merged so you can rebase and drop it form the patch stack here. Which you're at it I'd suggest squashing the two remaining commits and adding the following line to the commit comment to additionally request the "coverage" builder. By default we stopped running it for every PR a little while back and have only been running it for merged commits. But in this case I think we need to run the kmemleak checker to ensure no abds are ever getting leaked.
FYI, just in the last day or so we've seen some instability in the FreeBSD head builder. But since both the head and stable/12 builders failed right away when starting the ZTS odds are something still isn't quite right in the FreeBSD code modified by this PR. I'd have expected the stable/12 builder to pass. cc: @bwatkinson |
Codecov Report
@@ Coverage Diff @@
## master #11439 +/- ##
==========================================
+ Coverage 76.74% 76.76% +0.01%
==========================================
Files 400 400
Lines 127851 127816 -35
==========================================
- Hits 98118 98114 -4
+ Misses 29733 29702 -31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| static inline uint_t | ||
| abd_get_size(abd_t *abd) | ||
| { | ||
| return (abd->abd_size); | ||
| } |
There was a problem hiding this comment.
I originally created this function because we hid the definition on the ABD struct in abd_impl.h. Technically we don't need this anymore, but it can stay if everyone wants to keep it.
There was a problem hiding this comment.
Yeah, I could go either way but I figured keeping it as an accessor doesn't hurt.
|
@ahrens this has nothing to do with your PR, but could you possibly update line 775 in abd.c in this PR? I meant to update this from |
The `abd_get_offset_*()` routines create an abd_t that references another abd_t, and doesn't allocate any pages/buffers of its own. In some workloads, these routines may be called frequently, to create many abd_t's representing small pieces of a single large abd_t. In particular, the upcoming RAIDZ Expansion project makes heavy use of these routines. This commit adds the ability for the caller to allocate and provide the abd_t struct to a variant of `abd_get_offset_*()`. This eliminates the cost of allocating the abd_t and performing the accounting associated with it (`abdstat_struct_size`). The RAIDZ/DRAID code uses this for the `rc_abd`, which references the zio's abd. The upcoming RAIDZ Expansion project will leverage this infrastructure to increase performance of reads post-expansion by around 50%. Additionally, some of the interfaces around creating and destroying abd_t's are cleaned up. Most significantly, the distinction between `abd_put()` and `abd_free()` is eliminated; all types of abd_t's are now disposed of with `abd_free()`. Signed-off-by: Matthew Ahrens <mahrens@delphix.com> Requires-builders: arch,centos7,centos8,centosstream8,debian10,fedora33,ubuntu18,ubuntu20,builtin,freebsd12,freebsd13,coverage
|
@bwatkinson Sure thing. |
behlendorf
added
Status: Accepted
The `abd_get_offset_*()` routines create an abd_t that references another abd_t, and doesn't allocate any pages/buffers of its own. In some workloads, these routines may be called frequently, to create many abd_t's representing small pieces of a single large abd_t. In particular, the upcoming RAIDZ Expansion project makes heavy use of these routines. This commit adds the ability for the caller to allocate and provide the abd_t struct to a variant of `abd_get_offset_*()`. This eliminates the cost of allocating the abd_t and performing the accounting associated with it (`abdstat_struct_size`). The RAIDZ/DRAID code uses this for the `rc_abd`, which references the zio's abd. The upcoming RAIDZ Expansion project will leverage this infrastructure to increase performance of reads post-expansion by around 50%. Additionally, some of the interfaces around creating and destroying abd_t's are cleaned up. Most significantly, the distinction between `abd_put()` and `abd_free()` is eliminated; all types of abd_t's are now disposed of with `abd_free()`. Reviewed-by: Brian Atkinson <batkinson@lanl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Matthew Ahrens <mahrens@delphix.com> Issue openzfs#8853 Closes openzfs#11439
The `abd_get_offset_*()` routines create an abd_t that references another abd_t, and doesn't allocate any pages/buffers of its own. In some workloads, these routines may be called frequently, to create many abd_t's representing small pieces of a single large abd_t. In particular, the upcoming RAIDZ Expansion project makes heavy use of these routines. This commit adds the ability for the caller to allocate and provide the abd_t struct to a variant of `abd_get_offset_*()`. This eliminates the cost of allocating the abd_t and performing the accounting associated with it (`abdstat_struct_size`). The RAIDZ/DRAID code uses this for the `rc_abd`, which references the zio's abd. The upcoming RAIDZ Expansion project will leverage this infrastructure to increase performance of reads post-expansion by around 50%. Additionally, some of the interfaces around creating and destroying abd_t's are cleaned up. Most significantly, the distinction between `abd_put()` and `abd_free()` is eliminated; all types of abd_t's are now disposed of with `abd_free()`. Reviewed-by: Brian Atkinson <batkinson@lanl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Matthew Ahrens <mahrens@delphix.com> Issue openzfs#8853 Closes openzfs#11439
Motivation and Context
The
abd_get_offset_*()routines create an abd_t that referencesanother abd_t, and doesn't allocate any pages/buffers of its own. In
some workloads, these routines may be called frequently, to create many
abd_t's representing small pieces of a single large abd_t. In
particular, the upcoming RAIDZ Expansion project makes heavy use of
these routines.
Description
This commit adds the ability for the caller to allocate and provide the
abd_t struct to a variant of
abd_get_offset_*(). This eliminates thecost of allocating the abd_t and performing the accounting associated
with it (
abdstat_struct_size). The RAIDZ/DRAID code uses this for therc_abd, which references the zio's abd. The upcoming RAIDZ Expansionproject will leverage this infrastructure to increase performance of
reads post-expansion by around 50%.
Additionally, some of the interfaces around creating and destroying
abd_t's are cleaned up. Most significantly, the distinction between
abd_put()andabd_free()is eliminated; all types of abd_t's are nowdisposed of with
abd_free().How Has This Been Tested?
ZTS, zloop, and perf testing with RAIDZ Expansion code (#8853).
Note: I haven't tested the FreeBSD changes. We'll see how it does with buildbot.
Types of changes
Checklist:
Signed-off-by.