Revert feature flags for rename2

This means in cases where a pool is not cleanly exported you may end up with loss of unflushed TX_RENAME_EXCHANGE and TX_RENAME_WHITEOUT transactions and all transactions that follow them in the ZIL for that dataset if the pool is then imported by a release build of ZFS that does not recognize the new TX types. A warning message is logged to dmesg and the pool is fully usable, just missing the last changes. Debug builds will panic when they fail to replay the full log. It's also possible a different new TX type might land upstream before this, in which case similar results are likely to be encountered. The alternative is a pool that cannot be imported on any version of ZFS lacking support for TX_RENAME_EXCHANGE and TX_RENAME_WHITEOUT replay when uncleanly exported. That prevents people from either upgrading their pool for the ability to use overlayfs features or going back to a previous boot environment after having upgraded the pool. Signed-off-by: Ryan Moeller <[email protected]>
truenas · Oct 19, 2022 · fbb9b45 · fbb9b45
1 parent 0abd108
commit fbb9b45
Show file tree

Hide file tree

Showing 11 changed files with 0 additions and 253 deletions.
diff --git a/include/zfeature_common.h b/include/zfeature_common.h
@@ -75,8 +75,6 @@ typedef enum spa_feature {
 	SPA_FEATURE_DEVICE_REBUILD,
 	SPA_FEATURE_ZSTD_COMPRESS,
 	SPA_FEATURE_DRAID,
-	SPA_FEATURE_RENAME_EXCHANGE,
-	SPA_FEATURE_RENAME_WHITEOUT,
 	SPA_FEATURES
 } spa_feature_t;
 

diff --git a/module/os/linux/zfs/zfs_vnops_os.c b/module/os/linux/zfs/zfs_vnops_os.c
@@ -68,7 +68,6 @@
 #include <sys/cred.h>
 #include <sys/zpl.h>
 #include <sys/zil.h>
