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 ;-)
