Rob's feedback

Signed-off-by: Paul Dagnelie <[email protected]>

Author: Paul Dagnelie

cmd/zdb/zdb.c

@@ -4899,7 +4899,7 @@ dump_l2arc_header(int fd)
 	int error = B_FALSE;
 
 	if (pread64(fd, &l2dhdr, sizeof (l2dhdr),
-	    VDEV_LABEL_START_SIZE(B_FALSE)) != sizeof (l2dhdr)) {
+	    VDEV_OLD_LABEL_START_SIZE) != sizeof (l2dhdr)) {
 		error = B_TRUE;
 	} else {
 		if (l2dhdr.dh_magic == BSWAP_64(L2ARC_DEV_HDR_MAGIC))
@@ -5494,7 +5494,7 @@ dump_label(const char *dev)
 		if (large_label) {
 			char toc_buf[VDEV_TOC_SIZE];
 			if (pread64(fd, toc_buf, VDEV_TOC_SIZE,
-			    label->label_offset + VDEV_NEW_PAD_SIZE) !=
+			    label->label_offset + VDEV_LARGE_PAD_SIZE) !=
 			    VDEV_TOC_SIZE) {
 				if (!dump_opt['q'])
 					(void) printf("failed to read label "
@@ -5506,7 +5506,7 @@ dump_label(const char *dev)
 
 			label->cksum_valid =
 			    phys_cksum_valid(toc_buf,
-			    label->label_offset + VDEV_NEW_PAD_SIZE,
+			    label->label_offset + VDEV_LARGE_PAD_SIZE,
 			    VDEV_TOC_SIZE);
 
 			label->read_failed = B_FALSE;
@@ -5552,7 +5552,7 @@ dump_label(const char *dev)
 			buf = alloca(conf_size);
 			buflen = conf_size;
 			uint64_t phys_off = label->label_offset +
-			    VDEV_NEW_PAD_SIZE + toc_size + bootenv_size;
+			    VDEV_LARGE_PAD_SIZE + toc_size + bootenv_size;
 			if (pread64(fd, buf, conf_size, phys_off) !=
 			    conf_size) {
 				if (!dump_opt['q'])

cmd/ztest.c

@@ -6667,8 +6667,7 @@ ztest_fault_inject(ztest_ds_t *zd, uint64_t id)
 		 * odd label, so that we can handle crashes in the
 		 * middle of vdev_config_sync().
 		 */
-		boolean_t new = vdrand->vdev_large_label;
-		if ((leaf & 1) == 0 && offset < VDEV_LABEL_START_SIZE(new))
+		if ((leaf & 1) == 0 && offset < VDEV_LABEL_START_SIZE(vdrand))
 			continue;
 
 		/*
@@ -6677,10 +6676,10 @@ ztest_fault_inject(ztest_ds_t *zd, uint64_t id)
 		 * sizeof (vdev_label_t).
 		 */
 		uint64_t psize = P2ALIGN_TYPED(fsize,
-		    new ? VDEV_LARGE_LABEL_ALIGN : sizeof (vdev_label_t),
-		    uint64_t);
+		    vdrand->vdev_large_label ? VDEV_LARGE_LABEL_ALIGN :
+		    sizeof (vdev_label_t), uint64_t);
 		if ((leaf & 1) == 1 && offset + sizeof (bad) >
-		    psize - VDEV_LABEL_END_SIZE(new))
+		    psize - VDEV_LABEL_END_SIZE(vdrand))
 			continue;
 
 		if (mirror_save != zs->zs_mirrors) {

include/sys/vdev_impl.h

@@ -432,6 +432,13 @@ struct vdev {
 	uint64_t	vdev_mmp_kstat_id;	/* to find kstat entry */
 	uint64_t	vdev_expansion_time;	/* vdev's last expansion time */
 	list_node_t	vdev_leaf_node;		/* leaf vdev list */
+	/*
+	 * vdev_large_label has different meanings for leaf and non-leaf vdevs.
+	 * For leaf vdevs, it is true if that specific vdev is using the large
+	 * label format. For non-leaf vdevs, it is true if any of its children
+	 * is using the new format, so that we know if we need to invoke the
+	 * large label sync logic.
+	 */
 	boolean_t	vdev_large_label;
 
 	kmutex_t	vdev_be_lock;
@@ -548,11 +555,13 @@ typedef struct vdev_label {
 } vdev_label_t;						/* 256K total */
 
 /*
- * The new label format is intended to help future-proof ZFS as sector sizes
- * grow. The number of uberblocks that can be safely written is limited by the
- * size of the ring divided by the sector size, which is already getting
- * uncomfortably small. The new label is only used on top-level vdevs and their
- * children; l2arc and spare devices are excluded. The new label layout:
+ * The large label format was introduced to help future-proof ZFS as sector
+ * sizes grow. The number of uberblocks that can be safely written is limited
+ * by the size of the ring divided by the sector size, which in the original
+ * format was already getting uncomfortably small. The new label is only used
+ * on top-level vdevs and their children; l2arc and spare devices are excluded.
+ *
+ * Layout of the large label format:
  *
  *   16 MiB             112 MiB                           128 MiB
  * +---------+--------------------------------+-------------------------------+
@@ -570,9 +579,9 @@ typedef struct vdev_label {
  * sub-section is larger than 16MiB, it will be split in 16MiB - sizeof
  * (zio_eck_t) chunks, which will each have their own checksum.
  */
-#define	VDEV_NEW_PAD_SIZE	(1 << 24) // 16MiB
-#define	VDEV_NEW_DATA_SIZE	((1 << 27) - VDEV_NEW_PAD_SIZE)
-#define	VDEV_LARGE_LABEL_SIZE	(VDEV_NEW_PAD_SIZE + VDEV_NEW_DATA_SIZE + \
+#define	VDEV_LARGE_PAD_SIZE	(1 << 24) // 16MiB
+#define	VDEV_LARGE_DATA_SIZE	((1 << 27) - VDEV_LARGE_PAD_SIZE)
+#define	VDEV_LARGE_LABEL_SIZE	(VDEV_LARGE_PAD_SIZE + VDEV_LARGE_DATA_SIZE + \
 	VDEV_LARGE_UBERBLOCK_RING) // 256MiB per label
 #define	VDEV_LARGE_LABEL_ALIGN	(1 << 24) // 16MiB
 
@@ -587,9 +596,13 @@ typedef struct vdev_label {
  * when we need to read this info.
  */
 #define	VDEV_TOC_TOC_SIZE	"toc_size"
+/* The size of the section that stores the boot region */
 #define	VDEV_TOC_BOOT_REGION	"boot_region"
+/* The size of the section that stores the vdev config */
 #define	VDEV_TOC_VDEV_CONFIG	"vdev_config"
+/* The size of the section that stores the pool config */
 #define	VDEV_TOC_POOL_CONFIG	"pool_config"
+/* The size of the section that stores auxilliary uberblocks */
 #define	VDEV_TOC_AUX_UBERBLOCK	"aux_uberblock"
 
 /*
@@ -603,24 +616,31 @@ typedef struct vdev_label {
 /*
  * Size of embedded boot loader region on each label.
  * The total size of the first two labels plus the boot area is 4MB.
- * On RAIDZ, this space is overwritten during RAIDZ expansion.
+ * On RAIDZ, this space is overwritten durinvg RAIDZ expansion.
  */
 #define	VDEV_BOOT_SIZE		(7ULL << 19)			/* 3.5M */
 
 /*
  * Size of label regions at the start and end of each leaf device.
  */
-#define	VDEV_LABEL_START_SIZE(new)	(new ? \
-	VDEV_RESERVE_OFFSET + VDEV_RESERVE_SIZE : \
-	2 * sizeof (vdev_label_t) + VDEV_BOOT_SIZE)
-#define	VDEV_LABEL_END_SIZE(new)	(new ? \
-	2 * VDEV_LARGE_LABEL_SIZE : 2 * sizeof (vdev_label_t))
+#define	VDEV_OLD_LABEL_START_SIZE	(2 * sizeof (vdev_label_t) + \
+	VDEV_BOOT_SIZE)
+#define	VDEV_OLD_LABEL_END_SIZE		(2 * sizeof (vdev_label_t))
+
+#define	VDEV_LARGE_LABEL_START_SIZE	(VDEV_RESERVE_OFFSET + \
+	VDEV_RESERVE_SIZE)
+#define	VDEV_LARGE_LABEL_END_SIZE	(2 * VDEV_LARGE_LABEL_SIZE)
+
+#define	VDEV_LABEL_START_SIZE(vd)	((vd)->vdev_large_label ? \
+	VDEV_LARGE_LABEL_START_SIZE : VDEV_OLD_LABEL_START_SIZE)
+#define	VDEV_LABEL_END_SIZE(vd)	((vd)->vdev_large_label ? \
+	VDEV_LARGE_LABEL_END_SIZE : VDEV_OLD_LABEL_END_SIZE)
+
 #define	VDEV_LABELS		4
 #define	VDEV_BEST_LABEL		VDEV_LABELS
