Re: [PATCH v13 3/7] pagemap,pmem: Introduce ->memory_failure()

Shiyang Ruan
2022-04-22T07:06:21Z

在 2022/4/21 16:24, Miaohe Lin 写道:
> On 2022/4/19 12:50, Shiyang Ruan wrote:
>> When memory-failure occurs, we call this function which is implemented
>> by each kind of devices. For the fsdax case, pmem device driver
>> implements it. Pmem device driver will find out the filesystem in which
>> the corrupted page located in.
>>
>> With dax_holder notify support, we are able to notify the memory failure
>> from pmem driver to upper layers. If there is something not support in
>> the notify routine, memory_failure will fall back to the generic hanlder.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>> drivers/nvdimm/pmem.c | 17 +++++++++++++++++
>> include/linux/memremap.h | 12 ++++++++++++
>> mm/memory-failure.c | 14 ++++++++++++++
>> 3 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
>> index 58d95242a836..bd502957cfdf 100644
>> --- a/drivers/nvdimm/pmem.c
>> +++ b/drivers/nvdimm/pmem.c
>> @@ -366,6 +366,21 @@ static void pmem_release_disk(void *__pmem)
>> blk_cleanup_disk(pmem->disk);
>> }
>>
>> +static int pmem_pagemap_memory_failure(struct dev_pagemap *pgmap,
>> + unsigned long pfn, unsigned long nr_pages, int mf_flags)
>> +{
>> + struct pmem_device *pmem =
>> + container_of(pgmap, struct pmem_device, pgmap);
>> + u64 offset = PFN_PHYS(pfn) - pmem->phys_addr - pmem->data_offset;
>> + u64 len = nr_pages << PAGE_SHIFT;
>> +
>> + return dax_holder_notify_failure(pmem->dax_dev, offset, len, mf_flags);
>> +}
>> +
>> +static const struct dev_pagemap_ops fsdax_pagemap_ops = {
>> + .memory_failure = pmem_pagemap_memory_failure,
>> +};
>> +
>> static int pmem_attach_disk(struct device *dev,
>> struct nd_namespace_common *ndns)
>> {
>> @@ -427,6 +442,7 @@ static int pmem_attach_disk(struct device *dev,
>> pmem->pfn_flags = PFN_DEV;
>> if (is_nd_pfn(dev)) {
>> pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
>> + pmem->pgmap.ops = &fsdax_pagemap_ops;
>> addr = devm_memremap_pages(dev, &pmem->pgmap);
>> pfn_sb = nd_pfn->pfn_sb;
>> pmem->data_offset = le64_to_cpu(pfn_sb->dataoff);
>> @@ -440,6 +456,7 @@ static int pmem_attach_disk(struct device *dev,
>> pmem->pgmap.range.end = res->end;
>> pmem->pgmap.nr_range = 1;
>> pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
>> + pmem->pgmap.ops = &fsdax_pagemap_ops;
>> addr = devm_memremap_pages(dev, &pmem->pgmap);
>> pmem->pfn_flags |= PFN_MAP;
>> bb_range = pmem->pgmap.range;
>> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
>> index ad6062d736cd..bcfb6bf4ce5a 100644
>> --- a/include/linux/memremap.h
>> +++ b/include/linux/memremap.h
>> @@ -79,6 +79,18 @@ struct dev_pagemap_ops {
>> * the page back to a CPU accessible page.
>> */
>> vm_fault_t (*migrate_to_ram)(struct vm_fault *vmf);
>> +
>> + /*
>> + * Handle the memory failure happens on a range of pfns. Notify the
>> + * processes who are using these pfns, and try to recover the data on
>> + * them if necessary. The mf_flags is finally passed to the recover
>> + * function through the whole notify routine.
>> + *
>> + * When this is not implemented, or it returns -EOPNOTSUPP, the caller
>> + * will fall back to a common handler called mf_generic_kill_procs().
>> + */
>> + int (*memory_failure)(struct dev_pagemap *pgmap, unsigned long pfn,
>> + unsigned long nr_pages, int mf_flags);
>> };
>>
>> #define PGMAP_ALTMAP_VALID (1 << 0)
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 7c8c047bfdc8..a40e79e634a4 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1741,6 +1741,20 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>> if (!pgmap_pfn_valid(pgmap, pfn))
>> goto out;
>>
>> + /*
>> + * Call driver's implementation to handle the memory failure, otherwise
>> + * fall back to generic handler.
>> + */
>> + if (pgmap->ops->memory_failure) {
>> + rc = pgmap->ops->memory_failure(pgmap, pfn, 1, flags);
>> + /*
>> + * Fall back to generic handler too if operation is not
>> + * supported inside the driver/device/filesystem.
>> + */
>> + if (rc != -EOPNOTSUPP)
>> + goto out;
>> + }
>> +
>
> Thanks for your patch. There are two questions:
>
> 1.Is dax_lock_page + dax_unlock_page pair needed here?

They are moved into mf_generic_kill_procs() in Patch2. Callback will
implement its own dax lock/unlock method. For example, for
mf_dax_kill_procs() in Patch4, we implemented
dax_lock_mapping_entry()/dax_unlock_mapping_entry() for it.

> 2.hwpoison_filter and SetPageHWPoison will be handled by the callback or they're just ignored deliberately?

SetPageHWPoison() will be handled by callback or by mf_generic_kill_procs().

hwpoison_filter() is moved into mf_generic_kill_procs() too. The
callback will make sure the page is correct, so it is ignored.

--
Thanks,
Ruan.

>
> Thanks!
>
>> rc = mf_generic_kill_procs(pfn, flags, pgmap);
>> out:
>> /* drop pgmap ref acquired in caller */
>>
>