Re: [PATCH v3 4/8] iomap: Remove unnecessary test from iomap_release_folio()

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jun 12, 2023 at 09:39:06PM +0100, Matthew Wilcox (Oracle) wrote:
> The check for the folio being under writeback is unnecessary; the caller
> has checked this and the folio is locked, so the folio cannot be under
> writeback at this point.
> 
> The comment is somewhat misleading in that it talks about one specific
> situation in which we can see a dirty folio.  There are others, so change
> the comment to explain why we can't release the iomap_page.

> +	 * If the folio is dirty, we refuse to release our metadata because
> +	 * it may be partially dirty (FIXME, add a test for that).

Argh, forgot to fix this.

        /*
         * If the folio is dirty, we refuse to release our metadata because
-        * it may be partially dirty (FIXME, add a test for that).
+        * it may be partially dirty.  Once we track per-block dirty state,
+        * we can release the metadata if every block is dirty.
         */

> -	if (folio_test_dirty(folio) || folio_test_writeback(folio))
> +	if (folio_test_dirty(folio))
>  		return false;

Now I'm wondering if we shouldn't also refuse to release the metadata if
the folio is !uptodate.  It's not a correctness issue, it's a performance
issue, and a question of whose priorities are more important.

If we do release the metadata on a partially uptodate folio, we'll
re-read the parts of the folio from storage which had previously been
successfully read.  If we don't release the metadata, we prevent the
MM from splitting the page (eg on truncate).

No obviously right answer here, IMO.