-#define	VDEV_OFFSET_IS_LABEL(vd, off)                           	\
-	(((off) < VDEV_LABEL_START_SIZE(vd->vdev_large_label)) ||	\
-	((off) >= ((vd)->vdev_psize -					\
-	VDEV_LABEL_END_SIZE(vd->vdev_large_label))))
+#define	VDEV_OFFSET_IS_LABEL(vd, off)				\
+	(((off) < VDEV_LABEL_START_SIZE(vd)) ||			\
+	((off) >= ((vd)->vdev_psize - VDEV_LABEL_END_SIZE(vd))))
 
 #define	VDEV_ALLOC_LOAD		0
 #define	VDEV_ALLOC_ADD		1

lib/libzfs/libzfs.abi

@@ -638,7 +638,7 @@
     <elf-symbol name='fletcher_4_superscalar_ops' size='128' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
     <elf-symbol name='libzfs_config_ops' size='16' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
     <elf-symbol name='sa_protocol_names' size='16' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
-    <elf-symbol name='spa_feature_table' size='2576' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
+    <elf-symbol name='spa_feature_table' size='2632' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
     <elf-symbol name='zfeature_checks_disable' size='4' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
     <elf-symbol name='zfs_deleg_perm_tab' size='528' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
     <elf-symbol name='zfs_history_event_names' size='328' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
