[PATCH v2 0/8] A few cleanup and fixup patches for memory failure

[PATCH v2 0/8] A few cleanup and fixup patches for memory failure

[Miaohe Lin]

Hi,
This series contains a few patches to simplify the code logic, remove
unneeded variable and remove obsolete comment. Also we fix race changing
page more robustly in memory_failure. More details can be found in the
respective changelogs. Thanks!

---
v1->v2:
  Collect Acked-by tag and fix review comment.
  Thanks Naoya.
---
Miaohe Lin (8):
  mm/memory-failure.c: minor clean up for memory_failure_dev_pagemap
  mm/memory-failure.c: catch unexpected -EFAULT from vma_address()
  mm/memory-failure.c: rework the signaling logic in kill_proc
  mm/memory-failure.c: fix race with changing page more robustly
  mm/memory-failure.c: remove PageSlab check in hwpoison_filter_dev
  mm/memory-failure.c: rework the try_to_unmap logic in
    hwpoison_user_mappings()
  mm/memory-failure.c: remove obsolete comment in __soft_offline_page
  mm/memory-failure.c: remove unnecessary PageTransTail check

 mm/memory-failure.c | 87 +++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 50 deletions(-)

-- 
2.23.0

[PATCH v2 1/8] mm/memory-failure.c: minor clean up for memory_failure_dev_pagemap

[Miaohe Lin]

The flags always has MF_ACTION_REQUIRED and MF_MUST_KILL set. So we do
not need to check these flags again.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 55edb0cc3848..b3ff7e99a421 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1644,7 +1644,7 @@ 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(page, &tokill, true);
 
 	list_for_each_entry(tk, &tokill, nd)
 		if (tk->size_shift)
@@ -1659,7 +1659,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, size, 0);
 	}
-	kill_procs(&tokill, flags & MF_MUST_KILL, false, pfn, flags);
+	kill_procs(&tokill, true, false, pfn, flags);
 	rc = 0;
 unlock:
 	dax_unlock_page(page, cookie);
-- 
2.23.0

[PATCH v2 2/8] mm/memory-failure.c: catch unexpected -EFAULT from vma_address()

[Miaohe Lin]

It's unexpected to walk the page table when vma_address() return -EFAULT.
But dev_pagemap_mapping_shift() is called only when vma associated to the
error page is found already in collect_procs_{file,anon}, so vma_address()
should not return -EFAULT except with some bug, as Naoya pointed out. We
can use VM_BUG_ON_VMA() to catch this bug here.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b3ff7e99a421..eaa241058401 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -315,6 +315,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page,
 	pmd_t *pmd;
 	pte_t *pte;
 
+	VM_BUG_ON_VMA(address == -EFAULT, vma);
 	pgd = pgd_offset(vma->vm_mm, address);
 	if (!pgd_present(*pgd))
 		return 0;
-- 
2.23.0

[PATCH v2 3/8] mm/memory-failure.c: rework the signaling logic in kill_proc

[Miaohe Lin]

BUS_MCEERR_AR code is only sent when MF_ACTION_REQUIRED is set and the
target is current. Rework the code to make this clear.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index eaa241058401..7e205d91b2d7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -258,16 +258,13 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
 	pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
 			pfn, t->comm, t->pid);
 
-	if (flags & MF_ACTION_REQUIRED) {
-		if (t == current)
-			ret = force_sig_mceerr(BUS_MCEERR_AR,
-					 (void __user *)tk->addr, addr_lsb);
-		else
-			/* Signal other processes sharing the page if they have PF_MCE_EARLY set. */
-			ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr,
-				addr_lsb, t);
-	} else {
+	if ((flags & MF_ACTION_REQUIRED) && (t == current))
+		ret = force_sig_mceerr(BUS_MCEERR_AR,
+				 (void __user *)tk->addr, addr_lsb);
+	else
 		/*
+		 * Signal other processes sharing the page if they have
+		 * PF_MCE_EARLY set.
 		 * Don't use force here, it's convenient if the signal
 		 * can be temporarily blocked.
 		 * This could cause a loop when the user sets SIGBUS
@@ -275,7 +272,6 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
 		 */
 		ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr,
 				      addr_lsb, t);  /* synchronous? */
