9185 metaslab_verify_weight_and_frag() shouldn't cause side-effects #9209
Conversation
`metaslab_verify_weight_and_frag()` a verification function and by the end of it there shouldn't be any side-effects. The function calls `metaslab_weight()` which in turn calls `metaslab_set_fragmentation()`. The latter can dirty and otherwise not dirty metaslab fro the next TXGand set `metaslab_condense_wanted` if the spacemaps were just upgraded (meaning we just enabled the SPACEMAP_HISTOGRAM feature through upgrade). This patch ensures that metaslabs like these are skipped thus avoiding that problem. We could also get rid of that function completely but I hesitated because it has caught issues during development of other features in the past. Fixing this issue should also help with with most failures that issue openzfs#9186 has been causing to the test-bots recently. Signed-off-by: Serapheim Dimitropoulos <serapheim@delphix.com>
sdimitro
force-pushed
the
zol-issue-9185
branch
from
f4da0ac
to
89e732a
Compare
Codecov Report
@@ Coverage Diff @@
## master #9209 +/- ##
==========================================
+ Coverage 58.23% 58.58% +0.34%
==========================================
Files 235 235
Lines 77720 77570 -150
==========================================
+ Hits 45259 45442 +183
+ Misses 32461 32128 -333
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #9209 +/- ##
==========================================
+ Coverage 77.54% 79.11% +1.57%
==========================================
Files 389 400 +11
Lines 121529 122003 +474
==========================================
+ Hits 94237 96522 +2285
+ Misses 27292 25481 -1811
Continue to review full report at Codecov.
|
| * those metaslabs. | ||
| */ | ||
| if (msp->ms_sm->sm_dbuf->db_size != sizeof (space_map_phys_t)) | ||
| return; |
There was a problem hiding this comment.
The comment explains this well, but this seems like this is exposing a poorly-designed corner of the code. Could we do something like pass a nodirty flag to metaslab_weight(), or have a metaslab_weight_impl(), which just recalculates the weight and doesn't check about condensing? That way, if metaslab_weight() decides to mutate the metaslab due to some other condition, the relevant code will be close by, and we won't have to add another check here.
|
Closing. This should be unnecessary with the fix proposed in #9209. |
metaslab_verify_weight_and_frag()a verification function andby the end of it there shouldn't be any side-effects.
The function calls
metaslab_weight()which in turn callsmetaslab_set_fragmentation(). The latter can dirty and otherwisenot dirty metaslab fro the next TXGand set
metaslab_condense_wantedif the spacemaps were just upgraded (meaning we just enabled the
SPACEMAP_HISTOGRAM feature through upgrade).
This patch ensures that metaslabs like these are skipped thus
avoiding that problem. We could also get rid of that function
completely but I hesitated because it has caught issues during
development of other features in the past.
Fixing this issue should also help with with most failures that
issue #9186 has been causing to the test-bots recently.
Motivation and Context
Description
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.