Conversation
|
|
|
At the time I had reservations about running it through iostat, in that kstats can have less of an impact on performance, which I was worried about, but also, I didn't want to think much about the ABI changes required. These days I'm more interested in having uniform interfaces for all platforms, though that might be a uniform kstat-like interface. I will play :) |
|
@robn If you decide to go the kstat route, try reading the new kstats in a tight loop, while exporting the pool. It's a good smoke test for panics. |
robn
force-pushed
the
vdev-queue-stats
branch
from
1db22ec to
527199a
Compare
@tonyhutter I'm not sure if this was general advice, or something you'd specifically seen here. I have tried it a bunch now and couldn't make it blow up. If it is something you saw here, I wouldn't be surprised if it was caused by me not zeroing kstat module state properly; that was caught and fixed in review on #17142. But if you can make it blow up, let me know how! |
|
I've decided that for now at least, kstats are the way to go. It's a relatively uniform API and not much extra code; it shows some amount of implementation detail that we might not want to expose to userspace, and, frankly, I need this sooner rather than later. I do want to sort out some sort of more uniform kstat API and interface; make them more of a first-class, cross-platform thing. I have prototypes but nothing much to show yet. I think we'll need to work out what that means up against something like iostat; maybe they are the same thing, maybe not. But I really don't want to get into that for this one, unless you insist :) |
I've seen it blow up in the past when the kstats arn't taking the proper locks (like spa_namespace). Just looking at what you have here, it's probably ok, since you're properly adding/removing the kstat during queue add/removal.
We're one the hook to support whatever queue stats interface we come up with for years to come. I see a more benefits with adding them to
Thoughts? |
Adding a bunch of gauges and counters to show in-flight and total IOs, with per-class breakdowns, and some aggregation counters. Sponsored-by: Klara, Inc. Sponsored-by: Syneto Signed-off-by: Rob Norris <rob.norris@klarasystems.com>
|
Rebased to master, no changes. (Incidentally, I have come around to the view that these should be exposed through iostat, not kstat, but mostly for other reasons. Still, this PR is useful to show where the touch points are when I/someone gets back to it). |
3 participants
[Sponsors: Klara, Inc., Syneto]
Motivation and Context
Part of my ongoing quest to understand what's happening inside the box (previously).
This time, its counters showing what
vdev_queueis up to.Description
Adds a bunch of
wmsum_tcounters to evervdev_queueinstance for a real device. This show current count of IOs queued and in-flight (total and broken down by class), total IOs in/out over the lifetime of the queue, and basic aggregation counters.The counters are exposed under
/proc/spl/kstat/zfs/<pool>/vdev/<guid>/queueon Linux, orkstat.zfs.<pool>.vdev.<guid>.misc.queueon FreeBSD.FreeBSD:
Notes
The actual stats part is pretty unremarkable, being little more than the normal "sums & stats" boilerplate. They perhaps don't technically need to be
wmsum_t, since all the changes are made undervq_lockanyway, but its following a common pattern and part of why I want this is to assist with removing or greatly reducing the scope ofvq_lock, so this is where they'll need to be anyway.Update 2025-03-21: there was a bunch of stuff here about changes to kstats to allow "multi-level" kstats. That became its own PR, #17142, and was recently merged. So I've removed that discussion.
How Has This Been Tested?
Mostly through repeated pool create -> IO -> scrub -> export -> import -> IO -> export -> unload cycles, on both Linux and FreeBSD. Once the numbers looked good and things stopped complaining about replacement names and/or panicking, I declared it good.
Types of changes
Checklist:
Signed-off-by.