Replace the way we store the compression level

Rather than using a 2nd hidden property, compress_level, we store
the algorithm + bit shifted level in the regular compression property
in the dataset properties ZAP.

Introduce new macros:
ZIO_COMPRESS_ALGO(raw) extracts the algorithm from the property
ZIO_COMPRESS_LEVEL(raw) extracts the level from the property
ZIO_COMPRESS_RAW(algo, level) returns the combined value

This resolves issues around inheritence.

All of the internal ZFS code uses the separated _compress and
_complevel variables. Only the properties ZAP contains the
combined/bit-shifted value. The combined value is split when the
compression_changed_cb() callback is called, and sets both objset
members (os_compress and os_complevel).

The userspace tools all use the combined/bit-shifted value.

Signed-off-by: Allan Jude <allan@klarasystems.com>

Author: allanjude

include/sys/fs/zfs.h

@@ -186,7 +186,6 @@ typedef enum {
 	ZFS_PROP_IVSET_GUID,		/* not exposed to the user */
 	ZFS_PROP_REDACTED,
 	ZFS_PROP_REDACT_SNAPS,
-	ZFS_PROP_COMPRESS_LEVEL,	/* not exposed to the user */
 	ZFS_NUM_PROPS
 } zfs_prop_t;
 

include/sys/zio.h

@@ -163,8 +163,13 @@ enum zio_encrypt {
 	(compress) == ZIO_COMPRESS_ON ||		\
 	(compress) == ZIO_COMPRESS_OFF)
 
-#define	ZIO_COMPLEVEL_ZSTD(level) \
-	(ZIO_COMPRESS_ZSTD | ((level) << SPA_COMPRESSBITS))
+
+#define	ZIO_COMPRESS_ALGO(x)	(x & SPA_COMPRESSMASK)
+#define	ZIO_COMPRESS_LEVEL(x)	((x & ~SPA_COMPRESSMASK) >> SPA_COMPRESSBITS)
+#define	ZIO_COMPRESS_RAW(type, level)	(type | ((level) << SPA_COMPRESSBITS))
+
+#define	ZIO_COMPLEVEL_ZSTD(level)	\
+	ZIO_COMPRESS_RAW(ZIO_COMPRESS_ZSTD, level)
 
 #define	ZIO_FAILURE_MODE_WAIT		0
 #define	ZIO_FAILURE_MODE_CONTINUE	1

include/sys/zio_compress.h

@@ -57,6 +57,11 @@ enum zio_compress {
 	ZIO_COMPRESS_FUNCTIONS
 };
 
+/* Compression algorithms that have levels */
+#define ZIO_COMPRESS_HASLEVEL(compress)	((compress == ZIO_COMPRESS_ZSTD || \
+					(compress >= ZIO_COMPRESS_GZIP_1 && \
+					compress <= ZIO_COMPRESS_GZIP_9)))
+
 #define	ZIO_COMPLEVEL_INHERIT	0
 #define	ZIO_COMPLEVEL_DEFAULT	255
 

module/zcommon/zfs_prop.c

@@ -710,10 +710,6 @@ zfs_prop_init(void)
 	zprop_register_hidden(ZFS_PROP_REMAPTXG, "remaptxg", PROP_TYPE_NUMBER,
 	    PROP_READONLY, ZFS_TYPE_DATASET, "REMAPTXG");
 