-#include <sys/zfeature.h>
 #include <sys/sa_impl.h>
 
 /*
@@ -2597,70 +2596,6 @@ zfs_setattr(znode_t *zp, vattr_t *vap, int flags, cred_t *cr)
 	return (err);
 }
 
-typedef struct zfs_activate_feature_arg {
-	const char *name;
-	spa_feature_t feature;
-} zfs_activate_feature_arg;
-
-static int
-zfs_activate_feature_check(void *arg, dmu_tx_t *tx)
-{
-	zfs_activate_feature_arg *zlefa = arg;
-	spa_feature_t f = zlefa->feature;
-
-	dsl_pool_t *dp = dmu_tx_pool(tx);
-
-	if (f == SPA_FEATURE_NONE)
-		return (SET_ERROR(EINVAL));
-
-	if (!spa_feature_is_enabled(dp->dp_spa, f))
-		return (SET_ERROR(ENOTSUP));
-
-	return (0);
-}
-
-static void
-zfs_activate_feature_sync(void *arg, dmu_tx_t *tx)
-{
-	zfs_activate_feature_arg *zlefa = arg;
-	spa_feature_t f = zlefa->feature;
-
-	dsl_pool_t *dp = dmu_tx_pool(tx);
-	dsl_dataset_t *ds = NULL;
-
-	ASSERT3S(spa_feature_table[f].fi_type, ==, ZFEATURE_TYPE_BOOLEAN);
-	VERIFY0(dsl_dataset_hold(dp, zlefa->name, FTAG, &ds));
-	if (dsl_dataset_feature_is_active(ds, f) != B_TRUE) {
-		ds->ds_feature_activation[f] = (void *)B_TRUE;
-		dsl_dataset_activate_feature(ds->ds_object, f,
-		    ds->ds_feature_activation[f], tx);
-		ds->ds_feature[f] = ds->ds_feature_activation[f];
-	}
-	dsl_dataset_rele(ds, FTAG);
-}
-
-static int
-zfs_activate_feature(spa_t *spa, spa_feature_t feature)
-{
-	zfs_activate_feature_arg zlefa = {
-		.name = spa_name(spa),
-		.feature = feature,
-	};
-
-	ASSERT(spa_feature_is_enabled(spa, feature));
-	if (spa_feature_is_active(spa, feature))
-		return (0);
-
-	/*
-	 * We need to wait for the activation to sync out before we continue
-	 * with the rename operation, so that when we create the ZIL entry the
-	 * dataset already has the feature activated.
-	 */
-	return (dsl_sync_task(spa_name(spa), zfs_activate_feature_check,
-	    zfs_activate_feature_sync, &zlefa, 0,
-	    ZFS_SPACE_CHECK_EXTRA_RESERVED));
-}
-
 typedef struct zfs_zlock {
 	krwlock_t	*zl_rwlock;	/* lock we acquired */
 	znode_t		*zl_znode;	/* znode we held */
@@ -2787,7 +2722,6 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,
 	zfs_dirlock_t	*sdl, *tdl;
 	dmu_tx_t	*tx;
 	zfs_zlock_t	*zl;
-	spa_feature_t   zil_feature = SPA_FEATURE_NONE;
 	int		cmp, serr, terr;
 	int		error = 0;
 	int		zflg = 0;
@@ -2824,21 +2758,6 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,
 	ZFS_VERIFY_ZP(tdzp);
 	zilog = zfsvfs->z_log;
 
-	switch (rflags & (RENAME_EXCHANGE | RENAME_WHITEOUT)) {
-	case RENAME_EXCHANGE:
-		zil_feature = SPA_FEATURE_RENAME_EXCHANGE;
-		break;
-	case RENAME_WHITEOUT:
-		zil_feature = SPA_FEATURE_RENAME_WHITEOUT;
-		break;
-	}
-
-	if (zil_feature != SPA_FEATURE_NONE &&
-	    !spa_feature_is_enabled(zfsvfs->z_os->os_spa, zil_feature)) {
-		ZFS_EXIT(zfsvfs);
-		return (SET_ERROR(EINVAL));
-	}
-
 	/*
 	 * We check i_sb because snapshots and the ctldir must have different
 	 * super blocks.
@@ -3079,19 +2998,6 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,
 		}
 	}
 
-	/*
-	 * Before the dmu_tx is created, activate the RENAME_* feature flags.
-	 * We want to do this as late as possible, to avoid activating the
-	 * feature if the operation is likely to fail, but we have to do it
-	 * before dmu_tx_assign() because we need the feature flag activation
-	 * to sync out before we create the ZIL entry.
-	 */
-	if (zil_feature != SPA_FEATURE_NONE) {
-		error = zfs_activate_feature(zfsvfs->z_os->os_spa, zil_feature);
-		if (error)
-			goto out;
-	}
-
 	tx = dmu_tx_create(zfsvfs->z_os);
 	dmu_tx_hold_sa(tx, szp->z_sa_hdl, B_FALSE);
 	dmu_tx_hold_sa(tx, sdzp->z_sa_hdl, B_FALSE);

diff --git a/module/zcommon/zfeature_common.c b/module/zcommon/zfeature_common.c
@@ -598,31 +598,6 @@ zpool_feature_init(void)
 	zfeature_register(SPA_FEATURE_DRAID,
 	    "org.openzfs:draid", "draid", "Support for distributed spare RAID",
 	    ZFEATURE_FLAG_MOS, ZFEATURE_TYPE_BOOLEAN, NULL);
-
-#ifdef __linux__
-	{
-		static const spa_feature_t rename_exchange_deps[] = {
-			SPA_FEATURE_EXTENSIBLE_DATASET,
-			SPA_FEATURE_NONE
-		};
-		zfeature_register(SPA_FEATURE_RENAME_EXCHANGE,
-		    "org.openzfs:rename_exchange", "rename_exchange",
-		    "Support for renameat2(RENAME_EXCHANGE).",
-		    ZFEATURE_FLAG_READONLY_COMPAT | ZFEATURE_FLAG_PER_DATASET,
-		    ZFEATURE_TYPE_BOOLEAN, rename_exchange_deps);
-	}
-	{
-		static const spa_feature_t rename_whiteout_deps[] = {
-			SPA_FEATURE_EXTENSIBLE_DATASET,
-			SPA_FEATURE_NONE
-		};
-		zfeature_register(SPA_FEATURE_RENAME_WHITEOUT,
-		    "org.openzfs:rename_whiteout", "rename_whiteout",
-		    "Support for renameat2(RENAME_WHITEOUT).",
-		    ZFEATURE_FLAG_READONLY_COMPAT | ZFEATURE_FLAG_PER_DATASET,
-		    ZFEATURE_TYPE_BOOLEAN, rename_whiteout_deps);
-	}
-#endif
 }
 
 #if defined(_KERNEL)

diff --git a/module/zfs/zfs_log.c b/module/zfs/zfs_log.c
@@ -45,7 +45,6 @@
 #include <sys/spa.h>
 #include <sys/zfs_fuid.h>
 #include <sys/dsl_dataset.h>
