From: Matthew Wilcox <willy@infradead.org>
To: Jan Kara <jack@suse.cz>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
Hugh Dickins <hughd@google.com>,
William Kucharski <william.kucharski@oracle.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Yang Shi <yang.shi@linux.alibaba.com>,
Dave Chinner <dchinner@redhat.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 11/12] mm/truncate,shmem: Handle truncates that split THPs
Date: Wed, 30 Sep 2020 15:51:14 +0100 [thread overview]
Message-ID: <20200930145114.GQ20115@casper.infradead.org> (raw)
In-Reply-To: <20200930115919.GS10896@quack2.suse.cz>
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 ;-)
next prev parent reply other threads:[~2020-09-30 14:51 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-14 13:00 [PATCH v2 00/12] Overhaul multi-page lookups for THP Matthew Wilcox (Oracle)
2020-09-14 13:00 ` [PATCH v2 01/12] mm: Make pagecache tagged lookups return only head pages Matthew Wilcox (Oracle)
2020-09-29 9:13 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 02/12] mm/shmem: Use pagevec_lookup in shmem_unlock_mapping Matthew Wilcox (Oracle)
2020-09-29 8:28 ` Jan Kara
2020-09-29 18:36 ` Matthew Wilcox
2020-09-14 13:00 ` [PATCH v2 03/12] mm/filemap: Add helper for finding pages Matthew Wilcox (Oracle)
2020-09-29 8:27 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 04/12] mm/filemap: Add mapping_seek_hole_data Matthew Wilcox (Oracle)
2020-09-29 8:46 ` Jan Kara
2020-09-29 12:42 ` Matthew Wilcox
2020-09-29 13:39 ` Matthew Wilcox
2020-09-14 13:00 ` [PATCH v2 05/12] mm: Add and use find_lock_entries Matthew Wilcox (Oracle)
2020-09-29 8:58 ` Jan Kara
2020-09-29 12:48 ` Matthew Wilcox
2020-09-30 10:40 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 06/12] mm: Add an 'end' parameter to find_get_entries Matthew Wilcox (Oracle)
2020-09-14 13:00 ` [PATCH v2 07/12] mm: Add an 'end' parameter to pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-09-29 9:02 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 08/12] mm: Remove nr_entries parameter from pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-09-29 9:03 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 09/12] mm: Pass pvec directly to find_get_entries Matthew Wilcox (Oracle)
2020-09-29 9:07 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 10/12] mm: Remove pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-09-29 9:08 ` Jan Kara
2020-09-14 13:00 ` [PATCH v2 11/12] mm/truncate,shmem: Handle truncates that split THPs Matthew Wilcox (Oracle)
2020-09-30 11:59 ` Jan Kara
2020-09-30 14:51 ` Matthew Wilcox [this message]
2020-09-14 13:00 ` [PATCH v2 12/12] mm/filemap: Return only head pages from find_get_entries Matthew Wilcox (Oracle)
2020-09-30 12:15 ` Jan Kara
2020-09-30 12:36 ` Matthew Wilcox
2020-09-30 17:08 ` Jan Kara
2020-09-30 17:23 ` Matthew Wilcox
2020-10-01 7:17 ` Jan Kara
2020-10-25 23:19 ` Matthew Wilcox
2020-10-26 9:11 ` Jan Kara
2020-09-28 20:13 ` [PATCH v2 00/12] Overhaul multi-page lookups for THP Matthew Wilcox
2020-09-29 8:50 ` William Kucharski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200930145114.GQ20115@casper.infradead.org \
--to=willy@infradead.org \
--cc=akpm@linux-foundation.org \
--cc=dchinner@redhat.com \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=jack@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=william.kucharski@oracle.com \
--cc=yang.shi@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).