diff mbox

[3/3] libnvdimm, btt: consolidate arena validation

Message ID 1437518265-14294-4-git-send-email-vishal.l.verma@intel.com (mailing list archive)
State Changes Requested
Delegated to: Vishal Verma
Headers show

Commit Message

Vishal Verma July 21, 2015, 10:37 p.m. UTC
Use arena_is_valid as a common routine for checking the validity of an
info block from both discover_arenas, and nd_btt_probe.

As a result, don't check for validity of the BTT's UUID, and lbasize.
The checksum in the BTT info block and parent_uuid are the necessary and
sufficient conditions to make sure a BTT is both, consistent with itself,
and not associated with a stale namespace.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/nvdimm/btt.c      | 43 +-----------------------------------
 drivers/nvdimm/btt.h      |  3 +++
 drivers/nvdimm/btt_devs.c | 55 +++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 50 insertions(+), 51 deletions(-)

Comments

Dan Williams July 25, 2015, 4:20 p.m. UTC | #1
On Tue, Jul 21, 2015 at 3:37 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> Use arena_is_valid as a common routine for checking the validity of an
> info block from both discover_arenas, and nd_btt_probe.
>
> As a result, don't check for validity of the BTT's UUID, and lbasize.
> The checksum in the BTT info block and parent_uuid are the necessary and
> sufficient conditions to make sure a BTT is both, consistent with itself,
> and not associated with a stale namespace.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/nvdimm/btt.c      | 43 +-----------------------------------
>  drivers/nvdimm/btt.h      |  3 +++
>  drivers/nvdimm/btt_devs.c | 55 +++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 50 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 81781db..1958829 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -582,47 +582,6 @@ static void free_arenas(struct btt *btt)
>         }
>  }
>
> -static int uuid_is_null(u8 *uuid)
> -{
> -       static const u8 null_uuid[16];
> -
> -       return !memcmp(uuid, null_uuid, 16);
> -}

Hmm, changing code you just added is usually a sign the patch order is
wrong.  I think it makes sense to do the refactoring with no expected
change in behavior, and then add the parent uuid checking.