-#include <sys/zfeature.h>
 
 /*
  * These zfs_log_* functions must be called within a dmu tx, in one
@@ -538,12 +537,6 @@ zfs_log_rename_exchange(zilog_t *zilog, dmu_tx_t *tx, uint64_t txtype,
     znode_t *sdzp, const char *sname, znode_t *tdzp, const char *dname,
     znode_t *szp)
 {
-	spa_t *spa = zilog->zl_spa;
-
-	/* zfs_rename must have already activated this feature. */
-	VERIFY(spa_feature_is_enabled(spa, SPA_FEATURE_RENAME_EXCHANGE));
-	VERIFY(spa_feature_is_active(spa, SPA_FEATURE_RENAME_EXCHANGE));
-
 	txtype |= TX_RENAME_EXCHANGE;
 	do_zfs_log_rename(zilog, tx, txtype, sdzp, sname, tdzp, dname, szp);
 }
@@ -560,15 +553,10 @@ zfs_log_rename_whiteout(zilog_t *zilog, dmu_tx_t *tx, uint64_t txtype,
     znode_t *szp, znode_t *wzp)
 {
 	itx_t *itx;
-	spa_t *spa = zilog->zl_spa;
 	lr_rename_whiteout_t *lr;
 	size_t snamesize = strlen(sname) + 1;
 	size_t dnamesize = strlen(dname) + 1;
 
-	/* zfs_rename must have already activated this feature. */
-	VERIFY(spa_feature_is_enabled(spa, SPA_FEATURE_RENAME_WHITEOUT));
-	VERIFY(spa_feature_is_active(spa, SPA_FEATURE_RENAME_WHITEOUT));
-
 	if (zil_replaying(zilog, tx))
 		return;
 

diff --git a/module/zfs/zfs_replay.c b/module/zfs/zfs_replay.c
@@ -649,13 +649,6 @@ do_zfs_replay_rename(zfsvfs_t *zfsvfs, lr_rename_t *lr, char *sname,
 #ifdef __linux__
 	VERIFY0(rflags & ~(RENAME_EXCHANGE | RENAME_WHITEOUT));
 
-	VERIFY_IMPLY(rflags & RENAME_EXCHANGE,
-	    spa_feature_is_active(zfsvfs->z_os->os_spa,
-	    SPA_FEATURE_RENAME_EXCHANGE));
-	VERIFY_IMPLY(rflags & RENAME_WHITEOUT,
-	    spa_feature_is_active(zfsvfs->z_os->os_spa,
-	    SPA_FEATURE_RENAME_WHITEOUT));
-
 	/* wo_vap must be non-NULL iff. we're doing RENAME_WHITEOUT */
 	VERIFY_EQUIV(rflags & RENAME_WHITEOUT, wo_vap != NULL);
 #else

diff --git a/module/zfs/zil.c b/module/zfs/zil.c
@@ -3044,45 +3044,6 @@ zil_commit_impl(zilog_t *zilog, uint64_t foid)
 	zil_free_commit_waiter(zcw);
 }
 
-/*
- * Called as part of zil_sync once the replay succeeded successfully.
- *
- * For backwards-compatibility reasons, brand new TX_* records need to have a
- * feature flag associated with them so that a system will not be confused when
- * importing a pool with unknown TX_* types. These features are only activated
- * while the ZIL contains a log entry containing the new TX_* type, and need to
- * be deactivated when the ZIL has been applied and cleared (so that the pool
- * can be imported onto other systems after a clean 'zfs export').
- */
-static void
-zil_sync_deactivate_features(zilog_t *zilog, dmu_tx_t *tx, boolean_t keep_first)
-{
-	dsl_dataset_t *ds = dmu_objset_ds(zilog->zl_os);
-
-	/*
-	 * A destroyed ZIL chain can't contain any of the TX_* records which
-	 * are gated by these features. So, deactivate the features. They'll be
-	 * re-activated if the feature is needed again.
-	 */
-	spa_feature_t tx_features[] = {
-		SPA_FEATURE_RENAME_EXCHANGE,
-		SPA_FEATURE_RENAME_WHITEOUT,
-		/*
-		 * ZILSAXATTR is unconditionally activated for every ZIL, so if
-		 * we can only deactivate the feature if we're not going to
-		 * reuse this ZIL.
-		 */
-		keep_first ? SPA_FEATURE_NONE : SPA_FEATURE_ZILSAXATTR,
-	};
-	for (int i = 0; i < ARRAY_SIZE(tx_features); i++) {
-		spa_feature_t f = tx_features[i];
-		if (f == SPA_FEATURE_NONE)
-			continue;
-		if (dsl_dataset_feature_is_active(ds, f))
-			dsl_dataset_deactivate_feature(ds, f, tx);
-	}
-}
-
 /*
  * Called in syncing context to free committed log blocks and update log header.
  */
@@ -3132,8 +3093,6 @@ zil_sync(zilog_t *zilog, dmu_tx_t *tx)
 			zil_init_log_chain(zilog, &blk);
 			zh->zh_log = blk;
 		}
