DLPX-73172 [Backport of DLPX-73147 to 6.0.6.0] panic: refcount!=0 in …

…dsl_bookmark_destroy_sync_impl (openzfs#241)

include/sys/dmu.h

@@ -385,6 +385,8 @@ typedef struct dmu_buf {
 #define	DMU_POOL_ZPOOL_CHECKPOINT	"com.delphix:zpool_checkpoint"
 #define	DMU_POOL_LOG_SPACEMAP_ZAP	"com.delphix:log_spacemap_zap"
 #define	DMU_POOL_DELETED_CLONES		"com.delphix:deleted_clones"
+#define	DMU_POOL_BOOKMARK_V2_RECALCULATED \
+	"com.delphix:bookmark_v2_recalculated"
 
 /*
  * Allocate an object from this objset.  The range of object numbers

module/zfs/dsl_pool.c

@@ -1087,25 +1087,63 @@ static int
 sync_bookmark_featureflags_cb(dsl_pool_t *dp, dsl_dataset_t *ds, void *arg)
 {
 	dmu_tx_t *tx = arg;
-	for (dsl_bookmark_node_t *dbn = avl_first(&ds->ds_bookmarks);
-	    dbn != NULL; dbn = AVL_NEXT(&ds->ds_bookmarks, dbn)) {
-		if (dbn->dbn_phys.zbm_flags & ZBM_FLAG_HAS_FBN ||
-		    dbn->dbn_phys.zbm_redaction_obj != 0) {
+
+	if (ds->ds_bookmarks_obj == 0)
+		return (0);
+
+	int err = 0;
+	zap_cursor_t zc;
+	zap_attribute_t attr;
+
+	for (zap_cursor_init(&zc, dp->dp_meta_objset, ds->ds_bookmarks_obj);
+	    (err = zap_cursor_retrieve(&zc, &attr)) == 0;
+	    zap_cursor_advance(&zc)) {
+		ASSERT3U(attr.za_integer_length, ==, sizeof (uint64_t));
+		/*
+		 * This logic matches that in dsl_bookmark_destroy_sync_impl().
+		 */
+		if (attr.za_num_integers * attr.za_integer_length >
+		    BOOKMARK_PHYS_SIZE_V1) {
 			spa_feature_incr(dp->dp_spa, SPA_FEATURE_BOOKMARK_V2,
 			    tx);
 		}
 	}
-	return (0);
+	zap_cursor_fini(&zc);
+	if (err == ENOENT)
+		err = 0;
+	return (err);
 }
 
 void
 dsl_pool_sync_bookmark_featureflags(dsl_pool_t *dp, dmu_tx_t *tx)
 {
 	ASSERT(dmu_tx_is_syncing(tx));
 
+	/*
+	 * We may need to re-evaluate the feature refcount because it was
+	 * previously incorrectly calculated, due to a bug.  Therefore the
+	 * refcount may be nonzero.  Because some bookmarks may have been
+	 * deleted after the incorrect calculation, the current refcount value
+	 * may be unrelated to the current on-disk state.  Therefore we need to
+	 * reset the refcount to 0 and recalculate from scratch.
+	 */
+	uint64_t old_refcount = 0;
+	while (spa_feature_is_active(dp->dp_spa, SPA_FEATURE_BOOKMARK_V2)) {
+		spa_feature_decr(dp->dp_spa, SPA_FEATURE_BOOKMARK_V2, tx);
+		old_refcount++;
+	}
+
 	VERIFY0(dmu_objset_find_dp(dp, dp->dp_root_dir_obj,
-	    sync_bookmark_featureflags_cb, tx, DS_FIND_CHILDREN |
-	    DS_FIND_SERIALIZE));
+	    sync_bookmark_featureflags_cb, tx,
+	    DS_FIND_CHILDREN | DS_FIND_SERIALIZE));
+
+	uint64_t new_refcount;
+	VERIFY0(feature_get_refcount(dp->dp_spa,
+	    &spa_feature_table[SPA_FEATURE_BOOKMARK_V2], &new_refcount));
+	spa_history_log_internal(dp->dp_spa,
+	    "recalculate SPA_FEATURE_BOOKMARK_V2 refcount", tx,
+	    "old=%llu new=%llu",
+	    (long long)old_refcount, (long long)new_refcount);
 }
 
 void

