Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated

From: Wei Yang <richardw.yang@linux.intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <zwisler@kernel.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated
Date: Mon, 21 Jan 2019 08:24:26 +0800	[thread overview]
Message-ID: <20190121002426.GB3680@richard> (raw)
In-Reply-To: <CAPcyv4h+yJjBMfetF3ecxSHySgWjSWBH+Li9JWBZZSruqFBnLg@mail.gmail.com>

On Fri, Jan 18, 2019 at 12:38:18AM -0800, Dan Williams wrote:
>On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>>
>> On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote:
>> >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >>
>> >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote:
>> >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >>
>> >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb.
>> >> >>
>> >> >> For example:
>> >> >>
>> >> >>     nd_dax_probe()
>> >> >>         nd_pfn->pfn_sb = devm_kzalloc()
>> >> >>
>> >> >>     dax_pmem_probe()
>> >> >>         nvdimm_setup_pfn()
>> >> >>             nd_pfn_init()
>> >> >>                 nd_pfn->pfn_sb = devm_kzalloc()
>> >> >>
>> >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init().
>> >> >
>> >> >Ugh, this patch is unfortunately uncovering a deeper problem.
>> >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This
>> >> >effectively is always re-initializing the info-block, benign but
>> >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed
>> >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path.
>> >> >
>> >> >Good find!
>> >>
>> >> So we should fix this like:
>> >>
>> >> dax_pmem_probe()
>> >>     nd_pfn_validate()
>> >>     __nvdimm_setup_pfn()
>> >>
>> >> Is my understanding correct?
>> >
>> >Yes, at first glance, but it would need a deeper review and testing.
>> >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a
>> >similar fixup.
>>
>> I went through the code again and found I may remove the allocation in
>> nd_pfn_init().
>>
>> Below is my analysis.
>>
>> nvdimm_setup_pfn() is invoked in two places:
>>
>>   * dax_pmem_probe()
>>   * pmem_attach_disk(), when it is_nd_pfn()
>>
>> And before these two function called, corresponding device should be allocated
>> and created. And we can see these two corresponding places to create the
>> devices are:
>>
>>   * nd_dax_probe()
>>   * nd_pfn_probe()
>>
>> Both of them allocate pfn_sb. This means it is not necessary to allocate it
>> again when we do the probe function.
>>
>> Do you think it looks good to you?
>>
>
>No it doesn't because the real issue is that we should not be
>recreating the info block. So the fix is not avoiding the allocation,
>the fix is avoiding nd_pfn_init() in the probe path altogether.

Hmm... I may lost here. Which info block we recreated?

For one particular device type, nvdimm_setup_pfn() is just called once. By
preventing re-allocation here is enough to me. I may not get your point here.

Would you mind share some light on this place?

-- 
Wei Yang
Help you, Help me
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm