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

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

On 3/13/2024 8:51 PM, Matthew Wilcox wrote:

> On Wed, Mar 13, 2024 at 11:11:04AM -0700, Jane Chu wrote:
>> On 3/12/2024 8:23 PM, Matthew Wilcox wrote:
>>> 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?
>> Sorry for not being clear.   What I meant to say is that, before the patch,
>> page_address_in_vma(p, vma) is the means for determining tk->addr, except
>> for fsdax when filesystems made a claim of wanting to participate in the UE
>> handling, in that case, vma_pgoff_address(fsdax_pgoff, 1, vma) is used
>> instead. The difference between the two means is that, although the former
>> eventually calls the latter _if_ vma->vm_file->f_mapping exists and is
>> stable, what I am unclear from Shiyang Ruan's earlier patch is that, he
>> seems to be concerning a case where f_mapping is not reliable, hence his
>> patch went straight to call vma_pgoff_address(fsdax_pgoff, 1, vma), and on
>> top of that, providing nr=1 to ignore the address wrap around case.
>>
>> So I don't know whether removing the fsdax special case is okay.
> I don't think I'm removing the fsdax special case, just moving it.
>
> If I subtract out the renaming from this patch, it looks like:
>
>   	tk->addr = addr ? 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);
> +	if (is_zone_device_page(p)) {
> ...
>   {
> -	__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);
>   }
>
> So instead of passing in '0' as the addr from add_to_kill_fsdax(),
> we do the address lookup in add_to_kill_fsdax() and pass it in.
> That means we don't call page_address_in_vma() for DAX because
> addr is not 0.

You're right!  I overlooked the addr being passed in in fsdax case is 
different.

Thanks for taking the time explaining to me.

Reviewed-by: Jane Chu <jane.chu@oracle.com>

-jane