RE: [PATCH v3 05/11] mm, fsdax: Refactor memory-failure handler for dax mapping

[ruansy.fnst@xxxxxxxxxxx]

> -----Original Message-----
> From: zhong jiang <zhongjiang-ali@xxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v3 05/11] mm, fsdax: Refactor memory-failure handler for
> dax mapping
> 
> > +int mf_dax_mapping_kill_procs(struct address_space *mapping, pgoff_t
> > +index, int flags) {
> > +	const bool unmap_success = true;
> > +	unsigned long pfn, size = 0;
> > +	struct to_kill *tk;
> > +	LIST_HEAD(to_kill);
> > +	int rc = -EBUSY;
> > +	loff_t start;
> > +
> > +	/* load the pfn of the dax mapping file */
> > +	pfn = dax_load_pfn(mapping, index);
> > +	if (!pfn)
> > +		return rc;
> > +	/*
> > +	 * Unlike System-RAM there is no possibility to swap in a
> > +	 * different physical page at a given virtual address, so all
> > +	 * userspace consumption of ZONE_DEVICE memory necessitates
> > +	 * SIGBUS (i.e. MF_MUST_KILL)
> > +	 */
> > +	flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
> 
> MF_ACTION_REQUIRED only kill the current execution context. A page can be
> shared when reflink file be mapped by different process. We can not kill all
> process shared the page.  Other process still can access the posioned page ?

AFAIK, the other processes will receive a SIGBUS when accessing this corrupted range.  But I didn't add a testcase for this condition.  I'll test it.  Thanks for pointing out.


--
Thanks,
Ruan Shiyang.

> 
> Thanks,
> zhong jiang
> 
> > +	collect_procs_file(pfn_to_page(pfn), mapping, index, &to_kill,
> > +			   flags & MF_ACTION_REQUIRED);
> > +
> > +	list_for_each_entry(tk, &to_kill, nd)
> > +		if (tk->size_shift)
> > +			size = max(size, 1UL << tk->size_shift);
> > +	if (size) {
> > +		/*
> > +		 * Unmap the largest mapping to avoid breaking up
> > +		 * device-dax mappings which are constant size. The
> > +		 * actual size of the mapping being torn down is
> > +		 * communicated in siginfo, see kill_proc()
> > +		 */
> > +		start = (index << PAGE_SHIFT) & ~(size - 1);
> > +		unmap_mapping_range(mapping, start, start + size, 0);
> > +	}
> > +
> > +	kill_procs(&to_kill, flags & MF_MUST_KILL, !unmap_success,
> > +		   pfn, flags);
> > +	rc = 0;
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(mf_dax_mapping_kill_procs);
> > +
> >   static int memory_failure_hugetlb(unsigned long pfn, int flags)
> >   {
> >   	struct page *p = pfn_to_page(pfn);
> > @@ -1297,7 +1346,7 @@ static int memory_failure_dev_pagemap(unsigned
> long pfn, int flags,
> >   	const bool unmap_success = true;
> >   	unsigned long size = 0;
> >   	struct to_kill *tk;
> > -	LIST_HEAD(tokill);
> > +	LIST_HEAD(to_kill);
> >   	int rc = -EBUSY;
> >   	loff_t start;
> >   	dax_entry_t cookie;
> > @@ -1345,9 +1394,10 @@ static int
> memory_failure_dev_pagemap(unsigned long pfn, int flags,
> >   	 * SIGBUS (i.e. MF_MUST_KILL)
> >   	 */
> >   	flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
> > -	collect_procs(page, &tokill, flags & MF_ACTION_REQUIRED);
> > +	collect_procs_file(page, page->mapping, page->index, &to_kill,
> > +			   flags & MF_ACTION_REQUIRED);
> >
> > -	list_for_each_entry(tk, &tokill, nd)
> > +	list_for_each_entry(tk, &to_kill, nd)
> >   		if (tk->size_shift)
> >   			size = max(size, 1UL << tk->size_shift);
> >   	if (size) {
> > @@ -1360,7 +1410,7 @@ static int memory_failure_dev_pagemap(unsigned
> long pfn, int flags,
> >   		start = (page->index << PAGE_SHIFT) & ~(size - 1);
> >   		unmap_mapping_range(page->mapping, start, start + size, 0);
> >   	}
> > -	kill_procs(&tokill, flags & MF_MUST_KILL, !unmap_success, pfn, flags);
> > +	kill_procs(&to_kill, flags & MF_MUST_KILL, !unmap_success, pfn,
> > +flags);
> >   	rc = 0;
> >   unlock:
> >   	dax_unlock_page(page, cookie);
>