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;
Message-ID: <e803ec8e-a20a-949c-88b0-aee250f98208@huawei.com>
Date:   Mon, 24 Oct 2022 09:39:20 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH 3/3] mm: memory-failure: make action_result() return int
Content-Language: en-US
To:     =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= 
        <naoya.horiguchi@nec.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Miaohe Lin <linmiaohe@huawei.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20221021084611.53765-1-wangkefeng.wang@huawei.com>
 <20221021084611.53765-3-wangkefeng.wang@huawei.com>
 <20221023235633.GC4024629@hori.linux.bs1.fc.nec.co.jp>
From:   Kefeng Wang <wangkefeng.wang@huawei.com>
In-Reply-To: <20221023235633.GC4024629@hori.linux.bs1.fc.nec.co.jp>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2022/10/24 7:56, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Fri, Oct 21, 2022 at 04:46:11PM +0800, Kefeng Wang wrote:
>> Check mf_result in action_result(), only return 0 when MF_RECOVERED,
>> or return -EBUSY, which will simplify code a bit.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Thanks for the cleanup, Kefeng.
> I basically agree with the change. I have one comment below ...
>
>> ---
>>   mm/memory-failure.c | 42 ++++++++++++++++--------------------------
>>   1 file changed, 16 insertions(+), 26 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index ca0199d0f79d..3f469e2da047 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1182,14 +1182,16 @@ static struct page_state error_states[] = {
>>    * "Dirty/Clean" indication is not 100% accurate due to the possibility of
>>    * setting PG_dirty outside page lock. See also comment above set_page_dirty().
>>    */
>> -static void action_result(unsigned long pfn, enum mf_action_page_type type,
>> -			  enum mf_result result)
>> +static int action_result(unsigned long pfn, enum mf_action_page_type type,
>> +			 enum mf_result result)
>>   {
>>   	trace_memory_failure_event(pfn, type, result);
>>   
>>   	num_poisoned_pages_inc();
>>   	pr_err("%#lx: recovery action for %s: %s\n",
>>   		pfn, action_page_types[type], action_name[result]);
>> +
>> +	return result == MF_RECOVERED ? 0 : -EBUSY;
> I think that MF_DELAYED may be considered as success (returning 0), then
> page_action() can be cleaned up a little more (like below?)
Yes, MF_DELAYED should be considered as success,
>
>      static int page_action(struct page_state *ps, struct page *p,
>                              unsigned long pfn)
>      {
>              int result;
>      
>              /* page p should be unlocked after returning from ps->action().  */
>              result = ps->action(ps, p);
>      
>              /* Could do more checks here if page looks ok */
>              /*
>               * Could adjust zone counters here to correct for the missing page.
>               */
>      
>              return action_result(pfn, ps->type, result);
>      }
>
> Existing direct callers (I mean action_result() called from other than
> page_action()) are never called with result==MF_DELAYED, so this change
> should not affect them.
I will refresh this patch, thanks.
> Does it make sense for you?
>
> Thanks,
> Naoya Horiguchi