Re: [PATCH 1/8] mm/memory-failure: Remove fsdax_pgoff argument from __add_to_kill

[Matthew Wilcox <willy@infradead.org>]

On Tue, Mar 12, 2024 at 07:07:10PM -0700, Jane Chu wrote:
> On 2/29/2024 1:20 PM, Matthew Wilcox (Oracle) wrote:
> 
> > Unify the KSM and DAX codepaths by calculating the addr in
> > add_to_kill_fsdax() instead of telling __add_to_kill() to calculate it.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > ---
> >   mm/memory-failure.c | 27 +++++++++------------------
> >   1 file changed, 9 insertions(+), 18 deletions(-)
> > 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 9349948f1abf..9356227a50bb 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -416,21 +416,13 @@ static unsigned long dev_pagemap_mapping_shift(struct vm_area_struct *vma,
> >    * not much we can do.	We just print a message and ignore otherwise.
> >    */
> > -#define FSDAX_INVALID_PGOFF ULONG_MAX
> > -
> >   /*
> >    * Schedule a process for later kill.
> >    * Uses GFP_ATOMIC allocations to avoid potential recursions in the VM.
> > - *
> > - * Note: @fsdax_pgoff is used only when @p is a fsdax page and a
> > - * filesystem with a memory failure handler has claimed the
> > - * memory_failure event. In all other cases, page->index and
> > - * page->mapping are sufficient for mapping the page back to its
> > - * corresponding user virtual address.
> >    */
> >   static void __add_to_kill(struct task_struct *tsk, struct page *p,
> >   			  struct vm_area_struct *vma, struct list_head *to_kill,
> > -			  unsigned long ksm_addr, pgoff_t fsdax_pgoff)
> > +			  unsigned long addr)
> >   {
> >   	struct to_kill *tk;
> > @@ -440,12 +432,10 @@ static void __add_to_kill(struct task_struct *tsk, struct page *p,
> >   		return;
> >   	}
> > -	tk->addr = ksm_addr ? ksm_addr : page_address_in_vma(p, vma);
> > -	if (is_zone_device_page(p)) {
> > -		if (fsdax_pgoff != FSDAX_INVALID_PGOFF)
> > -			tk->addr = vma_pgoff_address(fsdax_pgoff, 1, vma);
> > +	tk->addr = addr ? addr : page_address_in_vma(p, vma);
> > +	if (is_zone_device_page(p))
> >   		tk->size_shift = dev_pagemap_mapping_shift(vma, tk->addr);
> 
> Not sure about the simplification.  The fsdax special calculation was added by
> 
> commit c36e2024957120566efd99395b5c8cc95b5175c1
> Author: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> Date:   Fri Jun 3 13:37:29 2022 +0800
> 
>     mm: introduce mf_dax_kill_procs() for fsdax case
> 
>     This new function is a variant of mf_generic_kill_procs that accepts a
>     file, offset pair instead of a struct to support multiple files sharing a
>     DAX mapping.  It is intended to be called by the file systems as part of
>     the memory_failure handler after the file system performed a reverse
>     mapping from the storage address to the file and file offset.
> 
>     Link: https://lkml.kernel.org/r/20220603053738.1218681-6-ruansy.fnst@fujitsu.co
> m
> [..]
> 
> @@ -354,9 +357,15 @@ static void add_to_kill(struct task_struct *tsk, struct page *p,
> 
>         }
> 
>         tk->addr = page_address_in_vma(p, vma);
> 
> -       if (is_zone_device_page(p))
> 
> -               tk->size_shift = dev_pagemap_mapping_shift(p, vma);
> 
> -       else
> 
> +       if (is_zone_device_page(p)) {
> 
> +               /*
> 
> +                * Since page->mapping is not used for fsdax, we need
> 
> +                * calculate the address based on the vma.
> 
> +                */
> 
> +               if (p->pgmap->type == MEMORY_DEVICE_FS_DAX)
> 
> +                       tk->addr = vma_pgoff_address(fsdax_pgoff, 1, vma);
> 
> +               tk->size_shift = dev_pagemap_mapping_shift(vma, tk->addr);
> 
> +       } else
> 
>                 tk->size_shift = page_shift(compound_head(p));
> 
> Looks like it was to deal away with this check
> 
> unsignedlongpage_address_in_vma <https://elixir.bootlin.com/linux/v6.8/C/ident/page_address_in_vma>(structpage
> <https://elixir.bootlin.com/linux/v6.8/C/ident/page>*page
> <https://elixir.bootlin.com/linux/v6.8/C/ident/page>,structvm_area_struct
> <https://elixir.bootlin.com/linux/v6.8/C/ident/vm_area_struct>*vma
> <https://elixir.bootlin.com/linux/v6.8/C/ident/vma>)
> { [..]
> 
> }elseif(vma <https://elixir.bootlin.com/linux/v6.8/C/ident/vma>->vm_file
> <https://elixir.bootlin.com/linux/v6.8/C/ident/vm_file>->f_mapping
> <https://elixir.bootlin.com/linux/v6.8/C/ident/f_mapping>!=folio
> <https://elixir.bootlin.com/linux/v6.8/C/ident/folio>->mapping
> <https://elixir.bootlin.com/linux/v6.8/C/ident/mapping>){
> return-EFAULT <https://elixir.bootlin.com/linux/v6.8/C/ident/EFAULT>;
> }
> 
> But, by providing nr=1 in vma_pgoff_address(fsdax_pgoff, 1, vma), it also avoided dealing with
> 
> wrap around, which perhaps doesn't matter? perhaps noone runs fsdax on 32-bit machine?

I don't understand what you're saying is wrong with this patch.
All I'm doing (apart from the renaming of ksm_addr to addr) is moving
the vma_pgoff_address() call from __add_to_kill() to its caller.
Is there some reason this doesn't work?