range_tree: Add zfs_recover_rt parameter and extra debug info

[ihoro]

Motivation and Context

There are production cases when loading of a metaslab leads to a ZFS panic due to unexpected entries in its spacemap (presumably). The assertions in zfs_range_tree_add_impl() and zfs_range_tree_remove_impl() fail due to overlapping or missing segments, etc. A business would like to go ahead with such pools while the root cause is being investigated.

Description

The idea is to allow loading such metaslabs with a potential space leak as a trade-off instead of a potential data loss.

We already have zfs_recover module parameter to mitigate various issues, including some range tree cases, and this patch adds zfs_recover_ms parameter to localize the recovery behavior to the metaslab loading process only.

The following diagrams are expected to help with the details:

How Has This Been Tested?

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)
• 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.

[ihoro]

The update includes:

• Re-work into zfs_recover_rt parameter instead of zfs_recover_ms. It covers all range trees.
• Add range tree name as an extra debug info resulting into a message like zfs: rt_instance=vdev_obsolete_segments: ....
• The metaslab related trees provide even more details like zfs: rt_instance={spa=p1 vdev_guid=4127788562752866619 ms_id=0 ms_allocatable}: ....
• Update zfs.4 man page respectively

[tonyhutter]

Looks like there's a build issue:

    CC       module/zfs/libzpool_la-dsl_bookmark.lo
    CC       module/zfs/libzpool_la-dsl_crypt.lo
  module/zfs/dnode.c: In function 'dnode_free_range':
  module/zfs/dnode.c:2441:59: error: passing argument 7 of 'zfs_range_tree_create_usecase' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
   2441 |                             ZFS_RANGE_TREE_UC_FREE_SPACE, "dn_free_ranges");
        |                                                           ^~~~~~~~~~~~~~~~
  In file included from ./include/sys/space_map.h:34,
                   from ./include/sys/spa.h:46,
                   from ./include/sys/dbuf.h:32,
                   from module/zfs/dnode.c:28:
  ./include/sys/range_tree.h:294:45: note: expected 'char *' but argument is of type 'const char *'
    294 |     zfs_range_tree_usecase_t usecase, char *instance);
        |                                       ~~~~~~^~~~~~~~
    CC       module/zfs/libzpool_la-dsl_dataset.lo
    CC       module/zfs/libzpool_la-dsl_deadlist.lo
  cc1: all warnings being treated as errors
  make[4]: *** [Makefile:10313: module/zfs/libzpool_la-dnode.lo] Error 1

[ihoro]

Looks like there's a build issue:

Indeed, there should have been one more dev iteration on my side. It should be fixed now.

[tonyhutter]

For some reason a bunch of the runners are failing. I'm going to manually restart then.

[tonyhutter]

Please rebase on master; that will pull in the new .github/workflow/* files and allow the tests to complete.

[ihoro]

For some reason a bunch of the runners are failing. I'm going to manually restart then.

Thank you.

Please rebase on master; that will pull in the new .github/workflow/* files and allow the tests to complete.

Sure, it's been moved 3 commits up, over the recent workflow changes, it should work now.

[amotin]

I see plenty of places where use case is not defined. We could look better. Also I think in many cases we could still log pool and vdev, even if we have no metaslab number to report.

[ihoro]

I see plenty of places where use case is not defined. We could look better.

It seems it's time to rename the _UC_UNKNOWN flag to the more appropriate one _UC_GENERIC. It depicts the intention better -- a range tree without special treatment where zfs_recover_rt simply does not panic upon unexpected additions/removal.

The unknown (now generic) flag is intentionally used for the range tree instances where special treatment is not expected. Sometimes it's not about allocated/free space or it's a temporary tree which is based on already "recovered" other ones. Anyway, I think I could review the instances once again, probably someone should not be GENERIC.

Also I think in many cases we could still log pool and vdev, even if we have no metaslab number to report.

Yes, it's worth the extra code to be maximum useful. It will come with the next iteration of the patch.

[amotin]

I don't think "GENERIC" is really meaningful. Easier and cleaner I think would be just to pass 0 if we can't say anything better (we really should).

[GregorKopka]

I disagree on logic outlined the diagrams, regardless if it can already be triggered by some module parameter:
Freeing something already free is something completely different than allocating something that is already in use.

It is IMHO a very bad idea to allow the latter to happen.

[amotin]

Cosmetics, but here and in other places I would reduce "rt_instance=" to "rt=", since it provides no useful information, and the line is too long. Or otherwise write full "range tree" to make it more human-readable if we don't care about length.

[amotin]

@ihoro So how about remove unneeded ZFS_RANGE_TREE_F_UC_GENERIC constant as I have asked and one more comment above? I don't really believe in the "recovery" part, but I do like more informative panic messages.

[amotin]

@ihoro ping?

[ihoro]

The debug info part has been extracted into a separate PR: #17581

[amotin]

This needs a rebase after #17581 has beed merged.

[behlendorf]

@ihoro can you rebase this now that #17581 has been merged.

[ihoro]

I've got back into the context of this and there is a desire to revise the interface and expectations from the end user perspective. I think we could discuss the following topics:

• First of all, it reminds me that this is not a fix. If a pool has overlapping issue then there is a chance it is already in trouble, i.e., some data is lost. There might be reasons why we want to make it running anyway, but users should understand that ignoring such troubles opens undefined behavior path and potentially leads to more issues on top of the existing ones.
• The above means to rework the man page to avoid wording like "to recover from fatal issues" etc. The old patch simply follows the existing zfs_recover param's concept.
• There is a strong intuition that this behavior must be detached from the existing zfs_recover. The old patch extends the area which zfs_recover param covers and allows to activate only part of it by using zfs_recover_rt. What do you think if we keep zfs_recover as is and simply add a new set of behavior activated separately? And, obviously, it should not have any recover in the name not to mislead compared to the existing zfs_recover.
• Also, there is an intuition that the interface should provide a way to ignore such issues only during metaslab loading and still panic upon all other cases. Otherwise, ignoring all range tree overlapping issues touches all aspects of ZFS, not only related to the allocation accounting.
• And we can provide the "full mode" to ignore all range tree issues.

The above is a conceptual discussion of what we would like to have. If we go down the technical road then we could discuss naming options, whether we want to use bool-like knobs or bitfield ones, whether it should collaborate with the existing metaslab_debug_load parameter instead of introducing a brand new one, and so on.

[amotin]

ignore such issues only during metaslab loading and still panic upon all other cases

This makes some sense to me. Duplicate frees found during loading has already happened and are already on disk, and didn't lead anywhere so far. Would it happen earlier (in other places), it could be more informative.

[behlendorf]

ignore such issues only during metaslab loading and still panic upon all other cases

To build on this a little bit, what I think we want is to add a metaslab_skip_unloadable tunable. When set, if any issues are encountered when loading a metaslab from disk (like duplicate frees) then; 1) log an error to the debug log, 2) abort the metaslab load, 3) mark the metaslab as damaged in memory so we don't try it again, and 4) try loading a different metaslab. This would at least allow the pool to be imported read/write without risking further damage to that metaslab. In all other cases I agree we need to panic.

[GregorKopka]

In all other cases I agree we need to panic.

Would it be feasible to 'panic' just the affected pool, preferably in a way that allows to (force) unload anything related to it without needing a (hard) reboot... instead halting the whole system?

[amotin]

Would it be feasible to 'panic' just the affected pool

Would we be able to handle it as Brian described, we would not need to panic at all.