| Message ID | 20230602222445.2284892-4-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Create large folios in iomap buffered write path | expand |
On Fri, Jun 02, 2023 at 11:24:40PM +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. Do we need a debug assertion here to validate that filemap_release_folio has already filtered out folios unergoing writeback? The documentation change in the next patch might be fine since you're the pagecache maintainer. > 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(-) > > 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). Er... is this FIXME reflective of incomplete code? --D > */ > - if (folio_test_dirty(folio) || folio_test_writeback(folio)) > + if (folio_test_dirty(folio)) > return false; > iomap_page_release(folio); > return true; > -- > 2.39.2 >
On Sun, Jun 04, 2023 at 11:01:41AM -0700, Darrick J. Wong wrote: > On Fri, Jun 02, 2023 at 11:24:40PM +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. > > Do we need a debug assertion here to validate that filemap_release_folio > has already filtered out folios unergoing writeback? The documentation > change in the next patch might be fine since you're the pagecache > maintainer. I don't think so? We don't usually include asserts in filesystems that the VFS is living up to its promises. > > /* > > - * 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). > > Er... is this FIXME reflective of incomplete code? It's a note to Ritesh ;-) Once we have per-block dirty bits, if all the dirty bits are set, we can free the iomap_page without losing any information. But I don't want to introduce a dependency between this and Ritesh's work.
> - * 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). I'd prefer not to add this FIXME as we're fine without the per-block dirty tracking. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
Matthew Wilcox <willy@infradead.org> writes: > On Sun, Jun 04, 2023 at 11:01:41AM -0700, Darrick J. Wong wrote: >> On Fri, Jun 02, 2023 at 11:24:40PM +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. >> >> Do we need a debug assertion here to validate that filemap_release_folio >> has already filtered out folios unergoing writeback? The documentation >> change in the next patch might be fine since you're the pagecache >> maintainer. > > I don't think so? We don't usually include asserts in filesystems that > the VFS is living up to its promises. > >> > /* >> > - * 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). >> >> Er... is this FIXME reflective of incomplete code? > > It's a note to Ritesh ;-) > > Once we have per-block dirty bits, if all the dirty bits are set, we > can free the iomap_page without losing any information. But I don't > want to introduce a dependency between this and Ritesh's work. Sure Matthew, noted. However this looks more like an optimization which can be done for cases where we have all bits as dirty, than a dependency. -ritesh
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(-)