Re: [PATCH 10/23] libnvdimm/labels: Add uuid helpers

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 9 Aug 2021 15:28:40 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for CXL labels that move the uuid to a different offset
> in the label, add nsl_{ref,get,validate}_uuid(). These helpers use the
> proper uuid_t type. That type definition predated the libnvdimm
> subsystem, so now is as a good a time as any to convert all the uuid
> handling in the subsystem to uuid_t to match the helpers.
> 
> As for the whitespace changes, all new code is clang-format compliant.
> 
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

There are a few interesting corners where you have cleaned out a pointless
copy before validating uuids. Perhaps call that out as a change in here
as it isn't as simple as just replacing like with like?
Perhaps I'm missing some reason that was needed in the code before this
patch.

All LGTM.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/nvdimm/btt.c            |   11 +++--
>  drivers/nvdimm/btt.h            |    4 +-
>  drivers/nvdimm/btt_devs.c       |   12 +++---
>  drivers/nvdimm/core.c           |   40 ++-----------------
>  drivers/nvdimm/label.c          |   34 +++++++---------
>  drivers/nvdimm/label.h          |    3 -
>  drivers/nvdimm/namespace_devs.c |   83 ++++++++++++++++++++-------------------
>  drivers/nvdimm/nd-core.h        |    5 +-
>  drivers/nvdimm/nd.h             |   37 ++++++++++++++++-
>  drivers/nvdimm/pfn_devs.c       |    2 -
>  include/linux/nd.h              |    4 +-
>  11 files changed, 115 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 92dec4952297..1cdfbadb7408 100644

> @@ -1050,7 +1050,6 @@ static int __blk_label_update(struct nd_region *nd_region,
>  	unsigned long *free, *victim_map = NULL;
>  	struct resource *res, **old_res_list;
>  	struct nd_label_id label_id;
> -	u8 uuid[NSLABEL_UUID_LEN];
>  	int min_dpa_idx = 0;
>  	LIST_HEAD(list);
>  	u32 nslot, slot;
> @@ -1088,8 +1087,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  		/* mark unused labels for garbage collection */
>  		for_each_clear_bit_le(slot, free, nslot) {
>  			nd_label = to_label(ndd, slot);
> -			memcpy(uuid, nd_label->uuid, NSLABEL_UUID_LEN);
> -			if (memcmp(uuid, nsblk->uuid, NSLABEL_UUID_LEN) != 0)
> +			if (!nsl_validate_uuid(ndd, nd_label, nsblk->uuid))
>  				continue;

The original code here was 'unusual'. I'm not sure why it couldn't always be
validated in place. 

>  			res = to_resource(ndd, nd_label);
>  			if (res && is_old_resource(res, old_res_list,
> @@ -1158,7 +1156,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  
>  		nd_label = to_label(ndd, slot);
>  		memset(nd_label, 0, sizeof_namespace_label(ndd));
> -		memcpy(nd_label->uuid, nsblk->uuid, NSLABEL_UUID_LEN);

> +		nsl_set_uuid(ndd, nd_label, nsblk->uuid);
>  		nsl_set_name(ndd, nd_label, nsblk->alt_name);
>  		nsl_set_flags(ndd, nd_label, NSLABEL_FLAG_LOCAL);
>  
> @@ -1206,8 +1204,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  		if (!nd_label)
>  			continue;
>  		nlabel++;
> -		memcpy(uuid, nd_label->uuid, NSLABEL_UUID_LEN);
> -		if (memcmp(uuid, nsblk->uuid, NSLABEL_UUID_LEN) != 0)
> +		if (!nsl_validate_uuid(ndd, nd_label, nsblk->uuid))
>  			continue;
>  		nlabel--;
>  		list_move(&label_ent->list, &list);
> @@ -1237,8 +1234,7 @@ static int __blk_label_update(struct nd_region *nd_region,
>  	}
>  	for_each_clear_bit_le(slot, free, nslot) {
>  		nd_label = to_label(ndd, slot);
> -		memcpy(uuid, nd_label->uuid, NSLABEL_UUID_LEN);
> -		if (memcmp(uuid, nsblk->uuid, NSLABEL_UUID_LEN) != 0)
> +		if (!nsl_validate_uuid(ndd, nd_label, nsblk->uuid))
>  			continue;
>  		res = to_resource(ndd, nd_label);
>  		res->flags &= ~DPA_RESOURCE_ADJUSTED;
> @@ -1318,12 +1314,11 @@ static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
>  	return max(num_labels, old_num_labels);
>  }