-	}
 	if (ret < 0)
 		pr_info("Memory failure: Error sending signal to %s:%d: %d\n",
 			t->comm, t->pid, ret);
-- 
2.23.0

[PATCH v2 4/8] mm/memory-failure.c: fix race with changing page more robustly

[Miaohe Lin]

We're only intended to deal with the non-Compound page after we split thp
in memory_failure. However, the page could have changed compound pages due
to race window. If this happens, we could try again to hopefully handle the
page next round. Also remove unneeded orig_head. It's always equal to the
hpage. So we can use hpage directly and remove this redundant one.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 7e205d91b2d7..d66f642888be 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1690,7 +1690,6 @@ int memory_failure(unsigned long pfn, int flags)
 {
 	struct page *p;
 	struct page *hpage;
-	struct page *orig_head;
 	struct dev_pagemap *pgmap;
 	int res = 0;
 	unsigned long page_flags;
@@ -1736,7 +1735,7 @@ int memory_failure(unsigned long pfn, int flags)
 		goto unlock_mutex;
 	}
 
-	orig_head = hpage = compound_head(p);
+	hpage = compound_head(p);
 	num_poisoned_pages_inc();
 
 	/*
@@ -1817,13 +1816,18 @@ int memory_failure(unsigned long pfn, int flags)
 	lock_page(p);
 
 	/*
-	 * The page could have changed compound pages during the locking.
-	 * If this happens just bail out.
+	 * We're only intended to deal with the non-Compound page here.
+	 * However, the page could have changed compound pages due to
+	 * race window. If this happens, we could try again to hopefully
+	 * handle the page next round.
 	 */
