Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 22 Jan 2019 08:29:16 +0800
From:   Wei Yang <richardw.yang@linux.intel.com>
To:     Wei Yang <richardw.yang@linux.intel.com>
Cc:     Dan Williams <dan.j.williams@intel.com>, linux-nvdimm@lists.01.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] libnvdimm: Clarify nd_pfn_init() flow
Message-ID: <20190122002915.GA5984@richard>
Reply-To: Wei Yang <richardw.yang@linux.intel.com>
References: <154785884329.2202034.3295476681016958230.stgit@dwillia2-desk3.amr.corp.intel.com>
 <20190121075102.GA3758@richard>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190121075102.GA3758@richard>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 21, 2019 at 03:51:02PM +0800, Wei Yang wrote:
>On Fri, Jan 18, 2019 at 04:47:23PM -0800, Dan Williams wrote:
>>In recent days, 2 engineers, including the original author of
>>nd_pfn_init(), overlooked the internal call to nd_pfn_validate() and the
>>implications to memory allocation.
>>
>>Clarify this situation to help anyone that reads through this code in
>>the future.
>>
>>Reported-by: Wei Yang <richardw.yang@linux.intel.com>
>>Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>---
>> drivers/nvdimm/btt_devs.c |    5 +++++
>> drivers/nvdimm/dax_devs.c |    5 +++++
>> drivers/nvdimm/pfn_devs.c |   21 +++++++++++++++++++++
>> 3 files changed, 31 insertions(+)
>>
>>diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
>>index 795ad4ff35ca..e0a6f2491e57 100644
>>--- a/drivers/nvdimm/btt_devs.c
>>+++ b/drivers/nvdimm/btt_devs.c
>>@@ -354,6 +354,11 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
>> 		put_device(btt_dev);
>> 	}
>> 
>>+	/*
>>+	 * Successful probe indicates to the caller that an nd_btt
>>+	 * personality device has been registered and the caller can
>>+	 * fail the probe of the baseline namespace device.
>>+	 */
>> 	return rc;
>> }
>> EXPORT_SYMBOL(nd_btt_probe);
>>diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
>>index 0453f49dc708..65010d955fa7 100644
>>--- a/drivers/nvdimm/dax_devs.c
>>+++ b/drivers/nvdimm/dax_devs.c
>>@@ -136,6 +136,11 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
>> 	} else
>> 		__nd_device_register(dax_dev);
>> 
>>+	/*
>>+	 * Successful probe indicates to the caller that a device-dax
>>+	 * personality device has been registered and the caller can
>>+	 * fail the probe of the baseline namespace device.
>>+	 */
>> 	return rc;
>> }
>> EXPORT_SYMBOL(nd_dax_probe);
>>diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
>>index 6f22272e8d80..a8783b5a76ba 100644
>>--- a/drivers/nvdimm/pfn_devs.c
>>+++ b/drivers/nvdimm/pfn_devs.c
>>@@ -576,6 +576,11 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
>> 	} else
>> 		__nd_device_register(pfn_dev);
>> 
>>+	/*
>>+	 * Successful probe indicates to the caller that an nd_pfn
>>+	 * personality device has been registered and the caller can
>>+	 * fail the probe of the baseline namespace device.
>>+	 */
>> 	return rc;
>> }
>> EXPORT_SYMBOL(nd_pfn_probe);
>>@@ -706,6 +711,22 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
>> 		sig = DAX_SIG;
>> 	else
>> 		sig = PFN_SIG;
>>+
>>+	/*
>>+	 * Check for an existing 'pfn' superblock before writing a new
>>+	 * one. The intended flow is that on the first probe of an
>>+	 * nd_{pfn,dax} device the superblock is calculated and written
>>+	 * to the namespace. In this case nd_pfn_validate() returns
>>+	 * -ENODEV because no valid superblock exists currently.
>>+	 *
>>+	 * On subsequent probes nd_pfn_validate() will find a valid
>>+	 * superblock and return 0.
>>+	 *
>>+	 * If an assumption of the superblock has been violated, like a
>>+	 * change to the physical alignment of the namespace,
>>+	 * nd_pfn_validate() will return an error other than -ENODEV to
>>+	 * fail probing.
>>+	 */
>
>Let me reply in this thread. Sorry for my poor understand, I don't get it
>clearly now.
>
>To be honest, the structure is a little bit complicated, if my understanding
>is not correct, please forgive my poor understand.
>
>Below is a code flow. To simply analysis, I setup kernel parameter memmap to
>emulate, and configure one namespace to mode devdax. So that we would have the
>same root for code flow.
>
>Let's start with nd_region_driver:
>
>    nd_region_probe
>        nd_region_register_namespaces
>	    create_namespaces
>	nd_region->btt_seed = nd_btt_create(nd_region);
>	nd_region->pfn_seed = nd_pfn_create(nd_region);
>	nd_region->dax_seed = nd_dax_create(nd_region);

May I ask a question about the purpose to create these three device here?

I see nd_pfn_create() doesn't allocate pfn_sb here, and the probe on these
devices failed. Confused why we need these three devices.

-- 
Wei Yang
Help you, Help me