initialize metaslab range trees in metaslab_init

[sdimitro]

= Motivation

We've noticed several zloop crashes within Delphix generated
due to the following sequence of events:

• A device gets expanded and new metaslabas are allocated for
  it. These metaslabs go through metaslab_init() but haven't
  gone through metaslab_sync_done() yet. This meas that the
  only range tree that's actually set is the ms_allocatable.
  All the others are NULL.

• A vdev_initialization is issues and vdev_initialize_thread
  starts processing one of these new metaslabs of the expanded
  vdev.

• As part of vdev_initialize_calculate_progress() we call
  into metaslab_load() and metaslab_load_impl() which
  in turn tries to dereference the metaslabs trees that
  are still NULL and therefore we crash.

The same failure can come up from the vdev_trim code paths.

= This Patch

We considered the following solutions to deal with this issue:

[A] Add logic to vdev_initialize/trim to skip those new
metaslabs. We decided against this as it would be good
to avoid exposing this lower-level detail to higer-level
operations.

[B] Have metaslab_load_impl() return early for new metaslabs
and thus never touch those range_trees that are NULL at
that time. This seemed more of a work-around for the bug
and not a clear-cut solution.

[C] Refactor our logic so all metaslabs have their range_trees
created at the time of their creatin in metaslab_init().

In this patch we decided to go with [C] because:

(1) It doesn't expose more metaslab details to higher level
operations such as vdev initialize and trim.

(2) The current behavior of creating the range trees lazily
in metaslab_sync_done() is unnecessarily complicated.

(3) Always initializing the metaslab range_trees makes other
parts of the codebase cleaner. For example, we used to
use ms_freed as the reference value for knowing whether
all the range_trees have been initialized. Now we no
longer need to do that check in most places (and in the
few that we do we use the ms_new boolean field now
which is more readable).

= Side Changes

Probably due to a mismerge we set ms_loaded to B_TRUE twice
in metasloab_load_impl(). In this patch we remove the extraneous
assignment.

Signed-off-by: Serapheim Dimitropoulos [email protected]

How Has This Been Tested?

Ran zloop for 4 days and the issue described didn't occur again.

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.

[behlendorf]

Thank you! Looks great. This was one of the ztest failures we hit more frequently so it'll be nice to have it resolved.