linux-kernel - Re: [PATCH 14/26] cxl/region: Read existing extents on region creation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <66162f721d5a_e9f9f2943e@iweiny-mobl.notmuch>
Date: Tue, 9 Apr 2024 23:19:30 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: fan <nifan.cxl@...il.com>, <ira.weiny@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>, Navneet Singh
	<navneet.singh@...el.com>, Dan Williams <dan.j.williams@...el.com>, Davidlohr
 Bueso <dave@...olabs.net>, Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>, <linux-btrfs@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 14/26] cxl/region: Read existing extents on region
 creation

fan wrote:
> On Sun, Mar 24, 2024 at 04:18:17PM -0700, ira.weiny@...el.com wrote:
> > From: Navneet Singh <navneet.singh@...el.com>
> > 

[snip]

> > +
> > +/**
> > + * cxl_read_dc_extents() - Read any existing extents
> > + * @cxled: Endpoint decoder which is part of a region
> > + *
> > + * Issue the Get Dynamic Capacity Extent List command to the device
> > + * and add any existing extents found which belong to this decoder.
> > + *
> > + * Return: 0 if command was executed successfully, -ERRNO on error.
> > + */
> > +int cxl_read_dc_extents(struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_memdev_state *mds = cxled_to_mds(cxled);
> > +	struct device *dev = mds->cxlds.dev;
> > +	unsigned int extent_gen_num;
> > +	int rc;
> > +
> > +	if (!cxl_dcd_supported(mds)) {
> > +		dev_dbg(dev, "DCD unsupported\n");
> > +		return 0;
> > +	}
> > +
> > +	rc = cxl_dev_get_dc_extent_cnt(mds, &extent_gen_num);
> > +	dev_dbg(mds->cxlds.dev, "Extent count: %d Generation Num: %d\n",
> > +		rc, extent_gen_num);
> > +	if (rc <= 0) /* 0 == no records found */
> > +		return rc;
> > +
> > +	return cxl_dev_get_dc_extents(cxled, extent_gen_num, rc);
> 
> Not sure about the behaviour here. From the cxl_dev_get_dc_extents
> implementation below, if gen_num changed or the expected extent count
> changed, it will return error. 

yep.

> If I understand it correctly, if the above two values change, it means
> the extent list has been updated due to extent add/release since last
> time we read the extent list info (cxl_dev_get_dc_extent_cnt), do we
> need to fail the operation or try again?

The original series was safe to fail the operation because the list was read on
memory device driver load and not when the regions were created.  This is an
oversight with the new architecture.  Now that regions query for the list
independent of other regions being active the list could indeed change during
this operation.  :-/  So a retry is necessary.

Let me work on the retry because some of the extents may have been surfaced
during the list processing which means a re-read of the list will need to
properly ignore those already found.  Or some other tracking needs to be put in
place.

Ira