Re: [next-20220215] WARNING at fs/iomap/buffered-io.c:75 with xfstests

[Matthew Wilcox <willy@infradead.org>]

On Wed, Feb 16, 2022 at 09:35:04AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 16, 2022 at 12:55:02PM +0530, Sachin Sant wrote:
> > [ 2547.662818] ------------[ cut here ]------------
> > [ 2547.662832] WARNING: CPU: 24 PID: 2463718 at fs/iomap/buffered-io.c:75 iomap_page_release+0x1b0/0x1e0
> 
> ...and I think this is complaining about this debugging test in
> iomap_page_release:
> 
> 	WARN_ON_ONCE(bitmap_full(iop->uptodate, nr_blocks) !=
> 			folio_test_uptodate(folio));
> 
> which checks that we're not releasing or invalidating a page that's
> partially uptodate on a blocksize < pagesize filesystem (or so I gather
> from "POWER10 LPAR" (64k pages?) and "XFS" (4k blocks?))...

This happens _all_ _the_ _time_ in my testing.  If your block size is
less than page size, you can expect it.

What it's supposed to be testing is that we remembered to set the
uptodate flag once all blocks in this page are uptodate.  What's
tripping the check is iomap_writepage_map()'s stupid clearing of the
uptodate flag on the folio:

        if (unlikely(error)) {
...
                if (!count) {
                        folio_clear_uptodate(folio);
                        folio_unlock(folio);
                        goto done;

What particularly annoys me about this is that the folio _was_ uptodate,
and it was dirty, so it has newer data in it than is on disk, but we're
going to re-read the folio from disk and throw away that user data.

> Given that in this case we've already cleared SB_ACTIVE from the
> superblock s_flags, I wonder if we could amend that code to read:
> 
> 	if (inode->i_sb->s_flags & SB_ACTIVE)
> 		WARN_ON_ONCE(bitmap_full(iop->uptodate, nr_blocks) !=
> 				folio_test_uptodate(folio));
> 
> Since we don't really care about memory pages that aren't fully up to
> date if we're in the midst of freeing all the incore filesystem state.
> 
> Thoughts?

Seems like papering over a bad design decision to me.