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

[Kefeng Wang]

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@xxxxxxxxxx>
> 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