| Message ID | 20220508203247.668791-25-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert aops->releasepage to aops->release_folio | expand |
On Sun, May 08, 2022 at 09:32:45PM +0100, Matthew Wilcox (Oracle) wrote: > Saves a few calls to compound_head(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Acked-by: Theodore Ts'o <tytso@mit.edu> ... although I have one question: > - get_page(page); > + folio_get(folio); > __brelse(bh); > - try_to_free_buffers(page); > - unlock_page(page); > - put_page(page); > + try_to_free_buffers(&folio->page); The patches in this series seem to assume that the folio contains only a single page. Or at least, a *lot* more detailed auditing to make sure the right thing would happen if a page happens to be a member of a folio with more than a single page. Should we be adding some kind of WARN_ON to check this particular assumption? - Ted
On Mon, May 09, 2022 at 09:23:10AM -0400, Theodore Ts'o wrote: > On Sun, May 08, 2022 at 09:32:45PM +0100, Matthew Wilcox (Oracle) wrote: > > Saves a few calls to compound_head(). > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Acked-by: Theodore Ts'o <tytso@mit.edu> > > ... although I have one question: > > > - get_page(page); > > + folio_get(folio); > > __brelse(bh); > > - try_to_free_buffers(page); > > - unlock_page(page); > > - put_page(page); > > + try_to_free_buffers(&folio->page); > > The patches in this series seem to assume that the folio contains only > a single page. Or at least, a *lot* more detailed auditing to make > sure the right thing would happen if a page happens to be a member of > a folio with more than a single page. > > Should we be adding some kind of WARN_ON to check this particular > assumption? You're right, a lot of places assume a single-page folio. Since filesystems need to explicitly enable large folios (by calling mapping_set_large_folios()), I haven't been adding assertions to aops entry points. I have been adding them to some common utility functions in fs/buffer.c eg block_read_full_folio has: VM_BUG_ON_FOLIO(folio_test_large(folio), folio); Other functions in fs/buffer.c look like they should handle large folios OK, eg buffer_check_dirty_writeback() and block_dirty_folio() and then others have existing assertions which are going to trigger if you try to use them with large folios, like __block_write_begin_int() has BUG_ON(from > PAGE_SIZE); and I haven't bothered to add assertions to either of those classes of function. JBD2 somewhat straddles a line between being considered "generic utility functions" and "part of a filesystem". I could go either way on adding assertions to document that this hasn't been audited for large folios. Although seeing '&folio->page' is always a sign that you should probably audit the function. By the end of this series, it seems to me that jbd2's release_buffer_page() is actually large-folio-safe. There's nothing in it that relies on the size of the folio, it calls functions that are large-folio-safe and the only actual use of 'page' in it is to get bh->b_page ... which should probably be in a union with a new ->b_folio so we don't need to muck around with calling page_folio().
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index ac7f067b7bdd..2f37108da0ec 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -62,6 +62,7 @@ static void journal_end_buffer_io_sync(struct buffer_head *bh, int uptodate) */ static void release_buffer_page(struct buffer_head *bh) { + struct folio *folio; struct page *page; if (buffer_dirty(bh)) @@ -71,18 +72,19 @@ static void release_buffer_page(struct buffer_head *bh) page = bh->b_page; if (!page) goto nope; - if (page->mapping) + folio = page_folio(page); + if (folio->mapping) goto nope; /* OK, it's a truncated page */ - if (!trylock_page(page)) + if (!folio_trylock(folio)) goto nope; - get_page(page); + folio_get(folio); __brelse(bh); - try_to_free_buffers(page); - unlock_page(page); - put_page(page); + try_to_free_buffers(&folio->page); + folio_unlock(folio); + folio_put(folio); return; nope:
Saves a few calls to compound_head(). Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/jbd2/commit.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)