Re: [PATCH] fs: buffer: check huge page size instead of single page for invalidatepage

On Mon, Oct 11, 2021 at 12:57 PM Yang Shi [off-list ref] wrote:

On Mon, Sep 20, 2021 at 3:35 PM Yang Shi [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 2:50 PM Matthew Wilcox [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 02:23:41PM -0700, Yang Shi wrote:

quoted

On Sun, Sep 19, 2021 at 7:41 AM Matthew Wilcox [off-list ref] wrote:

quoted

On Fri, Sep 17, 2021 at 05:07:03PM -0700, Yang Shi wrote:

quoted

quoted

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/ (local))
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

quoted

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.