Re: [PATCH 3/3] mm: memory-failure: make action_result() return int

[Kefeng Wang <wangkefeng.wang@huawei.com>]

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