Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Jul 2023 10:09:53 +0900
From:   Naoya Horiguchi <naoya.horiguchi@linux.dev>
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     akpm@linux-foundation.org, naoya.horiguchi@nec.com,
        shy828301@gmail.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] mm: memory-failure: add PageOffline() check
Message-ID: <20230720010953.GC1583723@ik1-406-35019.vs.sakura.ne.jp>
References: <20230715031729.2420338-1-linmiaohe@huawei.com>
 <20230715031729.2420338-5-linmiaohe@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230715031729.2420338-5-linmiaohe@huawei.com>
Precedence: bulk

On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote:
> Memory failure is not interested in logically offlined page. Skip this
> type of pages.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 42e63b0ab5f7..ed79b69837de 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  	 * Here we are interested only in user-mapped pages, so skip any
>  	 * other types of pages.
>  	 */
> -	if (PageReserved(p) || PageSlab(p) || PageTable(p))
> +	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))

hwpoison_user_mappings() is called after some checks are done, so I'm not
sure that it's the right place to check PageOffline().
We could check it before setting PageHWPoison() as we do at the beginning of
memory_failure() around pfn_to_online_page().  Does it make sense?

Thanks,
Naoya Horiguchi

>  		return true;
>  	if (!(PageLRU(hpage) || PageHuge(p)))
>  		return true;
> @@ -2513,7 +2513,8 @@ int unpoison_memory(unsigned long pfn)
>  		goto unlock_mutex;
>  	}
>  
> -	if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio))
> +	if (folio_test_slab(folio) || PageTable(&folio->page) ||
> +	    folio_test_reserved(folio) || PageOffline(&folio->page))
>  		goto unlock_mutex;
>  
>  	/*
> -- 
> 2.33.0
> 
> 
>