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.