module/zfs/spa.c

@@ -8744,30 +8744,51 @@ spa_sync_upgrades(spa_t *spa, dmu_tx_t *tx)
 		 * has those features that is upgraded to a version with
 		 * BOOKMARK_V2 will crash if the number of bookmarks ever goes
 		 * below the amount that existed when the pool was upgraded.
-		 * This code iterates over all the bookmarks in the system and
-		 * increments the V2 feature once for each bookmark that uses
-		 * either BOOKMARK_WRITTEN or REDACTION_BOOKMARKS.  It's not
-		 * just the sum of the two, because some bookmarks will have
-		 * both REDACTION and WRITTEN, and some could have either
-		 * without the other.
 		 *
-		 * This logic will only execute once because, going forwards,
-		 * any time BOOKMARK_WRITTEN or REDACTION_BOOKMARKS is
-		 * incremented, BOOKMARK_V2 will be as well.  As a result, its
-		 * count should never dip back to zero (maaking in inactive
-		 * but enabled) while the other two are active.
+		 * Additionally, the logic for decrementing BOOKMARK_V2 is
+		 * based on the bookmark's ZAP entry size, not the use of the
+		 * redaction or written fields.  In the current code, these
+		 * values are equivalent (the size is > BOOKMARK_PHYS_SIZE_V1
+		 * if and only if the new fields are in use), but in Delphix
+		 * version 5.0.x.x, a larger size was used unconditionally
+		 * (even if the redaction field was not set). Before Delphix
+		 * version 6.0.6.0, this upgrade code did not take into
+		 * account this possibility, resulting in a lower-than
+		 * expected refcount, and subsequent panics when attempting
+		 * to decrement a zero refcount.  Therefore, we need to
+		 * recalculate the refcount even if it was calculated once
+		 * (by the buggy code).
+		 *
+		 * We only need to recalculate the refcount once (with the
+		 * current algorithm), because now any time BOOKMARK_WRITTEN
+		 * or REDACTION_BOOKMARKS is incremented, BOOKMARK_V2 will be
+		 * as well.
+		 *
+		 * Presence of the DMU_POOL_BOOKMARK_V2_RECALCULATED field
+		 * indicates that it has been recalculated with the current
+		 * algorithm, based on the zap entry size.  After
+		 * recalculating the refcount, if the pool is brought back to
+		 * a system with the buggy upgrade code, that code won't be
+		 * invoked because it's predicated on the V2 refcount being
+		 * nonzero (and if it is zero, that's accurate and the old
+		 * code will leave it at zero). Therefore we can use this
+		 * backwards-compatible field rather than a feature flag.
+		 *
+		 * This upgrade code can be removed once we no longer support
+		 * upgrading from releases before Delphix 6.0.6.0.
 		 */
-		boolean_t bv2_en = spa_feature_is_enabled(spa,
-		    SPA_FEATURE_BOOKMARK_V2);
-		boolean_t bv2_ac = spa_feature_is_active(spa,
+		boolean_t bv2_enabled = spa_feature_is_enabled(spa,
 		    SPA_FEATURE_BOOKMARK_V2);
-
-		boolean_t dep_ac = spa_feature_is_active(spa,
-		    SPA_FEATURE_BOOKMARK_WRITTEN) || spa_feature_is_active(spa,
-		    SPA_FEATURE_REDACTION_BOOKMARKS);
-
-		if (bv2_en && !bv2_ac && dep_ac) {
+		boolean_t recalculated = (zap_contains(spa->spa_meta_objset,
+		    DMU_POOL_DIRECTORY_OBJECT,
+		    DMU_POOL_BOOKMARK_V2_RECALCULATED) == 0);
+		if (bv2_enabled && !recalculated) {
 			dsl_pool_sync_bookmark_featureflags(dp, tx);
+			uint64_t one = 1;
+			VERIFY0(zap_add(spa->spa_meta_objset,
+			    DMU_POOL_DIRECTORY_OBJECT,
+			    DMU_POOL_BOOKMARK_V2_RECALCULATED,
+			    sizeof (one), 1, &one, tx));
 		}
 	}