Re: [ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2022-08-11 at 11:42 -0700, Dan Williams wrote:
> Verma, Vishal L wrote:
> > On Wed, 2022-08-10 at 20:05 -0700, Dan Williams wrote:
> > 
> > 
> > [snip]
> > 
> > > 
> > > > +CXL_EXPORT struct cxl_region *
> > > > +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> > > > +{
> > > > +       struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> > > > +       char *path = decoder->dev_buf;
> > > > +       char buf[SYSFS_ATTR_SIZE];
> > > > +       struct cxl_region *region;
> > > > +       int rc;
> > > > +
> > > > +       sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> > > > +       rc = sysfs_read_attr(ctx, path, buf);
> > > > +       if (rc < 0) {
> > > > +               err(ctx, "failed to read new region name: %s\n",
> > > > +                   strerror(-rc));
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       rc = sysfs_write_attr(ctx, path, buf);
> > > > +       if (rc < 0) {
> > > > +               err(ctx, "failed to write new region name: %s\n",
> > > > +                   strerror(-rc));
> > > > +               return NULL;
> > > > +       }
> > > 
> > > I think there either needs to be a "decoder->regions_init = 0" here, or
> > > a direct call to "add_cxl_region(decoder...)" just in case this context
> > > had already listed regions before creating a new one.
> > > 
> > > I like the precision of "add_cxl_region()", but that needs to open code
> > > some of the internals of sysfs_device_parse(), so maybe
> > > "decoder->regions_init = 0" is ok for now.
> > 
> > Yes, I found that out - and added this - in patch 10 (I can instead
> > move it here - it makes sense).
> > 
> > Until patch 10, during region creation, nothing had done regions_init
> > until this point, so this happens to work. Patch 10 where we do walk
> > the regions before this point to calculate the max available space,
> > necessitates the reset here.
> > 
> > That being said, potentially all of patch 10 is squash-able into
> > different bits of the series - I left it at the end so the
> > max_available_extent stuff can be reviewed on its own.
> > 
> > I'm happy to go either way on squashing it or keeping it standalone.
> 
> If this was the kernel I would say squash, since bisecting might be
> impacted, but as long as the fix is there later in the series I think
> that's ok.
> 
> Note, I was less worried about the cxl-cli tool tripping over this, and
> more about 3rd party applications using libcxl, but those are even less
> likely to use a mid-release commit.


I think moving just the invalidation here to preserve bisectability for
non cxl-cli users makes sense.