zio: use a separate allocation for io_bp_copy and io_bp_orig
#17637
Conversation
robn
changed the title
io_bp_copy and io_bp_orig.io_bp_copy and io_bp_orig
Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
We only need our own copy of the BP if the caller provided one that they would free before we're done with it. Additionally, L2ARC currently abuses io_bp_copy to smuggle the originally BP back to arc_read_done() inside the L2ARC read ZIO. A small helper function is provided to handle that allocation when necessary. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
We only need a copy if we would modify the original and might need to revert, which only applies to non-vdev writes. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
Just a rough eyeball comparison to get a feel for what we're saving.
There was a problem hiding this comment.
This all looks good, and I can't say I have any strong objections. Out of curiosity, have you seen cases where we build up a large number of zio allocations on the system?
That said, this does make me a little uneasy. I have to wonder how worthwhile the memory savings is, compared to the slight risk that the extra allocation will have an observable performance impact with fast devices. Mind you I doubt an impact would be measurable. The cache will be hot and we're doing other more expensive things in the pipeline already.
behlendorf
added
Status: Accepted
|
@behlendorf great question. Mostly no, not in normal operation, which is what makes me uneasy about this change. #16722 was one where the size of #17388 was more recent and the concern there was about the weight of an extra intermediate Given both of those, I think we could just drop this and tackle both cases more directly - some rework of how we handle bulk frees, and then maybe a smaller form of |
|
@robn then let's put this one on the back burner for now, and tackle those other issues directly as you said. We can always revisit the need for this. |
[Sponsors: Klara, Inc., Wasabi Technology, Inc.]
Motivation and Context
256 bytes of
zio_tare devoted to twoblkptr_ts, that aren't used on many zio types. Instead, lets make them pointers instead, and only allocate them when we need them.This PR is intended as a kind of "polished prototype" for discussion. Maybe no one has strong opinions and it sails through, but idk, I have some mild unease about this one, so lets chat :)
Description
The short version: add a new cache,
zio_bp_cache. Inzio_create(), only allocate these when the IO types demand it - forio_bp_copy, when we need our own because the caller will free theirs, and forio_bp_orig, when we will modify the bp (ie most writes) and need the original around in case of a roll back.The three commits are very straightforward and should be squashed in the end; the top one without the signoff is a dumb stats addition just to make it easier to understand what's happening while we're testing and playing around (probably those would be better in the cache stats really, but that's another change for another day).
But now lets talk about the things I don't like :)
I don't like the separate allocation, but its kind of what it has to be. It should be efficient enough once the cache is warm, though there are never too many out at once. The gain on the memory side seems worth it though. In my unscientific tests, some straightforward readng & writing to a 4xZ1 suggests we only allocate from
zio_bp_cachefor about 10% of allzio_cacheallocations. A full ZTS run did about 40%, which may be more representative over time, but still seems worth a look.This is a fairly naive conversion which took me all of a day, including testing runs, $dayjob and a big sleep. We could likely do better if we spent some time consider how we pass BPs around and where we could be a bit smarter. Could we share BPs that won't change more through the IO tree? Reach into our parent or
io_logicalto grab one it if we're downstream? Some refcounting and/or copy-on-write arrangement?(at least, we can probably easily fix that L2ARC thing by just passing it
arc_read_done()instead of smuggling it inside the zio).Here's the size change. As you see, the new one has a pretty hefty hole in the middle of it. It's easy enough to reorder some things to fill it, but it doesn't materially change anything because of the cacheline alignment, we just get extra padding at the end. That feels like something for the next PR.
pahole before (1152 bytes)
pahole after (960 bytes)
The other thing I'm unsure about with this is whether we generally want to look towards this "componentised" kind of model of
zio_t, where it only has the pieces it needs. The fields can be grouped into a bunch of different "uses", eg, there's a handful items specific to queue operations, which aren't needed outside of read/write/trim to leaf devices. I can see arguments for and against; it does let us get the memory usage down to just what we need, but C doesn't make it easy to work with that sort of structure, and its also gotta be done carefully to avoid fragmenting memory too much. Maybe it doesn't matter if we don't see this PR as a precedent, or we just consider this a step in a direction.How Has This Been Tested?
Full ZTS run completed successfully against Linux 6.12. I would not however be surprised to find I've introduced some very subtle NULL deref in some niche situation though.
Types of changes
Checklist:
Signed-off-by.