@@ -9606,8 +9606,8 @@
     </function-decl>
   </abi-instr>
   <abi-instr address-size='64' path='module/zcommon/zfeature_common.c' language='LANG_C99'>
-    <array-type-def dimensions='1' type-id='83f29ca2' size-in-bits='20608' id='b9408bab'>
-      <subrange length='46' type-id='7359adad' id='8b86bc1b'/>
+    <array-type-def dimensions='1' type-id='83f29ca2' size-in-bits='21056' id='fd43354e'>
+      <subrange length='47' type-id='7359adad' id='8f8900fe'/>
     </array-type-def>
     <enum-decl name='zfeature_flags' id='6db816a4'>
       <underlying-type type-id='9cac1fee'/>
@@ -9685,7 +9685,7 @@
     <pointer-type-def type-id='611586a1' size-in-bits='64' id='2e243169'/>
     <qualified-type-def type-id='eaa32e2f' const='yes' id='83be723c'/>
     <pointer-type-def type-id='83be723c' size-in-bits='64' id='7acd98a2'/>
-    <var-decl name='spa_feature_table' type-id='b9408bab' mangled-name='spa_feature_table' visibility='default' elf-symbol-id='spa_feature_table'/>
+    <var-decl name='spa_feature_table' type-id='fd43354e' mangled-name='spa_feature_table' visibility='default' elf-symbol-id='spa_feature_table'/>
     <var-decl name='zfeature_checks_disable' type-id='c19b74c3' mangled-name='zfeature_checks_disable' visibility='default' elf-symbol-id='zfeature_checks_disable'/>
     <function-decl name='opendir' visibility='default' binding='global' size-in-bits='64'>
       <parameter type-id='80f4b756'/>