-
-		zil_sync_deactivate_features(zilog, tx, zilog->zl_keep_first);
 	}
 
 	while ((lwb = list_head(&zilog->zl_lwb_list)) != NULL) {

diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_get/zpool_get.cfg b/tests/zfs-tests/tests/functional/cli_root/zpool_get/zpool_get.cfg
@@ -104,10 +104,3 @@ if ! is_freebsd; then
 	    "feature@edonr"
 	)
 fi
-
-if is_linux; then
-	properties+=(
-		"feature@rename_exchange"
-		"feature@rename_whiteout"
-	)
-fi
diff --git a/tests/zfs-tests/tests/functional/renameat2/renameat2_exchange.ksh b/tests/zfs-tests/tests/functional/renameat2/renameat2_exchange.ksh
@@ -37,8 +37,6 @@ function cleanup
 log_assert "ZFS supports RENAME_EXCHANGE."
 log_onexit cleanup
 
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "enabled"
-
 cd $TESTDIR
 echo "foo" > foo
 echo "bar" > bar
@@ -49,23 +47,15 @@ log_must grep '^foo$' foo
 
 # Basic exchange.
 log_must renameat2 -x foo bar
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "active"
 log_must grep '^bar$' foo
 log_must grep '^foo$' bar
 
 # And exchange back.
 log_must renameat2 -x foo bar
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "active"
 log_must grep '^foo$' foo
 log_must grep '^bar$' bar
 
 # Exchange with a bad path should fail.
 log_mustnot renameat2 -x bar baz
 
-# The feature flag must remain active until a clean export.
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "active"
-log_must zpool export "$TESTPOOL1"
-log_must zpool import "$TESTPOOL1"
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "enabled"
-
 log_pass "ZFS supports RENAME_EXCHANGE as expected."
diff --git a/tests/zfs-tests/tests/functional/renameat2/renameat2_noreplace.ksh b/tests/zfs-tests/tests/functional/renameat2/renameat2_noreplace.ksh
@@ -37,9 +37,6 @@ function cleanup
 log_assert "ZFS supports RENAME_NOREPLACE."
 log_onexit cleanup
 
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "enabled"
-check_feature_flag "feature@rename_whiteout" "$TESTPOOL1" "enabled"
-
 cd $TESTDIR
 touch foo bar
 
@@ -51,8 +48,4 @@ log_mustnot renameat2 -n bar foo
 # Regular renames should succeed.
 log_must renameat2 -n bar baz
 
-# The other rename feature flags should not be active.
-check_feature_flag "feature@rename_exchange" "$TESTPOOL1" "enabled"
-check_feature_flag "feature@rename_whiteout" "$TESTPOOL1" "enabled"
-
 log_pass "ZFS supports RENAME_NOREPLACE as expected."
diff --git a/tests/zfs-tests/tests/functional/renameat2/renameat2_whiteout.ksh b/tests/zfs-tests/tests/functional/renameat2/renameat2_whiteout.ksh
@@ -37,23 +37,14 @@ function cleanup
 log_assert "ZFS supports RENAME_WHITEOUT."
 log_onexit cleanup
 
-check_feature_flag "feature@rename_whiteout" "$TESTPOOL1" "enabled"
-
 cd $TESTDIR
 echo "whiteout" > whiteout
 
 # Straight-forward rename-with-whiteout.
 log_must renameat2 -w whiteout new
-check_feature_flag "feature@rename_whiteout" "$TESTPOOL1" "active"
 # Check new file.
 log_must grep '^whiteout$' new
 # Check that the whiteout is actually a {0,0} char device.
 log_must grep '^character special file:0:0$' <<<"$(stat -c '%F:%t:%T' whiteout)"
 
-# The feature flag must remain active until a clean export.
-check_feature_flag "feature@rename_whiteout" "$TESTPOOL1" "active"
-log_must zpool export "$TESTPOOL1"
-log_must zpool import "$TESTPOOL1"
-check_feature_flag "feature@rename_whiteout" "$TESTPOOL1" "enabled"
-
 log_pass "ZFS supports RENAME_WHITEOUT as expected."