-	if (PageCompound(p) && compound_head(p) != orig_head) {
-		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
-		res = -EBUSY;
-		goto unlock_page;
+	if (PageCompound(p)) {
+		if (TestClearPageHWPoison(p))
+			num_poisoned_pages_dec();
+		unlock_page(p);
+		put_page(p);
+		flags &= ~MF_COUNT_INCREASED;
+		goto try_again;
 	}
 
 	/*
-- 
2.23.0

[PATCH v2 5/8] mm/memory-failure.c: remove PageSlab check in hwpoison_filter_dev

[Miaohe Lin]

Since commit 03e5ac2fc3bf ("mm: fix crash when using XFS on loopback"),
page_mapping() can handle the Slab pages. So remove this unnecessary
PageSlab check and obsolete comment.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d66f642888be..791d89e0c15a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -130,12 +130,6 @@ static int hwpoison_filter_dev(struct page *p)
 	    hwpoison_filter_dev_minor == ~0U)
 		return 0;
 
-	/*
-	 * page_mapping() does not accept slab pages.
-	 */
-	if (PageSlab(p))
-		return -EINVAL;
-
 	mapping = page_mapping(p);
 	if (mapping == NULL || mapping->host == NULL)
 		return -EINVAL;
-- 
2.23.0

[PATCH v2 6/8] mm/memory-failure.c: rework the try_to_unmap logic in hwpoison_user_mappings()

[Miaohe Lin]

Only for hugetlb pages in shared mappings, try_to_unmap should take
semaphore in write mode here. Rework the code to make it clear.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 791d89e0c15a..0100d4f9da9a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1404,26 +1404,22 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	if (kill)
 		collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
 
-	if (!PageHuge(hpage)) {
-		try_to_unmap(hpage, ttu);
+	if (PageHuge(hpage) && !PageAnon(hpage)) {
+		/*
+		 * For hugetlb pages in shared mappings, try_to_unmap
+		 * could potentially call huge_pmd_unshare.  Because of
+		 * this, take semaphore in write mode here and set
+		 * TTU_RMAP_LOCKED to indicate we have taken the lock
+		 * at this higher level.
+		 */
+		mapping = hugetlb_page_mapping_lock_write(hpage);
+		if (mapping) {
+			try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
+			i_mmap_unlock_write(mapping);
+		} else
+			pr_info("Memory failure: %#lx: could not lock mapping for mapped huge page\n", pfn);
 	} else {
-		if (!PageAnon(hpage)) {
-			/*
-			 * For hugetlb pages in shared mappings, try_to_unmap
-			 * could potentially call huge_pmd_unshare.  Because of
-			 * this, take semaphore in write mode here and set
-			 * TTU_RMAP_LOCKED to indicate we have taken the lock
-			 * at this higher level.
-			 */
-			mapping = hugetlb_page_mapping_lock_write(hpage);
-			if (mapping) {
-				try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
-				i_mmap_unlock_write(mapping);
-			} else
-				pr_info("Memory failure: %#lx: could not lock mapping for mapped huge page\n", pfn);
-		} else {
-			try_to_unmap(hpage, ttu);
-		}
+		try_to_unmap(hpage, ttu);
 	}
 
 	unmap_success = !page_mapped(hpage);
-- 
2.23.0

[PATCH v2 7/8] mm/memory-failure.c: remove obsolete comment in __soft_offline_page

[Miaohe Lin]

Since commit add05cecef80 ("mm: soft-offline: don't free target page in
successful page migration"), set_migratetype_isolate logic is removed.
Remove this obsolete comment.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 0100d4f9da9a..5d2e653a145d 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2165,10 +2165,6 @@ static int __soft_offline_page(struct page *page)
 		ret = invalidate_inode_page(page);
 	unlock_page(page);
 
-	/*
-	 * RED-PEN would be better to keep it isolated here, but we
-	 * would need to fix isolation locking first.
-	 */
 	if (ret) {
 		pr_info("soft_offline: %#lx: invalidated\n", pfn);
 		page_handle_poison(page, false, true);
-- 
2.23.0

[PATCH v2 8/8] mm/memory-failure.c: remove unnecessary PageTransTail check

[Miaohe Lin]

When we reach here, we're guaranteed to have non-compound page as thp is
already splited. Remove this unnecessary PageTransTail check.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 5d2e653a145d..6e7baa90214a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1842,7 +1842,7 @@ int memory_failure(unsigned long pfn, int flags)
 	 * page_lock. We need wait writeback completion for this page or it
 	 * may trigger vfs BUG while evict inode.
 	 */
-	if (!PageTransTail(p) && !PageLRU(p) && !PageWriteback(p))
+	if (!PageLRU(p) && !PageWriteback(p))
 		goto identify_page_state;
 
 	/*
-- 
2.23.0

Re: [PATCH v2 4/8] mm/memory-failure.c: fix race with changing page more robustly

[HORIGUCHI NAOYA(堀口 直也)]

On Wed, Feb 16, 2022 at 05:14:27PM +0800, Miaohe Lin wrote:
> We're only intended to deal with the non-Compound page after we split thp
> in memory_failure. However, the page could have changed compound pages due
> to race window. If this happens, we could try again to hopefully handle the
> page next round. Also remove unneeded orig_head. It's always equal to the
> hpage. So we can use hpage directly and remove this redundant one.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7e205d91b2d7..d66f642888be 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1690,7 +1690,6 @@ int memory_failure(unsigned long pfn, int flags)
>  {
>  	struct page *p;
>  	struct page *hpage;
> -	struct page *orig_head;
>  	struct dev_pagemap *pgmap;
>  	int res = 0;
>  	unsigned long page_flags;
> @@ -1736,7 +1735,7 @@ int memory_failure(unsigned long pfn, int flags)
>  		goto unlock_mutex;
>  	}
>  
> -	orig_head = hpage = compound_head(p);
> +	hpage = compound_head(p);
>  	num_poisoned_pages_inc();
>  
>  	/*
> @@ -1817,13 +1816,18 @@ int memory_failure(unsigned long pfn, int flags)
>  	lock_page(p);
>  
>  	/*
> -	 * The page could have changed compound pages during the locking.
> -	 * If this happens just bail out.
> +	 * We're only intended to deal with the non-Compound page here.
> +	 * However, the page could have changed compound pages due to
> +	 * race window. If this happens, we could try again to hopefully
> +	 * handle the page next round.
>  	 */
> -	if (PageCompound(p) && compound_head(p) != orig_head) {
> -		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
> -		res = -EBUSY;
> -		goto unlock_page;
> +	if (PageCompound(p)) {
> +		if (TestClearPageHWPoison(p))
> +			num_poisoned_pages_dec();
> +		unlock_page(p);
> +		put_page(p);
> +		flags &= ~MF_COUNT_INCREASED;

Could you limit the retry chance only once by using the local variable
"retry"?  It might be very rare to hit the race more than once in a single
error event, but just to be safe from potential infinite loop (that could be
opened by future changes).

Thanks,
Naoya Horiguchi

> +		goto try_again;
>  	}
>  
>  	/*
> -- 
> 2.23.0

Re: [PATCH v2 4/8] mm/memory-failure.c: fix race with changing page more robustly

[Miaohe Lin]

On 2022/2/18 9:13, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Wed, Feb 16, 2022 at 05:14:27PM +0800, Miaohe Lin wrote:
>> We're only intended to deal with the non-Compound page after we split thp
>> in memory_failure. However, the page could have changed compound pages due
>> to race window. If this happens, we could try again to hopefully handle the
>> page next round. Also remove unneeded orig_head. It's always equal to the
>> hpage. So we can use hpage directly and remove this redundant one.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory-failure.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 7e205d91b2d7..d66f642888be 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1690,7 +1690,6 @@ int memory_failure(unsigned long pfn, int flags)
>>  {
>>  	struct page *p;
>>  	struct page *hpage;
>> -	struct page *orig_head;
>>  	struct dev_pagemap *pgmap;
>>  	int res = 0;
>>  	unsigned long page_flags;
>> @@ -1736,7 +1735,7 @@ int memory_failure(unsigned long pfn, int flags)
>>  		goto unlock_mutex;
>>  	}
>>  
>> -	orig_head = hpage = compound_head(p);
>> +	hpage = compound_head(p);
>>  	num_poisoned_pages_inc();
>>  
>>  	/*
>> @@ -1817,13 +1816,18 @@ int memory_failure(unsigned long pfn, int flags)
>>  	lock_page(p);
>>  
>>  	/*
>> -	 * The page could have changed compound pages during the locking.
>> -	 * If this happens just bail out.
>> +	 * We're only intended to deal with the non-Compound page here.
>> +	 * However, the page could have changed compound pages due to
>> +	 * race window. If this happens, we could try again to hopefully
>> +	 * handle the page next round.
>>  	 */
>> -	if (PageCompound(p) && compound_head(p) != orig_head) {
>> -		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
>> -		res = -EBUSY;
>> -		goto unlock_page;
>> +	if (PageCompound(p)) {
>> +		if (TestClearPageHWPoison(p))
>> +			num_poisoned_pages_dec();
>> +		unlock_page(p);
>> +		put_page(p);
>> +		flags &= ~MF_COUNT_INCREASED;
> 
> Could you limit the retry chance only once by using the local variable
> "retry"?  It might be very rare to hit the race more than once in a single
> error event, but just to be safe from potential infinite loop (that could be
> opened by future changes).
> 

Sure. Will do it in V3. Thanks.

> Thanks,
> Naoya Horiguchi
> 
>> +		goto try_again;
>>  	}
>>  
>>  	/*
>> -- 
>> 2.23.0