lib/libzfs/libzfs_import.c

@@ -208,8 +208,7 @@ zpool_clear_label(int fd)
 		    "label < l2arc_dev_hdr_phys_t");
 		memset(label, 0, sizeof (l2arc_dev_hdr_phys_t));
 		if (pwrite64(fd, label, sizeof (l2arc_dev_hdr_phys_t),
-		    VDEV_LABEL_START_SIZE(B_FALSE)) ==
-		    sizeof (l2arc_dev_hdr_phys_t))
+		    VDEV_OLD_LABEL_START_SIZE) == sizeof (l2arc_dev_hdr_phys_t))
 			header_cleared = B_TRUE;
 	}
 

module/os/freebsd/zfs/vdev_label_os.c

@@ -107,7 +107,7 @@ vdev_check_boot_reserve(spa_t *spa, vdev_t *childvd)
 	 * offset lets us access the boot area.
 	 */
 	zio_nowait(zio_vdev_child_io(pio, NULL, childvd,
-	    VDEV_BOOT_OFFSET - VDEV_LABEL_START_SIZE(childvd->vdev_large_label),
+	    VDEV_BOOT_OFFSET - VDEV_LABEL_START_SIZE(childvd),
 	    abd, size, ZIO_TYPE_READ, ZIO_PRIORITY_ASYNC_READ, 0,
 	    vdev_child_done, pio));
 	zio_wait(pio);

module/zfs/arc.c

@@ -6332,12 +6332,10 @@ arc_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
 				} else {
 					abd = hdr_abd;
 				}
-				boolean_t large_label = vd->vdev_large_label;
 
-				ASSERT(addr >=
-				    VDEV_LABEL_START_SIZE(large_label) &&
+				ASSERT(addr >= VDEV_LABEL_START_SIZE(vd) &&
 				    addr + asize <= vd->vdev_psize -
-				    VDEV_LABEL_END_SIZE(large_label));
+				    VDEV_LABEL_END_SIZE(vd));
 
 				/*
 				 * l2arc read.  The SCL_L2ARC lock will be
@@ -9173,7 +9171,7 @@ l2arc_evict(l2arc_dev_t *dev, uint64_t distance, boolean_t all)
 				 */
 				spa_config_exit(dev->l2ad_spa, SCL_L2ARC, dev);
 				vdev_trim_simple(vd, dev->l2ad_evict -
-				    VDEV_LABEL_START_SIZE(vd->vdev_large_label),
+				    VDEV_LABEL_START_SIZE(vd),
 				    taddr - dev->l2ad_evict);
 				spa_config_enter(dev->l2ad_spa, SCL_L2ARC, dev,
 				    RW_READER);
@@ -9979,10 +9977,8 @@ l2arc_add_vdev(spa_t *spa, vdev_t *vd)
 	/* leave extra size for an l2arc device header */
 	l2dhdr_asize = adddev->l2ad_dev_hdr_asize =
 	    MAX(sizeof (*adddev->l2ad_dev_hdr), 1 << vd->vdev_ashift);
-	adddev->l2ad_start = VDEV_LABEL_START_SIZE(vd->vdev_large_label) +
-	    l2dhdr_asize;
-	adddev->l2ad_end = VDEV_LABEL_START_SIZE(vd->vdev_large_label) +
-	    vdev_get_min_asize(vd);
+	adddev->l2ad_start = VDEV_LABEL_START_SIZE(vd) + l2dhdr_asize;
+	adddev->l2ad_end = VDEV_LABEL_START_SIZE(vd) + vdev_get_min_asize(vd);
 	ASSERT3U(adddev->l2ad_start, <, adddev->l2ad_end);
 	adddev->l2ad_hand = adddev->l2ad_start;
 	adddev->l2ad_evict = adddev->l2ad_start;
@@ -10541,17 +10537,17 @@ l2arc_dev_hdr_read(l2arc_dev_t *dev)
 	const uint64_t		l2dhdr_asize = dev->l2ad_dev_hdr_asize;
 	abd_t 			*abd;
 	vdev_t			*vd = dev->l2ad_vdev;
