bdev_gpt: add new SPDK partition type for off-by-one fix

The gpt bdev module has an off-by-one error.  When it
calculates the size of the partition, it simply does
"end - start", when really it should be "end - start + 1".

We cannot just fix it by changing the math here, any
consumers of the partition may have put down metadata
on the partition based on the old size.

So instead add a new SPDK partition type.  SPDK will keep
the existing off-by-one behavior when it finds the old
partition type, but will use the correct math when finding
the new partition type.

Fixes issue #2801.

Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: I48eb48c781f1968b59e52b4477ca45e9c81eac11
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/16298
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: Paul Luse <paul.e.luse@intel.com>
Reviewed-by: Mike Gerdts <mgerdts@nvidia.com>
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>

deprecation.md

@@ -43,3 +43,18 @@ With the removal of this deprecation, calls to vbdev modules' `examine_disk()` a
 `examine_config()` callbacks will happen only on the app thread. This means that vbdev module
 maintainers will not need to make any changes to examine callbacks that call `spdk_bdev_register()`
 on the same thread as the examine callback uses.
+
+### gpt
+
+### `old_gpt_guid`
+
+Deprecated the SPDK partition type GUID `7c5222bd-8f5d-4087-9c00-bf9843c7b58c`. Partitions of this
+type have bdevs created that are one block less than the actual size of the partition. Existing
+partitions using the deprecated GUID can continue to use that GUID; support for the deprecated GUID
+will remain in SPDK indefinitely, and will continue to exhibit the off-by-one bug so that on-disk
+metadata layouts based on the incorrect size are not affected.
+
+See GitHub issue [2801](https://github.com/spdk/spdk/issues/2801) for additional details on the bug.
+
+New SPDK partition types should use GUID `6527994e-2c5a-4eec-9613-8f5944074e8b` which will create
+a bdev of the correct size.

doc/bdev.md

@@ -274,7 +274,7 @@ possibly multiple virtual bdevs.
 
 ### SPDK GPT partition table {#bdev_ug_gpt}
 
-The SPDK partition type GUID is `7c5222bd-8f5d-4087-9c00-bf9843c7b58c`. Existing SPDK bdevs
+The SPDK partition type GUID is `6527994e-2c5a-4eec-9613-8f5944074e8b`. Existing SPDK bdevs
 can be exposed as Linux block devices via NBD and then can be partitioned with
 standard partitioning tools. After partitioning, the bdevs will need to be deleted and
 attached again for the GPT bdev module to see any changes. NBD kernel module must be
@@ -312,7 +312,7 @@ parted -s /dev/nbd0 mkpart MyPartition '0%' '50%'
 
 # Change the partition type to the SPDK GUID.
 # sgdisk is part of the gdisk package.
-sgdisk -t 1:7c5222bd-8f5d-4087-9c00-bf9843c7b58c /dev/nbd0
+sgdisk -t 1:6527994e-2c5a-4eec-9613-8f5944074e8b /dev/nbd0
 
 # Stop the NBD device (stop exporting /dev/nbd0).
 rpc.py nbd_stop_disk /dev/nbd0

module/bdev/gpt/gpt.h

@@ -13,8 +13,24 @@
 #include "spdk/stdinc.h"
 
 #include "spdk/gpt_spec.h"
+#include "spdk/log.h"
+
+#define SPDK_GPT_PART_TYPE_GUID     SPDK_GPT_GUID(0x6527994e, 0x2c5a, 0x4eec, 0x9613, 0x8f5944074e8b)
+
+/* PART_TYPE_GUID_OLD partitions will be constructed as bdevs with one fewer block than expected.
+ * See GitHub issue #2801.
+ */
+#ifdef REGISTER_GUID_DEPRECATION
+/* Register the deprecation in the header file, to make it clear to readers that this GUID
+ * shouldn't be used for new SPDK GPT partitions.  We will never actually log this deprecation
+ * though, since we are not recommending that users try to migrate existing partitions with the
+ * old GUID to the new GUID. Wrap it in this REGISTER_GUID_DEPRECATION flag to avoid defining
+ * this deprecation in multiple compilation units.
+ */
+SPDK_LOG_DEPRECATION_REGISTER(old_gpt_guid, "old gpt guid", "Never", 0)
+#endif
+#define SPDK_GPT_PART_TYPE_GUID_OLD SPDK_GPT_GUID(0x7c5222bd, 0x8f5d, 0x4087, 0x9c00, 0xbf9843c7b58c)
 
-#define SPDK_GPT_PART_TYPE_GUID SPDK_GPT_GUID(0x7c5222bd, 0x8f5d, 0x4087, 0x9c00, 0xbf9843c7b58c)
 #define SPDK_GPT_BUFFER_SIZE 32768  /* 32KB */
 #define	SPDK_GPT_GUID_EQUAL(x,y) (memcmp(x, y, sizeof(struct spdk_gpt_guid)) == 0)
 

module/bdev/gpt/vbdev_gpt.c

@@ -8,6 +8,7 @@
  * each partition.
  */
 
+#define REGISTER_GUID_DEPRECATION
 #include "gpt.h"
 
 #include "spdk/endian.h"
@@ -313,12 +314,26 @@ vbdev_gpt_create_bdevs(struct gpt_base *gpt_base)
 		p = &gpt->partitions[i];
 		uint64_t lba_start = from_le64(&p->starting_lba);
 		uint64_t lba_end = from_le64(&p->ending_lba);
+		uint64_t partition_size = lba_end - lba_start + 1;
 
-		if (!SPDK_GPT_GUID_EQUAL(&gpt->partitions[i].part_type_guid,
+		if (lba_start == 0) {
-					 &SPDK_GPT_PART_TYPE_GUID) ||
-		    lba_start == 0) {
 			continue;
 		}
+
+		if (SPDK_GPT_GUID_EQUAL(&gpt->partitions[i].part_type_guid,
+					&SPDK_GPT_PART_TYPE_GUID_OLD)) {
+			/* GitHub issue #2801 - we continue to report these partitions with
+			 * off-by-one sizing error to ensure we don't break layouts based
+			 * on that smaller size. */
+			partition_size -= 1;
+		} else if (!SPDK_GPT_GUID_EQUAL(&gpt->partitions[i].part_type_guid,
+						&SPDK_GPT_PART_TYPE_GUID)) {
+			/* Partition type isn't TYPE_GUID or TYPE_GUID_OLD, so this isn't
+			 * an SPDK parition.  Continue to the next partition.
+			 */
+			continue;
+		}
+
 		if (lba_start < head_lba_start || lba_end > head_lba_end) {
 			continue;
 		}
@@ -339,7 +354,7 @@ vbdev_gpt_create_bdevs(struct gpt_base *gpt_base)
 		}
 
 		rc = spdk_bdev_part_construct(&d->part, gpt_base->part_base, name,
-					      lba_start, lba_end - lba_start, "GPT Disk");
+					      lba_start, partition_size, "GPT Disk");
 		free(name);
 		if (rc) {
 			SPDK_ERRLOG("could not construct bdev part\n");

scripts/common.sh

@@ -361,12 +361,25 @@ block_in_use() {
 	return 0
 }
 
+get_spdk_gpt_old() {
+	local spdk_guid
+
+	[[ -e $rootdir/module/bdev/gpt/gpt.h ]] || return 1
+
+	GPT_H="$rootdir/module/bdev/gpt/gpt.h"
+	IFS="()" read -r _ spdk_guid _ < <(grep -w SPDK_GPT_PART_TYPE_GUID_OLD "$GPT_H")
+	spdk_guid=${spdk_guid//, /-} spdk_guid=${spdk_guid//0x/}
+
+	echo "$spdk_guid"
+}
+
 get_spdk_gpt() {
 	local spdk_guid
 
 	[[ -e $rootdir/module/bdev/gpt/gpt.h ]] || return 1
 
-	IFS="()" read -r _ spdk_guid _ < <(grep SPDK_GPT_PART_TYPE_GUID "$rootdir/module/bdev/gpt/gpt.h")
+	GPT_H="$rootdir/module/bdev/gpt/gpt.h"
+	IFS="()" read -r _ spdk_guid _ < <(grep -w SPDK_GPT_PART_TYPE_GUID "$GPT_H")
 	spdk_guid=${spdk_guid//, /-} spdk_guid=${spdk_guid//0x/}
 
 	echo "$spdk_guid"

scripts/spdk-gpt.py

@@ -40,6 +40,7 @@ def is_spdk_gpt(block, entry):
     disk_lbsize = lbsize(block)
     gpt_sig = 0x5452415020494645  # EFI PART
     spdk_guid = [0x7c5222bd, 0x8f5d, 0x4087, 0x9c00, 0xbf9843c7b58c]
+    spdk_guid2 = [0x6527994e, 0x2c5a, 0x4eec, 0x9613, 0x8f5944074e8b]
 
     if readb(block_path, disk_lbsize, 8) != gpt_sig:
         print("No valid GPT data, bailing")
@@ -56,7 +57,7 @@ def is_spdk_gpt(block, entry):
         readb(block_path, part_entry_lba + 10, 8, ">Q") >> 16
     ]
 
-    return guid == spdk_guid
+    return guid == spdk_guid or guid == spdk_guid2
 
 
 if __name__ == "__main__":

test/bdev/blockdev.sh

@@ -121,9 +121,10 @@ function setup_gpt_conf() {
 		# Create gpt partition table
 		parted -s "$gpt_nvme" mklabel gpt mkpart SPDK_TEST_first '0%' '50%' mkpart SPDK_TEST_second '50%' '100%'
 		# change the GUID to SPDK GUID value
+		SPDK_GPT_OLD_GUID=$(get_spdk_gpt_old)
 		SPDK_GPT_GUID=$(get_spdk_gpt)
 		sgdisk -t "1:$SPDK_GPT_GUID" "$gpt_nvme"
-		sgdisk -t "2:$SPDK_GPT_GUID" "$gpt_nvme"
+		sgdisk -t "2:$SPDK_GPT_OLD_GUID" "$gpt_nvme"
 		"$rootdir/scripts/setup.sh"
 		"$rpc_py" bdev_get_bdevs
 		setup_nvme_conf