Re: [PATCH v2 11/12] mm/truncate,shmem: Handle truncates that split THPs

[Matthew Wilcox]

On Wed, Sep 30, 2020 at 01:59:19PM +0200, Jan Kara wrote:
> > @@ -931,33 +904,39 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
> >  		index++;
> >  	}
> >  
> > -	if (partial_start) {
> > -		struct page *page = NULL;
> > -		shmem_getpage(inode, start - 1, &page, SGP_READ);
> > -		if (page) {
> > -			unsigned int top = PAGE_SIZE;
> > -			if (start > end) {
> > -				top = partial_end;
> > -				partial_end = 0;
> > -			}
> > -			zero_user_segment(page, partial_start, top);
> > -			set_page_dirty(page);
> > -			unlock_page(page);
> > -			put_page(page);
> > +	index = -1;
> > +	if (end != -1 && ((lend + 1) % PAGE_SIZE))
> 				    ^^
> Hum, is this guaranteed to compile properly on 32-bit archs without
> optimization? It would be 64-bit division... Maybe we don't care, it just
> caught my eye...

Looks like GCC properly reduces it:

00000000 <f>:
int f(long long x)
{
	if ((x + 1) % 4096)
   0:	8b 44 24 04          	mov    0x4(%esp),%eax
   4:	83 c0 01             	add    $0x1,%eax
   7:	25 ff 0f 00 00       	and    $0xfff,%eax
   c:	0f 95 c0             	setne  %al
   f:	0f b6 c0             	movzbl %al,%eax
		return 1;
	return 0;
}
  12:	c3                   	ret    

> BTW you could just drop end != -1 part because end == -1 iff lend == -1 so
> (lend + 1) % PAGE_SIZE is stronger.

Yes.  I think that actually makes it easier to read.

> > +		index = lend >> PAGE_SHIFT;
> > +	page = NULL;
> > +	shmem_getpage(inode, lstart >> PAGE_SHIFT, &page, SGP_READ);
> > +	if (page) {
> > +		bool same_page;
> > +
> > +		page = thp_head(page);
> > +		same_page = lend + 1 < page_offset(page) + thp_size(page);
> 			    ^^^^^^^^ Just lend here?

Oops.  Yes.  If lstart is 4096 and lend is 8191, this is definitely same_page.

> > -	if (partial_end) {
> > -		struct page *page = NULL;
> > +
> > +	if (index != -1)
> >  		shmem_getpage(inode, end, &page, SGP_READ);
> > -		if (page) {
> > -			zero_user_segment(page, 0, partial_end);
> > -			set_page_dirty(page);
> > -			unlock_page(page);
> > -			put_page(page);
> > -		}
> > +	if (page) {
> > +		page = thp_head(page);
> > +		set_page_dirty(page);
> > +		if (!truncate_inode_partial_page(page, lstart, lend))
> > +			end = page->index;
> > +		unlock_page(page);
> > +		put_page(page);
> >  	}
> > -	if (start >= end)
> > -		return;
> 
> You use 'index' effectively as bool in all of the above (only ever check
> index != -1). And effectively you only use it to communicate whether tail
> partial page got already handled or not. Maybe there's some less cryptic
> way to achieve that? Even separate bool just for that would be probably
> better that this.

As you note below, it makes more sense in truncate.c and for my own
sanity I was trying to keep the two functions as similar as possible.
I'm not sure why I looked up the page with 'end' instead of 'index'.

This didn't end up being as big a simplification as I thought it was going
to be.  I think I'll reintroduce partial_end as a bool, like you suggest.

> >  	index = start;
> >  	while (index < end) {
> 
> > diff --git a/mm/truncate.c b/mm/truncate.c
> > index d62aeffbffcc..06ed2f93069d 100644
> > --- a/mm/truncate.c
> > +++ b/mm/truncate.c
> > @@ -224,6 +224,53 @@ int truncate_inode_page(struct address_space *mapping, struct page *page)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Handle partial (transparent) pages.  The page may be entirely within the
> > + * range if a split has raced with us.  If not, we zero the part of the
> > + * page that's within the (start, end] range, and then split the page if
>                              ^ '[' here - start is inclusive as well...

Good point.

> > + * it's a THP.  split_page_range() will discard pages which now lie beyond
> > + * i_size, and we rely on the caller to discard pages which lie within a
> > + * newly created hole.
> > + *
> > + * Returns false if THP splitting failed so the caller can can avoid
> 							  ^^^^^^^ just 'can'

You're going to put Randy out of a job ;-)