-	boolean_t 		large_label = vd ? vd->vdev_large_label :
-	    B_FALSE;
+	uint64_t		offset = vd ? VDEV_LABEL_START_SIZE(vd) :
+	    VDEV_OLD_LABEL_START_SIZE;
 
 	guid = spa_guid(dev->l2ad_vdev->vdev_spa);
 
 	abd = abd_get_from_buf(l2dhdr, l2dhdr_asize);
 
-	err = zio_wait(zio_read_phys(NULL, dev->l2ad_vdev,
-	    VDEV_LABEL_START_SIZE(large_label), l2dhdr_asize, abd,
-	    ZIO_CHECKSUM_LABEL, NULL, NULL, ZIO_PRIORITY_SYNC_READ,
-	    ZIO_FLAG_CANFAIL | ZIO_FLAG_DONT_PROPAGATE | ZIO_FLAG_DONT_RETRY |
+	err = zio_wait(zio_read_phys(NULL, dev->l2ad_vdev, offset,
+	    l2dhdr_asize, abd, ZIO_CHECKSUM_LABEL, NULL, NULL,
+	    ZIO_PRIORITY_SYNC_READ, ZIO_FLAG_CANFAIL |
+	    ZIO_FLAG_DONT_PROPAGATE | ZIO_FLAG_DONT_RETRY |
 	    ZIO_FLAG_SPECULATIVE, B_FALSE));
 
 	abd_free(abd);
@@ -10905,8 +10901,8 @@ l2arc_dev_hdr_update(l2arc_dev_t *dev)
 	abd_t			*abd;
 	int			err;
 	vdev_t			*vd = dev->l2ad_vdev;
-	boolean_t 		large_label = vd ? vd->vdev_large_label :
-	    B_FALSE;
+	uint64_t		offset = vd ? VDEV_LABEL_START_SIZE(vd) :
+	    VDEV_OLD_LABEL_START_SIZE;
 
 	VERIFY(spa_config_held(dev->l2ad_spa, SCL_STATE_ALL, RW_READER));
 
@@ -10929,9 +10925,8 @@ l2arc_dev_hdr_update(l2arc_dev_t *dev)
 	abd = abd_get_from_buf(l2dhdr, l2dhdr_asize);
 
 	err = zio_wait(zio_write_phys(NULL, dev->l2ad_vdev,
-	    VDEV_LABEL_START_SIZE(large_label), l2dhdr_asize, abd,
-	    ZIO_CHECKSUM_LABEL, NULL, NULL, ZIO_PRIORITY_ASYNC_WRITE,
-	    ZIO_FLAG_CANFAIL, B_FALSE));
+	    offset, l2dhdr_asize, abd, ZIO_CHECKSUM_LABEL, NULL, NULL,
+	    ZIO_PRIORITY_ASYNC_WRITE, ZIO_FLAG_CANFAIL, B_FALSE));
 
 	abd_free(abd);
 

module/zfs/vdev.c

@@ -1933,7 +1933,7 @@ vdev_probe(vdev_t *vd, zio_t *zio)
 		offset_t offset;
 		if (vd->vdev_large_label) {
 			size = P2ROUNDUP(VDEV_TOC_SIZE, 1 << vd->vdev_ashift);
-			offset = VDEV_NEW_PAD_SIZE;
+			offset = VDEV_LARGE_PAD_SIZE;
 		} else {
 			size = VDEV_PAD_SIZE;
 			offset = offsetof(vdev_label_t, vl_be);
@@ -2272,15 +2272,16 @@ vdev_open(vdev_t *vd)
 			    VDEV_AUX_TOO_SMALL);
 			return (SET_ERROR(EOVERFLOW));
 		}
+		uint64_t ssize = large_label ? VDEV_LARGE_LABEL_START_SIZE :
+		    VDEV_OLD_LABEL_START_SIZE;
+		uint64_t esize = large_label ? VDEV_LARGE_LABEL_END_SIZE :
+		    VDEV_OLD_LABEL_END_SIZE;
 		psize = osize;