-	zprop_register_impl(ZFS_PROP_COMPRESS_LEVEL, "compress_level",
-	    PROP_TYPE_NUMBER, ZIO_COMPLEVEL_INHERIT, NULL, PROP_INHERIT,
-	    ZFS_TYPE_DATASET, "<level>", "COMPLEVEL", B_TRUE,
-	    B_FALSE, NULL);
 	/* oddball properties */
 	zprop_register_impl(ZFS_PROP_CREATION, "creation", PROP_TYPE_NUMBER, 0,
 	    NULL, PROP_READONLY, ZFS_TYPE_DATASET | ZFS_TYPE_BOOKMARK,

module/zfs/dmu.c

@@ -2126,6 +2126,8 @@ dmu_write_policy(objset_t *os, dnode_t *dn, int level, int wp, zio_prop_t *zp)
 	} else {
 		compress = zio_compress_select(os->os_spa, dn->dn_compress,
 		    compress);
+		complevel = zio_complevel_select(os->os_spa, compress,
+		    complevel, complevel);
 
 		checksum = (dedup_checksum == ZIO_CHECKSUM_OFF) ?
 		    zio_checksum_select(dn->dn_checksum, checksum) :
@@ -2201,7 +2203,6 @@ dmu_write_policy(objset_t *os, dnode_t *dn, int level, int wp, zio_prop_t *zp)
 	    os->os_zpl_special_smallblock : 0;
 
 	ASSERT3U(zp->zp_compress, !=, ZIO_COMPRESS_INHERIT);
-	ASSERT3U(zp->zp_complevel, !=, ZIO_COMPLEVEL_INHERIT);
 }
 
 /*

module/zfs/dmu_objset.c

@@ -196,16 +196,9 @@ compression_changed_cb(void *arg, uint64_t newval)
 	ASSERT(newval != ZIO_COMPRESS_INHERIT);
 
 	os->os_compress = zio_compress_select(os->os_spa,
-	    newval & SPA_COMPRESSMASK, ZIO_COMPRESS_ON);
-}
-
-static void
-compress_level_changed_cb(void *arg, uint64_t newval)
-{
-	objset_t *os = arg;
-
+	    ZIO_COMPRESS_ALGO(newval), ZIO_COMPRESS_ON);
 	os->os_complevel = zio_complevel_select(os->os_spa, os->os_compress,
-	    newval, ZIO_COMPLEVEL_DEFAULT);
+	    ZIO_COMPRESS_LEVEL(newval), ZIO_COMPLEVEL_DEFAULT);
 }
 
 static void
@@ -540,11 +533,6 @@ dmu_objset_open_impl(spa_t *spa, dsl_dataset_t *ds, blkptr_t *bp,
 				    zfs_prop_to_name(ZFS_PROP_COMPRESSION),
 				    compression_changed_cb, os);
 			}
-			if (err == 0) {
-				err = dsl_prop_register(ds,
-				    zfs_prop_to_name(ZFS_PROP_COMPRESS_LEVEL),
-				    compress_level_changed_cb, os);
-			}
 			if (err == 0) {
 				err = dsl_prop_register(ds,
 				    zfs_prop_to_name(ZFS_PROP_COPIES),

module/zfs/zcp_get.c

@@ -15,8 +15,6 @@
 
 /*
  * Copyright (c) 2016 by Delphix. All rights reserved.
- * Copyright (c) 2019, Klara Inc.
- * Copyright (c) 2019, Allan Jude
  */
 
 #include <sys/lua/lua.h>
@@ -233,7 +231,6 @@ get_special_prop(lua_State *state, dsl_dataset_t *ds, const char *dsname,
 	char setpoint[ZFS_MAX_DATASET_NAME_LEN] =
 	    "Internal error - setpoint not determined";
 	zfs_type_t ds_type;
-	const char *prop_name = zfs_prop_to_name(zfs_prop);
 	zprop_type_t prop_type = zfs_prop_get_type(zfs_prop);
 	(void) get_objset_type(ds, &ds_type);
 
@@ -412,25 +409,7 @@ get_special_prop(lua_State *state, dsl_dataset_t *ds, const char *dsname,
 		nvlist_free(nvl);
 		break;
 	}
-	case ZFS_PROP_COMPRESSION:
-		error = dsl_prop_get_ds(ds, prop_name, sizeof (numval), 1,
-		    &numval, setpoint);
-		/* Special handling is only required for ZSTD */
-		if (error || numval != ZIO_COMPRESS_ZSTD)
-			break;
-
-		uint64_t levelval;
-		const char *complevel_name =
-		    zfs_prop_to_name(ZFS_PROP_COMPRESS_LEVEL);
 
-		error = dsl_prop_get_ds(ds, complevel_name, sizeof (levelval),
-		    1, &levelval, setpoint);
-		if (error == 0) {
-			if (levelval == ZIO_COMPLEVEL_DEFAULT)
-				break;
-			numval |= levelval << SPA_COMPRESSBITS;
-		}
-		break;
 	default:
 		/* Did not match these props, check in the dsl_dir */
 		error = get_dsl_dir_prop(ds, zfs_prop, &numval);

module/zfs/zfs_ioctl.c

@@ -2043,34 +2043,6 @@ zfs_ioc_vdev_setfru(zfs_cmd_t *zc)
 	return (error);
 }
 
-static int
-get_prop_uint64(nvlist_t *nv, const char *prop, nvlist_t **nvp,
-    uint64_t *val)
-{
-	int err = 0;
-	nvlist_t *subnv;
-	nvpair_t *pair;
-	nvpair_t *propval;
-
-	if (nvlist_lookup_nvpair(nv, prop, &pair) != 0)
-		return (EINVAL);
-
-	/* decode the property value */
-	propval = pair;
-	if (nvpair_type(pair) == DATA_TYPE_NVLIST) {
-		subnv = fnvpair_value_nvlist(pair);
-		if (nvp != NULL)
-			*nvp = subnv;
-		if (nvlist_lookup_nvpair(subnv, ZPROP_VALUE, &propval) != 0)
-			err = EINVAL;
-	}
-	if (nvpair_type(propval) == DATA_TYPE_UINT64) {
-		*val = fnvpair_value_uint64(propval);
-	}
-
-	return (err);
-}
-
 static int
 zfs_ioc_objset_stats_impl(zfs_cmd_t *zc, objset_t *os)
 {
@@ -2098,28 +2070,6 @@ zfs_ioc_objset_stats_impl(zfs_cmd_t *zc, objset_t *os)
 			}
 			VERIFY0(error);
 		}
