Re: [PATCH V3 06/20] nvdimm/region_label: Add region label update support

[Neeraj Kumar <s.neeraj@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 7036 bytes --]

On 17/09/25 04:36PM, Jonathan Cameron wrote:
>On Wed, 17 Sep 2025 19:11:02 +0530
>Neeraj Kumar <s.neeraj@samsung.com> wrote:
>
>> Modified __pmem_label_update() to update region labels into LSA
>>
>I'm struggling to follow the use of the union for the two label types
>in much of this code.  To me if feels like that should only be a thing
>at the init_labels() point on the basis I think it's only there that
>we need to handle both in the same storage.
>
>For the rest I'd just pay the small price of duplication that will
>occur if you just split he functions up.

I got your point Jonathan, Let me revisit it again.

>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>> ---
>>  drivers/nvdimm/label.c          | 269 ++++++++++++++++++++++++++------
>>  drivers/nvdimm/label.h          |  15 ++
>>  drivers/nvdimm/namespace_devs.c |  12 ++
>>  drivers/nvdimm/nd.h             |  38 ++++-
>>  include/linux/libnvdimm.h       |   8 +
>>  5 files changed, 289 insertions(+), 53 deletions(-)
>>

[snip]

>> -	nsl_set_position(ndd, nd_label, pos);
>> -	nsl_set_isetcookie(ndd, nd_label, cookie);
>> -	nsl_set_rawsize(ndd, nd_label, resource_size(res));
>> -	nsl_set_lbasize(ndd, nd_label, nspm->lbasize);
>> -	nsl_set_dpa(ndd, nd_label, res->start);
>> -	nsl_set_slot(ndd, nd_label, slot);
>> -	nsl_set_alignment(ndd, nd_label, 0);
>> -	nsl_set_type_guid(ndd, nd_label, &nd_set->type_guid);
>> -	nsl_set_region_uuid(ndd, nd_label, NULL);
>> -	nsl_set_claim_class(ndd, nd_label, ndns->claim_class);
>> -	nsl_calculate_checksum(ndd, nd_label);
>> -	nd_dbg_dpa(nd_region, ndd, res, "\n");
>> +	lsa_label = (union nd_lsa_label *) to_label(ndd, slot);
>
>This cast feels rather dubious.
>
>If the union makes sense in general, then this should be changed
>to return the union.

Sure, I will fix it in next patch-set

