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

On Wed, Oct 20, 2021 at 05:24:09PM -0700, Yang Shi wrote:

On Wed, Oct 20, 2021 at 4:51 PM Matthew Wilcox [off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 04:38:49PM -0700, Yang Shi wrote:

quoted

quoted

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?

No.  I have changes in this area as part of the folio patchset (where
we end up converting this to invalidate_folio).  I'm not really
interested in doing anything before that, since this shouldn't be
reachable today.

Understood. But this is definitely reachable unless Hugh's patch
(skipping non-regular file) is applied.

Right.  That's the bug that needs to be fixed.  Seeing THPs here is
a symptom.  Getting rid of the error just makes the problem silent.

quoted

quoted

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?

Definitely not.  We want to invalidate the entire folio at once.

I didn't look at the folio patch (on each individual patch level), but
I'm supposed it still needs to invalidate buffer for each subpage,
right?

No.  Buffers are tracked for the entire folio, not on each subpage.

Actually, the filesystem people currently believe that the O(n^2) nature
of buffer-head handling mean that it's a bad idea to create multi-page
folios for bufferhead based filesystems (which includes block devices),
and the correct path forward is to migrate away from buffer-heads.
That may change, but it's the current plan.