> -
> -/*
> - * This function checks if the metadata layout is valid and error free
> - */
> -static int arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super)
> -{
> -       const u8 *parent_uuid = nd_dev_to_uuid(&nd_btt->ndns->dev);
> -       u64 checksum;
> -
> -       if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0)
> -               return 0;
> -
> -       if (memcmp(super->uuid, nd_btt->uuid, 16))
> -               return 0;
> -
> -       if (!uuid_is_null(super->parent_uuid))
> -               if (memcmp(super->parent_uuid, parent_uuid, 16))
> -                       return 0;
> -
> -       checksum = le64_to_cpu(super->checksum);
> -       super->checksum = 0;
> -       if (checksum != nd_btt_sb_checksum(super))
> -               return 0;
> -       super->checksum = cpu_to_le64(checksum);
> -
> -       if (nd_btt->lbasize != le32_to_cpu(super->external_lbasize))
> -               return 0;
> -
> -       /* TODO: figure out action for this */
> -       if ((le32_to_cpu(super->flags) & IB_FLAG_ERROR_MASK) != 0)
> -               dev_info(&nd_btt->dev, "Found arena with an error flag\n");
> -
> -       return 1;
> -}
> -
>  /*
>   * This function reads an existing valid btt superblock and
>   * populates the corresponding arena_info struct
> @@ -680,7 +639,7 @@ static int discover_arenas(struct btt *btt)
>                 if (ret)
>                         goto out;
>
> -               if (!arena_is_valid(btt->nd_btt, super)) {
> +               if (!nd_btt_arena_is_valid(btt->nd_btt, super)) {
>                         if (remaining == btt->rawsize) {
>                                 btt->init_state = INIT_NOTFOUND;
>                                 dev_info(to_dev(arena), "No existing arenas\n");
> diff --git a/drivers/nvdimm/btt.h b/drivers/nvdimm/btt.h
> index 75b0d80..f2ac1bd 100644
> --- a/drivers/nvdimm/btt.h
> +++ b/drivers/nvdimm/btt.h
> @@ -182,4 +182,7 @@ struct btt {
>         int init_state;
>         int num_arenas;
>  };
> +
> +int nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super);
> +
>  #endif
> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
> index 6ac8c0f..4ad4de6 100644
> --- a/drivers/nvdimm/btt_devs.c
> +++ b/drivers/nvdimm/btt_devs.c
> @@ -342,6 +342,51 @@ struct device *nd_btt_create(struct nd_region *nd_region)
>         return dev;
>  }
>
> +static int uuid_is_null(u8 *uuid)
> +{
> +       static const u8 null_uuid[16];
> +
> +       return !memcmp(uuid, null_uuid, 16);
> +}
> +
> +/**
> + * nd_btt_arena_is_valid - check if the metadata layout is valid
> + * @nd_btt:    device with BTT geometry and backing device info
> + * @super:     pointer to the arena's info block being tested
> + *
> + * Check consistency of the btt info block with itself by validating
> + * the checksum, and with the parent namespace by verifying the
> + * parent_uuid contained in the info block with the one supplied in.
> + *
> + * Returns:
> + * 0 for an invalid info block, 1 for a valid one

true / false for "is()" routines please.

Outside of that, and the patch ordering, looks good.
diff mbox

Patch

diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 81781db..1958829 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -582,47 +582,6 @@  static void free_arenas(struct btt *btt)
 	}
 }
 
-static int uuid_is_null(u8 *uuid)
-{
-	static const u8 null_uuid[16];
-
-	return !memcmp(uuid, null_uuid, 16);
-}
-
-/*
- * This function checks if the metadata layout is valid and error free
- */
-static int arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super)
-{
-	const u8 *parent_uuid = nd_dev_to_uuid(&nd_btt->ndns->dev);
-	u64 checksum;
-
-	if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0)
-		return 0;
-
-	if (memcmp(super->uuid, nd_btt->uuid, 16))
-		return 0;
-
-	if (!uuid_is_null(super->parent_uuid))
-		if (memcmp(super->parent_uuid, parent_uuid, 16))
-			return 0;
-
-	checksum = le64_to_cpu(super->checksum);
-	super->checksum = 0;
-	if (checksum != nd_btt_sb_checksum(super))
-		return 0;
-	super->checksum = cpu_to_le64(checksum);
-
-	if (nd_btt->lbasize != le32_to_cpu(super->external_lbasize))
-		return 0;
-
-	/* TODO: figure out action for this */
-	if ((le32_to_cpu(super->flags) & IB_FLAG_ERROR_MASK) != 0)
-		dev_info(&nd_btt->dev, "Found arena with an error flag\n");
-
-	return 1;
-}
-
 /*
  * This function reads an existing valid btt superblock and
  * populates the corresponding arena_info struct
@@ -680,7 +639,7 @@  static int discover_arenas(struct btt *btt)
 		if (ret)
 			goto out;
 
-		if (!arena_is_valid(btt->nd_btt, super)) {
+		if (!nd_btt_arena_is_valid(btt->nd_btt, super)) {
 			if (remaining == btt->rawsize) {
 				btt->init_state = INIT_NOTFOUND;
 				dev_info(to_dev(arena), "No existing arenas\n");
diff --git a/drivers/nvdimm/btt.h b/drivers/nvdimm/btt.h
index 75b0d80..f2ac1bd 100644
--- a/drivers/nvdimm/btt.h
+++ b/drivers/nvdimm/btt.h
@@ -182,4 +182,7 @@  struct btt {
 	int init_state;
 	int num_arenas;
 };
+
+int nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super);
+
 #endif
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 6ac8c0f..4ad4de6 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -342,6 +342,51 @@  struct device *nd_btt_create(struct nd_region *nd_region)
 	return dev;
 }
 
+static int uuid_is_null(u8 *uuid)
+{
+	static const u8 null_uuid[16];
+
+	return !memcmp(uuid, null_uuid, 16);
+}
+
+/**
+ * nd_btt_arena_is_valid - check if the metadata layout is valid
+ * @nd_btt:	device with BTT geometry and backing device info
+ * @super:	pointer to the arena's info block being tested
+ *
+ * Check consistency of the btt info block with itself by validating
+ * the checksum, and with the parent namespace by verifying the
+ * parent_uuid contained in the info block with the one supplied in.
+ *
+ * Returns:
+ * 0 for an invalid info block, 1 for a valid one
+ */
+int nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super)
+{
+	const u8 *parent_uuid = nd_dev_to_uuid(&nd_btt->ndns->dev);
+	u64 checksum;
+
+	if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0)
+		return 0;
+
+	if (!uuid_is_null(super->parent_uuid))
+		if (memcmp(super->parent_uuid, parent_uuid, 16))
+			return 0;
+
+	checksum = le64_to_cpu(super->checksum);
+	super->checksum = 0;
+	if (checksum != nd_btt_sb_checksum(super))
+		return 0;
+	super->checksum = cpu_to_le64(checksum);
+
+	/* TODO: figure out action for this */
+	if ((le32_to_cpu(super->flags) & IB_FLAG_ERROR_MASK) != 0)
+		dev_info(&nd_btt->dev, "Found arena with an error flag\n");
+
+	return 1;
+}
+EXPORT_SYMBOL(nd_btt_arena_is_valid);
+
 /*
  * nd_btt_sb_checksum: compute checksum for btt info block
  *
@@ -364,8 +409,6 @@  EXPORT_SYMBOL(nd_btt_sb_checksum);
 static int __nd_btt_probe(struct nd_btt *nd_btt,
 		struct nd_namespace_common *ndns, struct btt_sb *btt_sb)
 {
-	u64 checksum;
-
 	if (!btt_sb || !ndns || !nd_btt)
 		return -ENODEV;
 
@@ -375,14 +418,8 @@  static int __nd_btt_probe(struct nd_btt *nd_btt,
 	if (nvdimm_namespace_capacity(ndns) < SZ_16M)
 		return -ENXIO;
 
-	if (memcmp(btt_sb->signature, BTT_SIG, BTT_SIG_LEN) != 0)
-		return -ENODEV;
-
-	checksum = le64_to_cpu(btt_sb->checksum);
-	btt_sb->checksum = 0;
-	if (checksum != nd_btt_sb_checksum(btt_sb))
+	if (!nd_btt_arena_is_valid(nd_btt, btt_sb))
 		return -ENODEV;
-	btt_sb->checksum = cpu_to_le64(checksum);
 
 	nd_btt->lbasize = le32_to_cpu(btt_sb->external_lbasize);
 	nd_btt->uuid = kmemdup(btt_sb->uuid, 16, GFP_KERNEL);