>
>> +	memset(lsa_label, 0, sizeof_namespace_label(ndd));
>> +
>> +	switch (ltype) {
>> +	case NS_LABEL_TYPE:
>> +		dev = &nspm->nsio.common.dev;
>> +		rc = namespace_label_update(nd_region, nd_mapping,
>> +				nspm, pos, flags, &lsa_label->ns_label,
>> +				nsindex, slot);
>> +		if (rc)
>> +			return rc;
>> +
>> +		break;
>> +	case RG_LABEL_TYPE:
>> +		dev = &nd_region->dev;
>> +		region_label_update(nd_region, &lsa_label->region_label,
>> +				    nd_mapping, pos, flags, slot);
>> +
>> +		break;
>> +	}
>>
>>  	/* update label */
>> -	offset = nd_label_offset(ndd, nd_label);
>> -	rc = nvdimm_set_config_data(ndd, offset, nd_label,
>> +	offset = nd_label_offset(ndd, &lsa_label->ns_label);
>
>This doesn't make sense as the type might be either an ns_label or a region_label.
>If there is a generic header (I'm guessing there is) then define that as part of the
>union with just the shared parts and use that to avoid any implication of what the type
>is in calls like this.  Also make nd_label_offset() take the union not the specific bit.

Okay, I will update the signature of nd_label_offset() to use union and not the specific bit

>
>> +	rc = nvdimm_set_config_data(ndd, offset, lsa_label,
>>  			sizeof_namespace_label(ndd));
>>  	if (rc < 0)
>>  		return rc;
>> @@ -955,8 +1054,10 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>>  		if (!label_ent->label)
>>  			continue;
>> -		if (test_and_clear_bit(ND_LABEL_REAP, &label_ent->flags) ||
>> -		    nsl_uuid_equal(ndd, label_ent->label, nspm->uuid))
>> +
>> +		if (is_label_reapable(nd_set, nspm, ndd,
>> +				      (union nd_lsa_label *) label_ent->label,
>
>If we are going with the union that label_ent->label should be a union that
>we don't need to cast.

Sure, I will fix this in next patch-set

>
>> +				      ltype, &label_ent->flags))
>>  			reap_victim(nd_mapping, label_ent);
>>  	}
>>
>> @@ -966,19 +1067,20 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	if (rc)
>>  		return rc;
>>
>> -	list_for_each_entry(label_ent, &nd_mapping->labels, list)
>> -		if (!label_ent->label) {
>> -			label_ent->label = nd_label;
>> -			nd_label = NULL;
>> -			break;
>> -		}
>> -	dev_WARN_ONCE(&nspm->nsio.common.dev, nd_label,
>> -			"failed to track label: %d\n",
>> -			to_slot(ndd, nd_label));
>> -	if (nd_label)
>> -		rc = -ENXIO;
>> +	list_for_each_entry(label_ent, &nd_mapping->labels, list) {
>> +		if (label_ent->label)
>> +			continue;
>
>This flow change is unrelated to the rest here. I'd push it back
>to the simpler patch that change the locking. Make sure to call it out there
>though.  Or just don't do it and keep this patch a little more readable!

Yes, I will fix this in previous patch.

>> @@ -1091,12 +1209,19 @@ int nd_pmem_namespace_label_update(struct nd_region *nd_region,
>>  				count++;
>>  		WARN_ON_ONCE(!count);
>>
>> -		rc = init_labels(nd_mapping, count);
>> +		region_label_cnt = find_region_label_count(ndd, nd_mapping);
>> +		/*
>> +		 * init_labels() scan labels and allocate new label based
>> +		 * on its second parameter (num_labels). Therefore to
>> +		 * allocate new namespace label also include previously
>> +		 * added region label
>> +		 */
>> +		rc = init_labels(nd_mapping, count + region_label_cnt);
>>  		if (rc < 0)
>>  			return rc;
>>
>>  		rc = __pmem_label_update(nd_region, nd_mapping, nspm, i,
>> -				NSLABEL_FLAG_UPDATING);
>> +				NSLABEL_FLAG_UPDATING, NS_LABEL_TYPE);
>
>The amount of shared code in __pmem_label_update() across the two types in
>the union isn't that high.  I'd be tempted to just split it for simplicity.

Sure, I will split it out in next patch-set.

>
>>  		if (rc)
>>  			return rc;
>>  	}
>>
[snip]
>> +int nd_region_label_update(struct nd_region *nd_region)
>> +{
>> +	int rc;
>> +
>> +	nvdimm_bus_lock(&nd_region->dev);
>> +	rc = nd_pmem_region_label_update(nd_region);
>> +	nvdimm_bus_unlock(&nd_region->dev);
>Looks like it would be worth introducing a
>DEFINE_GUARD() for this lock.
>
>Not necessarily in this series however.

Yes, Fixing it here will add some extra change.

>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(nd_region_label_update);
>> +
>>  static int nd_namespace_label_update(struct nd_region *nd_region,
>>  		struct device *dev)
>>  {
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index e362611d82cc..f04c042dcfa9 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>
>>  bool nsl_validate_type_guid(struct nvdimm_drvdata *ndd,
>>  			    struct nd_namespace_label *nd_label, guid_t *guid);
>>  enum nvdimm_claim_class nsl_get_claim_class(struct nvdimm_drvdata *ndd,
>> @@ -399,7 +432,10 @@ enum nd_label_flags {
>>  struct nd_label_ent {
>>  	struct list_head list;
>>  	unsigned long flags;
>> -	struct nd_namespace_label *label;
>> +	union {
>> +		struct nd_namespace_label *label;
>> +		struct cxl_region_label *region_label;
>
>It is a bit odd to have a union above of the two types in
>here but then union the pointers here.

Yes Jonathan, I will replace this unnamed union with "union nd_lsa_label". I will
also help in avoid extra typecasting in is_region_label() and is_label_reapable()


Regards,
Neeraj

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]