-		asize = osize - (VDEV_LABEL_START_SIZE(large_label) +
-		    VDEV_LABEL_END_SIZE(large_label));
-		max_asize = max_osize - (VDEV_LABEL_START_SIZE(large_label) +
-		    VDEV_LABEL_END_SIZE(large_label));
+		asize = osize - (ssize + esize);
+		max_asize = max_osize - (ssize + esize);
 	} else {
 		if (vd->vdev_parent != NULL && osize < SPA_MINDEVSIZE -
-		    (VDEV_LABEL_START_SIZE(B_FALSE) +
-		    VDEV_LABEL_END_SIZE(B_FALSE))) {
+		    (VDEV_OLD_LABEL_START_SIZE + VDEV_OLD_LABEL_END_SIZE)) {
 			vdev_set_state(vd, B_TRUE, VDEV_STATE_CANT_OPEN,
 			    VDEV_AUX_TOO_SMALL);
 			return (SET_ERROR(EOVERFLOW));
@@ -4923,9 +4924,8 @@ vdev_get_stats_ex(vdev_t *vd, vdev_stat_t *vs, vdev_stat_ex_t *vsx)
 
 		if (vd->vdev_ops->vdev_op_leaf) {
 			vs->vs_pspace = vd->vdev_psize;
-			vs->vs_rsize +=
-			    VDEV_LABEL_START_SIZE(vd->vdev_large_label) +
-			    VDEV_LABEL_END_SIZE(vd->vdev_large_label);
+			vs->vs_rsize += VDEV_LABEL_START_SIZE(vd) +
+			    VDEV_LABEL_END_SIZE(vd);
 			/*
 			 * Report initializing progress. Since we don't
 			 * have the initializing locks held, this is only

module/zfs/vdev_draid.c

@@ -2480,11 +2480,9 @@ vdev_draid_spare_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 	vdev_draid_calculate_asize(tvd, &asize, &max_asize,
 	    logical_ashift, physical_ashift);
 
-	*psize = asize + VDEV_LABEL_START_SIZE(vd->vdev_large_label) +
-	    VDEV_LABEL_END_SIZE(vd->vdev_large_label);
-	*max_psize = max_asize +
-	    VDEV_LABEL_START_SIZE(vd->vdev_large_label) +
-	    VDEV_LABEL_END_SIZE(vd->vdev_large_label);
+	*psize = asize + VDEV_LABEL_START_SIZE(vd) + VDEV_LABEL_END_SIZE(vd);
+	*max_psize = max_asize + VDEV_LABEL_START_SIZE(vd) +
+	    VDEV_LABEL_END_SIZE(vd);
 
 	vds->vds_draid_vdev = tvd;
 	vd->vdev_nonrot = tvd->vdev_nonrot;
@@ -2586,8 +2584,7 @@ vdev_draid_spare_io_start(zio_t *zio)
 {
 	vdev_t *cvd = NULL, *vd = zio->io_vd;
 	vdev_draid_spare_t *vds = vd->vdev_tsd;
-	uint64_t offset = zio->io_offset -
-	    VDEV_LABEL_START_SIZE(vd->vdev_large_label);
+	uint64_t offset = zio->io_offset - VDEV_LABEL_START_SIZE(vd);
 
 	/*
 	 * If the vdev is closed, it's likely in the REMOVED or FAULTED state.

module/zfs/vdev_indirect.c

@@ -952,8 +952,7 @@ vdev_indirect_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
     uint64_t *logical_ashift, uint64_t *physical_ashift)
 {
 	*psize = *max_psize = vd->vdev_asize +
-	    VDEV_LABEL_START_SIZE(vd->vdev_large_label) +
-	    VDEV_LABEL_END_SIZE(vd->vdev_large_label);
+	    VDEV_LABEL_START_SIZE(vd) + VDEV_LABEL_END_SIZE(vd);
 	*logical_ashift = vd->vdev_ashift;
 	*physical_ashift = vd->vdev_physical_ashift;
 	return (0);