Why is it not possible to change a pool feature from enabled to disabled?

[geppi]

As long as a pool feature is not in the active state, no on-disk format changes have been made to the pool.

So what is the reason that zpool set feature@some-feature=disabled pool-name does not work on a pool with feature@some-feature enabled ?

I can perfectly understand the reason why a feature that is in the active state cannot simply be disabled.
However, even from the active state a feature can move back to enabled after all on disk data structures that used the feature have been destroyed.

What complex changes to which data structures are made when just enabling a pool feature?

[scineram]

Nothing really, it must be a race thing. It should be easier to implement at import I would guess.

[geppi]

I think it would be valuable to have this functionality available.

As an example I had just recently upgraded to zfs 2.2.0 on a server and upgraded the pools.
After that I became aware of issue #15554 and checked the pools which have feature@block_cloning enabled but not active.
I could have just disabled the feature@block_cloning and would have been safe until a fix got released.

From what you're saying and which matches my understanding it shouldn't be too much effort to implement this functionality.

So would that be worth something like a feature request?

[IvanVolosyuk]

The corruption is pretty rare (happens in very specific situations), doesn't cause damage to the pool metadata and happens without block cloning even on ZFS 2.1.13 as far as I'm aware. The ***@***.***_cloning *in *enabled* state doesn't prevent a pool from being importable from ZFS 2.1.13, though I didn't check the other features, they might be a road blocker to go back to the previous stable release. ZFS 2.2.1 has a tunable which disables block cloning by default, so that's another option for you. Ability to disable enabled features is functionality which is quite rare and may be a source of new bugs and require more development resources.

…

On Sat, Nov 25, 2023 at 12:02 AM Thomas Geppert ***@***.***> wrote: I think it would be valuable to have this functionality available. As an example I had just recently upgraded to zfs 2.2.0 on a server and upgraded the pools. After that I became aware of issue #15554 <#15554> and checked the pools which have ***@***.***_cloning* enabled but not active. I could have just disabled the ***@***.***_cloning* and would have been safe until a fix got released. From what you're saying and which matches my understanding it shouldn't be too much effort to implement this functionality. So would that be worth something like a feature request? — Reply to this email directly, view it on GitHub <#15570 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXQ6HOAQ7UD3R3GGWI4DUTYGCLFZAVCNFSM6AAAAAA7ZBUEPCVHI2DSMVQWIX3LMV43SRDJONRXK43TNFXW4Q3PNVWWK3TUHM3TMNRQGQYTK> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[geppi]

@IvanVolosyuk You're right, according to PR #15571 it seems to boil down to a problem existing for much longer and the block cloning feature did just increase the probability to hit this bug.

However, let's not divert from the subject of this discussion.

I checked the sources and indeed reverting a feature from enabled to disabled on a pool that is not imported wouldn't be a problem.
So the easiest way to implement this functionality would probably be to integrate it into the pool import or export function.

Nevertheless it would be even more useful if it could be performed on an already imported pool.
While there exists comprehensive documentation of the ZFS on disk format, understanding the in memory structures and processes seems to require pretty extensive digging into the sources. Unfortunately I'm not that familiar with the code to point out where those race conditions that @scineram has mentioned could happen.

Do those functions that require a pool feature to be enabled check this every time before doing their work?
So would it be sufficient to put some kind of lock on an in memory representation of the zpool nv_list before reverting?

Any hint where to look in the sources would be appreciated.