Re: [PATCH 1/2] mm: Add unmap_mapping_file() helper

Hello,

> > + * unmap_mapping_file() - Unmap all mmaps of an open file.
> > + * @file: The file to unmap.
> 
> We are unmapping pages/folios, not really mmaps.
> 
> "Unmap folio from all mmaps of a file." ?

Got it.  I will make sure that v2 has the wording improved.

> > + * space it is attached to.  This also covers files with f_mapping
> > + * swapped to a different address space at open time, since such VMAs
> > + * cannot be found through the file's own inode mapping.
> 
> God this is confusing :)

Sorry about that!  I am not very familiar with the inner workings of
mm/, so please bear with me here a little. :)

> Sorry for the stupid question, but why can't we call
> 
> 	unmap_mapping_range(file->f_mapping, 0, 0, 1);

Since commit 636b21b50152 ("PCI: Revoke mappings like devmem") the PCI
resource and legacy files swap their f_mapping to the shared iomem
address space returned by iomem_get_mapping(), so that revoke_iomem()
can zap existing mappings when a driver claims a region.

That address space holds the VMAs of every resourceN file of every PCI
device, the /proc/bus/pci mappings, and every /dev/mem mapping in the
system.

So, calling unmap_mapping_range(file->f_mapping, 0, 0, 1) from
kernfs_drain_open_files() would then zap all of them on any
single device removal as a side effect.  This is not ideal.

I have tried exactly that, to be sure: mmap resource0 of two different
devices, then remove one of them.  The mapping of the other device was
torn down as well, and the next access through it got SIGBUS, even
though that device was alive and bound.

Hence the new helper, especially since kernfs does not know which
physical range belongs to the file being removed.  Only the PCI side
does.  The struct file in vma->vm_file was the only thing I could
think of to use as a filter.

> What is a bit annoying is that you now do in the caller:
> 
> 	if (of->file->f_mapping != inode->i_mapping)
> 		unmap_mapping_file(of->file);
> 	else
> 		unmap_mapping_range(inode->i_mapping, 0, 0, 1);
> 
> Shouldn't we just have one function that takes care of that internally ... so
> it's harder to get wrong?

The idea was to introduce as little change under mm/ as possible, keeping
things inside kernfs, which is the de facto consumer of the API from mm/,
so I wanted to make it obvious at the call site what needs to be done, so
to speak.

I will change it, such that in v2, kernfs will simply call
unmap_mapping_file(), and the helper will take care of both cases
internally.

Thank you for the review!

	Krzysztof