-		/*
-		 * ZSTD stores the compression level in a separate hidden
-		 * property to avoid using up a large number of bits in the
-		 * on-disk compression algorithm enum. We need to swap things
-		 * back around when the property is read.
-		 */
-		nvlist_t *cnv;
-		uint64_t compval, levelval;
-
-		if (get_prop_uint64(nv, "compression", &cnv, &compval) != 0)
-			compval = ZIO_COMPRESS_INHERIT;
-
-		if (error == 0 && compval == ZIO_COMPRESS_ZSTD &&
-		    get_prop_uint64(nv, "compress_level", NULL,
-		    &levelval) == 0) {
-			if (levelval == ZIO_COMPLEVEL_DEFAULT)
-				levelval = 0;
-			fnvlist_remove(cnv, ZPROP_VALUE);
-			fnvlist_add_uint64(cnv, ZPROP_VALUE,
-			    compval | (levelval << SPA_COMPRESSBITS));
-		}
-
 		if (error == 0)
 			error = put_nvlist(zc, nv);
 		nvlist_free(nv);
@@ -2563,32 +2513,6 @@ zfs_prop_set_special(const char *dsname, zprop_source_t source,
 		}
 		break;
 	}
-	case ZFS_PROP_COMPRESSION:
-		/* Special handling is only required for ZSTD */
-		if ((intval & SPA_COMPRESSMASK) != ZIO_COMPRESS_ZSTD) {
-			err = -1;
-			break;
-		}
-		/*
-		 * Store the ZSTD compression level separate from the compress
-		 * property in its own hidden property.
-		 */
-		uint64_t levelval;
-
-		if (intval == ZIO_COMPRESS_ZSTD) {
-			levelval = ZIO_COMPLEVEL_DEFAULT;
-		} else {
-			levelval = (intval & ~SPA_COMPRESSMASK)
-			    >> SPA_COMPRESSBITS;
-		}
-		err = dsl_prop_set_int(dsname, "compress_level", source,
-		    levelval);
-		if (err == 0) {
-			/* Store the compression algorithm normally */
-			err = dsl_prop_set_int(dsname, propname, source,
-			    intval & SPA_COMPRESSMASK);
-		}
-		break;
 	default:
 		err = -1;
 	}
@@ -4448,7 +4372,7 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr)
 	const char *propname = nvpair_name(pair);
 	boolean_t issnap = (strchr(dsname, '@') != NULL);
 	zfs_prop_t prop = zfs_name_to_prop(propname);
-	uint64_t intval;
+	uint64_t intval, compval;
 	int err;
 
 	if (prop == ZPROP_INVAL) {
@@ -4530,19 +4454,20 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr)
 		 * we'll catch them later.
 		 */
 		if (nvpair_value_uint64(pair, &intval) == 0) {
-			if (intval >= ZIO_COMPRESS_GZIP_1 &&
-			    intval <= ZIO_COMPRESS_GZIP_9 &&
+			compval = ZIO_COMPRESS_ALGO(intval);
+			if (compval >= ZIO_COMPRESS_GZIP_1 &&
+			    compval <= ZIO_COMPRESS_GZIP_9 &&
 			    zfs_earlier_version(dsname,
 			    SPA_VERSION_GZIP_COMPRESSION)) {
 				return (SET_ERROR(ENOTSUP));
 			}
 
-			if (intval == ZIO_COMPRESS_ZLE &&
+			if (compval == ZIO_COMPRESS_ZLE &&
 			    zfs_earlier_version(dsname,
 			    SPA_VERSION_ZLE_COMPRESSION))
 				return (SET_ERROR(ENOTSUP));
 
-			if (intval == ZIO_COMPRESS_LZ4) {
+			if (compval == ZIO_COMPRESS_LZ4) {
 				spa_t *spa;
 
 				if ((err = spa_open(dsname, &spa, FTAG)) != 0)
@@ -4556,7 +4481,7 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr)
 				spa_close(spa, FTAG);
 			}
 
-			if (intval == ZIO_COMPRESS_ZSTD) {
+			if (compval == ZIO_COMPRESS_ZSTD) {
 				spa_t *spa;
 
 				if ((err = spa_open(dsname, &spa, FTAG)) != 0)
@@ -4578,7 +4503,7 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr)
 			 * implies a downrev pool version.
 			 */
 			if (zfs_is_bootfs(dsname) &&
-			    !BOOTFS_COMPRESS_VALID(intval)) {
+			    !BOOTFS_COMPRESS_VALID(compval)) {
 				return (SET_ERROR(ERANGE));
 			}
 		}

module/zfs/zio_compress.c

@@ -76,6 +76,9 @@ zio_complevel_select(spa_t *spa, enum zio_compress compress, uint8_t child,
 {
 	uint8_t result;
 
+	if (!ZIO_COMPRESS_HASLEVEL(compress))
+		return (0);
+
 	result = child;
 	if (result == ZIO_COMPLEVEL_INHERIT)
 		result = parent;