Introduce two types of root ZIOs

[grwilson]

The zio_root() interface was recently changed to remove the READY stage in the pipeline. There might be workloads that rely on zio_root() calling the READY pipeline phase.

This change addresses that by re-introducing the original functionality of zio_root() and introduce a new interface, zio_root_done(), which is used in the performance critical paths of the ZIL.

[amotin]

George, what is the motivation of this change? Compatibility with some third-party code, or what? I have reviewed all ~50 zio_root() calls to make sure they do not care about ready stage. A code that cares about it should use zio_null() (which only a couple NULL arguments more complicated), and I actually addressed couple cases of that in my patch. Now you are introducing another API, using it only in handful of places instead of ~50, and reverting all others to less efficient one for no good reason, I suppose just to not modify all the code. I am not happy.

[grwilson]

We have code that relies on the old behavior. I wanted to post this in case it benefits other. If the community doesn't want the change then we can just close this but there might be others that have a similar need.

[amotin]

OK, lets listen for people, but there is no reason to not just switch to zio_null() in third-party code. It would be compatible with both old and new semantics. Sure, I've changes the semantics, but IMO to more logical and efficient.

[grwilson]

I looked at changing things but the conceptual nature of a ROOT zio made the code more readable and that was more important than the negligible efficiency gains in the pipeline. I am ok with not merging this change, but since it was something that impacted us, I wanted to see if it was helpful to anybody else.

[amotin]

To me my concept looks logical: ROOT ZIO means really a root, i.e. nothing on top of it, only a done callback. Same time NULL ZIO is more technical, allowing complicated tricks with being somewhere in between or having fancy hooks, etc.

What I don't like in this patch is that you are returning several dozen calls to the previous semantics they do not need. And would you update all of them to zio_root_done(), zio_done() would left with no (or may be one that I can think of) use cases in the base code.

[grwilson]

I understand your point and acknowledge that the master branch does not rely on the old behavior of zio_root(). The point of this PR is that there might be other downstream consumers that do need this functionality and I was trying to reduce the impact to those developers. We have already addressed this in our tree and if no other consumer relies on this then we simply don't merge this PR. The approach taken in this PR was to revert to old behavior except in the key areas where a performance win was identified.

[ikozhukhov]

i'd like to merge this code for flexible migration of old changes if needed. saved compatibility with be in benefit, it's my opinion. we can expand interfaces if needed and end SW can use one or another way.

[amotin]

@ikozhukhov Are you talking about some particular "old changes"? Can't they be switched to zio_null() if not yet and become "new changes"? I do not want to indefinitely keep the proposed mix of zio_root() and zio_root_done() for no reason.

[ikozhukhov]

@ikozhukhov Are you talking about some particular "old changes"? Can't they be switched to zio_null() if not yet and become "new changes"? I do not want to indefinitely keep the proposed mix of zio_root() and zio_root_done() for no reason.

i mean about old API what can be used by others. i understand about stable and unstable API, but for many reasons i'd like to keep older API at all because some older private apps can use it. and i'd like to add new API and move logic to it where it possible and let to others try to update apps to new API or stay on old one. we can add additional notes/comments to code and docs to be sure do not forget about changes.

for example(not about this change but for reference): some apps can use ld_open() to library and use API internally - it is jenkins with oracle jdk - and to keep compatibility i did updates for libzfs and needs support it because some installation wants to keep to use oracle jdk instead of openjdk.
this example just for info, nothing else. but it show how to hard to support older sw if needed if we will update API instead of extend it by new.

[ryao]

zio_root() and zio_root_done() are confusing, since zio_root_done() looks like a done function for zio_root(). Honestly, I cannot think of a good balance between preserving ABI stability for external consumers and keeping this easy to understand for developers working on the code.

I would like to say no, but how about this? Introduce a zio_root_legacy() for external consumers that need the old functionality. That would minimize additional confusion to people working on the codebase, while giving external consumers the ability to get the old functionality. The only downside is that the external consumers would need to adapt to use this when it is available.

[amotin]

@ryao Old functionality can be achieved just by using zio_null(). It does not need additional function. It is all purely about running unmodified code.

[ryao]

@ryao Old functionality can be achieved just by using zio_null(). It does not need additional function. It is all purely about running unmodified code.

ZIO is already fairly convoluted. I feel like doing this would just make that worse. :/

[amotin]

I suppose we may close this almost a year later.