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@xxxxxxxxxxxxx>
>
> Acked-by: Theodore Ts'o <tytso@xxxxxxx>
>
> ... 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().
