lkml.org 
[lkml]   [2021]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] fs: buffer: check huge page size instead of single page for invalidatepage
On Mon, Oct 11, 2021 at 12:57 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Mon, Sep 20, 2021 at 3:35 PM Yang Shi <shy828301@gmail.com> wrote:
> >
> > On Mon, Sep 20, 2021 at 2:50 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Sep 20, 2021 at 02:23:41PM -0700, Yang Shi wrote:
> > > > On Sun, Sep 19, 2021 at 7:41 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Fri, Sep 17, 2021 at 05:07:03PM -0700, Yang Shi wrote:
> > > > > > > The debugging showed the page passed to invalidatepage is a huge page
> > > > > > > and the length is the size of huge page instead of single page due to
> > > > > > > read only FS THP support. But block_invalidatepage() would throw BUG if
> > > > > > > the size is greater than single page.
> > > > >
> > > > > Things have already gone wrong before we get to this point. See
> > > > > do_dentry_open(). You aren't supposed to be able to get a writable file
> > > > > descriptor on a file which has had huge pages added to the page cache
> > > > > without the filesystem's knowledge. That's the problem that needs to
> > > > > be fixed.
> > > >
> > > > I don't quite understand your point here. Do you mean do_dentry_open()
> > > > should fail for such cases instead of truncating the page cache?
> > >
> > > No, do_dentry_open() should have truncated the page cache when it was
> > > called and found that there were THPs in the cache. Then khugepaged
> > > should see that someone has the file open for write and decline to create
> > > new THPs. So it shouldn't be possible to get here with THPs in the cache.
> >
>
> I think Hugh's skipping special file patch
> (https://lore.kernel.org/linux-mm/a07564a3-b2fc-9ffe-3ace-3f276075ea5c@google.com/)
> could fix this specific BUG report and seems like a more proper fix to
> this.
>
> However, it still doesn't make too much sense to have thp_size passed
> to do_invalidatepage(), then have PAGE_SIZE hardcoded in a BUG
> assertion IMHO. So it seems this patch is still useful because
> block_invalidatepage() is called by a few filesystems as well, for
> example, ext4. Or I'm wondering whether we should call
> do_invalidatepage() for each subpage of THP in truncate_cleanup_page()
> since private is for each subpage IIUC.

Seems no interest?

Anyway the more I was staring at the code the more I thought calling
do_invalidatepage() for each subpage made more sense. So, something
like the below makes sense?

diff --git a/mm/truncate.c b/mm/truncate.c
index 714eaf19821d..9048f498cd02 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -169,11 +169,16 @@ void do_invalidatepage(struct page *page,
unsigned int offset,
*/
static void truncate_cleanup_page(struct page *page)
{
+ int nr = thp_nr_pages(page);
+ int i;
+
if (page_mapped(page))
unmap_mapping_page(page);

- if (page_has_private(page))
- do_invalidatepage(page, 0, thp_size(page));
+ for (i = 0; i < nr; i++) {
+ if (page_has_private(page + i))
+ do_invalidatepage(page + i, 0, PAGE_SIZE);
+ }

/*
* Some filesystems seem to re-dirty the page even after
>
> > AFAICT, it does so.
> >
> > In do_dentry_open():
> > /*
> > * XXX: Huge page cache doesn't support writing yet. Drop all page
> > * cache for this file before processing writes.
> > */
> > if (f->f_mode & FMODE_WRITE) {
> > /*
> > * Paired with smp_mb() in collapse_file() to ensure nr_thps
> > * is up to date and the update to i_writecount by
> > * get_write_access() is visible. Ensures subsequent insertion
> > * of THPs into the page cache will fail.
> > */
> > smp_mb();
> > if (filemap_nr_thps(inode->i_mapping))
> > truncate_pagecache(inode, 0);
> > }
> >
> >
> > In khugepaged:
> > filemap_nr_thps_inc(mapping);
> > /*
> > * Paired with smp_mb() in do_dentry_open() to ensure
> > * i_writecount is up to date and the update to nr_thps is
> > * visible. Ensures the page cache will be truncated if the
> > * file is opened writable.
> > */
> > smp_mb();
> > if (inode_is_open_for_write(mapping->host)) {
> > result = SCAN_FAIL;
> > __mod_lruvec_page_state(new_page, NR_FILE_THPS, -nr);
> > filemap_nr_thps_dec(mapping);
> > goto xa_locked;
> > }
> >
> > But I'm not quite sure if there is any race condition.

\
 
 \ /
  Last update: 2021-10-21 01:40    [W:0.500 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site