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

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

On Wed, 2019-06-19 at 22:10 -0400, Song Liu wrote:
> > On Jun 19, 2019, at 6:39 PM, Rik van Riel <riel@fb.com> wrote:
> > 
> > On Tue, 2019-06-18 at 23:24 -0700, Song Liu wrote:
> > 
> > > index 8563339041f6..bab8d9eef46c 100644
> > > --- a/mm/truncate.c
> > > +++ b/mm/truncate.c
> > > @@ -790,7 +790,11 @@ EXPORT_SYMBOL_GPL(invalidate_inode_pages2);
> > > void truncate_pagecache(struct inode *inode, loff_t newsize)
> > > {
> > > 	struct address_space *mapping = inode->i_mapping;
> > > -	loff_t holebegin = round_up(newsize, PAGE_SIZE);
> > > +	loff_t holebegin;
> > > +
> > > +	/* if non-shmem file has thp, truncate the whole file */
> > > +	if (filemap_nr_thps(mapping))
> > > +		newsize = 0;
> > > 
> > 
> > I don't get it. Sometimes truncate is used to
> > increase the size of a file, or to change it
> > to a non-zero size.
> > 
> > Won't forcing the newsize to zero break applications,
> > when the file is truncated to a different size than
> > they expect?
> 
> This is not truncate the file. It only drops page cache. 
> truncate_setsize() will still set correct size. I don't 
> think this breaks anything. 

Ahhh, indeed. Good point.

I wonder if the dropping of the page cache could be
done automatically from open(), if it determines that
there are no more readonly THP users of the file, and
the new opener wants to write to the file?

That magic could be in a helper function, so it would
be just a one line change in the same spot where it
currently denies the permission :)

> We can probably make it smarter and only drop the clean
> huge pages (dirty page should not exist).