metaslab: don't pass whole zio to throttle reserve APIs; rename IO_ALLOCATING to ALLOC_THROTTLED

[robn]

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

First, see my story in #17507. Part of the reason that it took me so long to understand what was happening was that metaslab_class_throttle_reserve() is adding the IO_ALLOCATING flag to the zio directly. This isn’t a bug as such, and I knew it was happening, but because it wasn’t in zio.c where I was spending most of my time, I didn’t notice it on the scene in quite the same way. Skimming the code looking for things to ask more questions of is a pretty standard part of my debugging approach, and it not being visible means I missed it.

Initially, I thought to just lift the flag change out to the two call sites in zio.c. But, looking closer, it seems like there’s no particular reason for _reserve() and _unreserve() to take an entire zio as an argument. It only needs three items from it, of which one is already passed in directly, and haven’t them separate adds confusion in zio_dva_allocate_done() where parts of both are passed to it. And if we don’t pass the zio in, there’s no temptation to just fiddle with it in place. The API only asks for exactly what it needs to do the job, and now it also looks a lot more like the rest of it - there’s already some functions that take class/allocator pairs, and none that take zios, so it looks less weird.

The flag name IO_ALLOCATING is also confusing, since its applied when the allocation was throttled and a reservation is taken. It could be read as “an allocation is in progress”, or also something like the IO_IS_ALLOCATING() macro, which is not directly related. So, I’ve renamed that as well to ALLOC_THROTTLED, which at least says closer to what it means.

Description

Summarising above:

• reworked metaslab_class_throttle_reserve() and metaslab_class_throttle_unreserve() to not take a zio, just passed what it needs.
• renamed ZIO_FLAG_IO_ALLOCATING to ZIO_FLAG_ALLOC_THROTTLED.

How Has This Been Tested?

Compiled checked on both platforms, full test run completed on Linux.

I would not expect any differences really, since it’s little more than a rename.

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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• 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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

zio was originally there to be used as refcount tag, similar to metaslab_group_alloc_increment(), but in this implementation I reduced refcounts to atomics, so it is not required any more. So I have no objections. May be we could rename slots arguments to copies, since also after my rework the code now reserves bytes instead of commands, so slots IMHO lost some of its sense.

[robn]

zio was originally there to be used as refcount tag, similar to metaslab_group_alloc_increment(), but in this implementation I reduced refcounts to atomics, so it is not required any more. So I have no objections.

Thanks, I remember that you did some work here recently, but I wasn't following closely at the time. Appreciate the explainer and reminder!

May be we could rename slots arguments to copies, since also after my rework the code now reserves bytes instead of commands, so slots IMHO lost some of its sense.

Yeah, copies is good, done. I've adjusted the comment to match.

Incidentally, I was a little bummed that I couldn't just pass in single size and make it truly "give me this much", but the call to metaslab_class_balance() is conditioned on the IO size. It's no big deal really though, just offends my like of symmetry :)