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

[Yang Shi <shy828301@gmail.com>]

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.