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

[Jane Chu <jane.chu@oracle.com>]

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?

Add Shiyang Ruan for comment.

thanks,

-jane



> -	} else
> +	else
>   		tk->size_shift = page_shift(compound_head(p));
>   
>   	/*
> @@ -475,7 +465,7 @@ static void add_to_kill_anon_file(struct task_struct *tsk, struct page *p,
>   				  struct vm_area_struct *vma,
>   				  struct list_head *to_kill)
>   {
> -	__add_to_kill(tsk, p, vma, to_kill, 0, FSDAX_INVALID_PGOFF);
> +	__add_to_kill(tsk, p, vma, to_kill, 0);
>   }
>   
>   #ifdef CONFIG_KSM
> @@ -493,10 +483,10 @@ static bool task_in_to_kill_list(struct list_head *to_kill,
>   }
>   void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>   		     struct vm_area_struct *vma, struct list_head *to_kill,
> -		     unsigned long ksm_addr)
> +		     unsigned long addr)
>   {
>   	if (!task_in_to_kill_list(to_kill, tsk))
> -		__add_to_kill(tsk, p, vma, to_kill, ksm_addr, FSDAX_INVALID_PGOFF);
> +		__add_to_kill(tsk, p, vma, to_kill, addr);
>   }
>   #endif
>   /*
> @@ -670,7 +660,8 @@ static void add_to_kill_fsdax(struct task_struct *tsk, struct page *p,
>   			      struct vm_area_struct *vma,
>   			      struct list_head *to_kill, pgoff_t pgoff)
>   {
> -	__add_to_kill(tsk, p, vma, to_kill, 0, pgoff);
> +	unsigned long addr = vma_pgoff_address(pgoff, 1, vma);
> +	__add_to_kill(tsk, p, vma, to_kill, addr);
>   }
>   
>   /*