Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test

From: Ira Weiny <ira.weiny@intel.com>
To: "Verma, Vishal L" <vishal.l.verma@intel.com>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	"Weiny, Ira" <ira.weiny@intel.com>
Cc: "Williams, Dan J" <dan.j.williams@intel.com>,
	"Jiang, Dave" <dave.jiang@intel.com>,
	"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
	"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>
Subject: Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
Date: Tue, 5 Dec 2023 13:22:23 -0800	[thread overview]
Message-ID: <656f948febeb0_182977294ec@iweiny-mobl.notmuch> (raw)
In-Reply-To: <000bba54c1a3cde1aa63bc8052c01e745835468d.camel@intel.com>

Verma, Vishal L wrote:

[snip]

> > > > +# Find a memory device to create regions on to test the destroy
> > > > +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> > > > +for mem in ${mems[@]}; do
> > > > +        ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> > > > +        if [ "$ramsize" == "null" ]; then
> > > > +                continue
> > > > +        fi
> > > > +        decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> > > > +                  jq -r ".[] |
> > > > +                  select(.volatile_capable == true) |
> > > > +                  select(.nr_targets == 1) |
> > > > +                  select(.size >= ${ramsize}) |
> > > > +                  .decoder")
> > > > +        if [[ $decoder ]]; then
> > > > +               check_destroy_ram $mem $decoder
> > > > +               check_destroy_devdax $mem $decoder
> > > > +                break
> > > > +        fi
> > > > +done
> > > 
> > > Does this need to check results of the region disable & destroy?
> > > 
> > > Did the regression this is after leave a trace in the dmesg log,
> > > so checking that is all that's needed?
> > > 
> > 
> > The regression causes
> > 
> >         check_destroy_devdax()
> >                 $CXL disable-region $region
> > 
> > to fail.  That command failure will exit with an error which causes the
> > test script to exit with that error as well.
> > 
> > At least that is what happened when I used this to test the fix.  I'll
> > defer to Vishal if there is a more explicit or better way to check for
> > that cxl-cli command to fail.
> > 
> Correct, the set -e will cause the script to abort with an error exit
> code whenever a command fails.
> 
> I do wonder if we need this new test - with Dave's patch here[1],

I'm not sure.

> destroy-region and disable-region both use the same helper that
> performs the libdaxctl checks.
> 
> cxl-create-region.sh already has flows that create a region and then
> destroy it. Those should now cover this case as well yeah?

I thought it would have but I don't think it covers the case where the dax
device is not system ram (the default when creating a region).

Ira