| Message ID | 20230612203910.724378-5-willy@infradead.org (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | Create large folios in iomap buffered write path | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 08ee293c4117..2054b85c9d9b 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -483,12 +483,10 @@ bool iomap_release_folio(struct folio *folio, gfp_t gfp_flags) folio_size(folio)); /* - * mm accommodates an old ext3 case where clean folios might - * not have had the dirty bit cleared. Thus, it can send actual - * dirty folios to ->release_folio() via shrink_active_list(); - * skip those here. + * If the folio is dirty, we refuse to release our metadata because + * it may be partially dirty (FIXME, add a test for that). */ - if (folio_test_dirty(folio) || folio_test_writeback(folio)) + if (folio_test_dirty(folio)) return false; iomap_page_release(folio); return true;
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. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/iomap/buffered-io.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)