Re: [ndctl PATCH v2 5/6] ndctl/cxl: Add extent output to region query

[Ira Weiny <ira.weiny@intel.com>]

Alison Schofield wrote:
> On Mon, Nov 04, 2024 at 08:10:49PM -0600, Ira Weiny wrote:
> > DCD regions have 0 or more extents.  The ability to list those and their
> > properties is useful to end users.
> > 
> 
> Should we describe those new useful properties in the man page...see
> below...
> 
> > Add extent output to region queries.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  Documentation/cxl/cxl-list.txt |   4 ++
> >  cxl/filter.h                   |   3 +
> >  cxl/json.c                     |  47 ++++++++++++++
> >  cxl/json.h                     |   3 +
> >  cxl/lib/libcxl.c               | 138 +++++++++++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym             |   5 ++
> >  cxl/lib/private.h              |  11 ++++
> >  cxl/libcxl.h                   |  11 ++++
> >  cxl/list.c                     |   3 +
> >  util/json.h                    |   1 +
> >  10 files changed, 226 insertions(+)
> > 
> > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> > index 9a9911e7dd9bba561c6202784017db1bb4b9f4bd..71fd313cfec2509c79f8ad1e0f64857d0d804c13 100644
> > --- a/Documentation/cxl/cxl-list.txt
> > +++ b/Documentation/cxl/cxl-list.txt
> > @@ -411,6 +411,10 @@ OPTIONS
> >  }
> >  ----
> >  
> > +-N::
> > +--extents::
> > +	Extend Dynamic Capacity region listings extent information.
> > +
> 
> a sample perhaps?  or some verbage on  what to expect.

Good idea I've added an example.

> 
> 
> snip
> 
> >  
> > +static void cxl_extents_init(struct cxl_region *region)
> > +{
> > +	const char *devname = cxl_region_get_devname(region);
> > +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> > +	char *extent_path, *dax_region_path;
> > +	struct dirent *de;
> > +	DIR *dir = NULL;
> > +
> > +	if (region->extents_init)
> > +		return;
> > +	region->extents_init = 1;
> > +
> > +	dbg(ctx, "Checking extents: %s\n", region->dev_path);
> 
> Rather than emit the above which makes me assume success if
> no err message follows, how about emitting the success debug
> msg when all is done below.

Fair enough.


[snip]

> > +
> > +	while ((de = readdir(dir)) != NULL) {
> > +		struct cxl_region_extent *extent;

[snip]

> > +
> > +		list_node_init(&extent->list);
> > +		list_add(&region->extents, &extent->list);
> > +	}
> 
> 	Here - dbg the success message

Actually after moving the simple message here I feel it is not really
helping much.  I think it best to report via debug each extent as it is
added.  So I've changed the above to:

	while (...) {
	...
		dbg(ctx, "%s added extent%d.%d\n", devname, region_id, id);
	}

This gives more information on what might be going wrong if debug is
required.

Ira

[snip]