On 17/09/25 04:36PM, Jonathan Cameron wrote:
On Wed, 17 Sep 2025 19:11:02 +0530
Neeraj Kumar <[email protected]> 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 <[email protected]>
---
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
