Re: [PATCH v4 6/6] mm,thp: avoid writes to file with THP in pagecache

[Rik van Riel <riel@fb.com>]

On Thu, 2019-06-20 at 10:27 -0700, Song Liu wrote:

> +++ b/mm/mmap.c
> @@ -3088,6 +3088,18 @@ int vm_brk(unsigned long addr, unsigned long
> len)
>  }
>  EXPORT_SYMBOL(vm_brk);
>  
> +static inline void release_file_thp(struct vm_area_struct *vma)
> +{
> +#ifdef CONFIG_READ_ONLY_THP_FOR_FS
> +	struct file *file = vma->vm_file;
> +
> +	if (file && (vma->vm_flags & VM_DENYWRITE) &&
> +	    atomic_read(&file_inode(file)->i_writecount) == 0 &&
> +	    filemap_nr_thps(file_inode(file)->i_mapping))
> +		truncate_pagecache(file_inode(file), 0);
> +#endif
> +}
> +
>  /* Release all mmaps. */
>  void exit_mmap(struct mm_struct *mm)
>  {
> @@ -3153,6 +3165,8 @@ void exit_mmap(struct mm_struct *mm)
>  	while (vma) {
>  		if (vma->vm_flags & VM_ACCOUNT)
>  			nr_accounted += vma_pages(vma);
> +
> +		release_file_thp(vma);
>  		vma = remove_vma(vma);
>  	}
>  	vm_unacct_memory(nr_accounted);

I like how you make the file accessible again to other
users, but am somewhat unsure about the mechanism used.

First, if multiple processes have the same file mmapped,
do you really want to blow away the page cache?

Secondly, by hooking into exit_mmap, you miss making
files writable again that get unmapped through munmap.

Would it be better to blow away the page cache when
the last mmap user unmaps it?

The page->mapping->i_mmap interval tree will be empty
when nobody has the file mmap()d.

Alternatively, open() could check whether the file is
currently mmaped, and blow away the page cache then.
That would leave the page cache intact if the same file 
gets execve()d several times in a row without any writes
in-between, which seems like it might be a relatively
common case.