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

[Ira Weiny]

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.we...@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]