Re: [PATCH v3 11/12] ext4: Stop providing .writepage hook

Re: [PATCH v3 11/12] ext4: Stop providing .writepage hook

[Theodore Ts'o]

On Mon, Dec 05, 2022 at 01:29:25PM +0100, Jan Kara wrote:
> Now we don't need .writepage hook for anything anymore. Reclaim is fine
> with relying on .writepages to clean pages and we often couldn't do much
> from the .writepage callback anyway. We only need to provide
> .migrate_folio callback for the ext4_journalled_aops - let's use
> buffer_migrate_page_norefs() there so that buffers cannot be modified
  ^^^^^^^^^^^^^^^^^^^^^^^^^^  this should be buffer_migrate_folio_norefs, no?
> under jdb2's hands.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jan Kara <jack@suse.cz>

Could you clarify in the commit how critical it is to use the
_norefs() variant?  It's not entirely clear what you mean by "let's
use...".  I think what is meant is that we need to use ..._noref() or
we can get in trouble if while the page update is getting committed,
there is an attempted to migrate the folio containing the page.

buffer_migrate_folio_norefs() is currently not exported (although
buffer_migrate_folio is).  So if we need it for ext4, we're going to
have to EXPORT_SYMBOL buffer_migrate_folio_norefs.

Any objections from the mm folks?

				- Ted

Re: [PATCH v3 11/12] ext4: Stop providing .writepage hook

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

On Mon 05-12-22 22:17:26, Theodore Ts'o wrote:
> On Mon, Dec 05, 2022 at 01:29:25PM +0100, Jan Kara wrote:
> > Now we don't need .writepage hook for anything anymore. Reclaim is fine
> > with relying on .writepages to clean pages and we often couldn't do much
> > from the .writepage callback anyway. We only need to provide
> > .migrate_folio callback for the ext4_journalled_aops - let's use
> > buffer_migrate_page_norefs() there so that buffers cannot be modified
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^  this should be buffer_migrate_folio_norefs, no?
> > under jdb2's hands.
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Could you clarify in the commit how critical it is to use the
> _norefs() variant?  It's not entirely clear what you mean by "let's
> use...".  I think what is meant is that we need to use ..._noref() or
> we can get in trouble if while the page update is getting committed,
> there is an attempted to migrate the folio containing the page.

Exacly. For example when commit code does writeout of transaction buffers
in jbd2_journal_write_metadata_buffer(), we don't hold page lock or have
page writeback bit set or have buffer locked. So page migration code would
go and happily migrate the page elsewhere while the copy is running thus
corrupting data.

I've added this to the changelog.

> buffer_migrate_folio_norefs() is currently not exported (although
> buffer_migrate_folio is).  So if we need it for ext4, we're going to
> have to EXPORT_SYMBOL buffer_migrate_folio_norefs.
> 
> Any objections from the mm folks?

I don't expect any objection. The only reason we didn't export that
function when I've added it was that only blkdev code was using it and that
cannot be compiled as a module. Should I send a patch to 

I've added a patch to the series to export this function. It is attached.

I can also repost the whole series if these are the only changes that block
the inclusion.


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-mm-Export-buffer_migrate_folio_norefs.patch --]
[-- Type: text/x-patch, Size: 767 bytes --]

From 69eb9d34de54862f8653a3c2ca4f96891e69f64b Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 6 Dec 2022 11:42:29 +0100
Subject: [PATCH] mm: Export buffer_migrate_folio_norefs()

Ext4 needs this function to allow safe migration for journalled data
pages.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/migrate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/migrate.c b/mm/migrate.c
index dff333593a8a..f00f5f6607b2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -820,6 +820,7 @@ int buffer_migrate_folio_norefs(struct address_space *mapping,
 {
 	return __buffer_migrate_folio(mapping, dst, src, mode, true);
 }
+EXPORT_SYMBOL(buffer_migrate_folio_norefs);
 #endif
 
 int filemap_migrate_folio(struct address_space *mapping,
-- 
2.35.3

Re: [PATCH v3 11/12] ext4: Stop providing .writepage hook

[Christoph Hellwig]

On Tue, Dec 06, 2022 at 11:52:25AM +0100, Jan Kara wrote:
> I don't expect any objection. The only reason we didn't export that
> function when I've added it was that only blkdev code was using it and that
> cannot be compiled as a module. Should I send a patch to 
> 
> I've added a patch to the series to export this function. It is attached.
> 
> I can also repost the whole series if these are the only changes that block
> the inclusion.

I'd do an EXPORT_SYMBOL_GPL, but otherwise the export looks fine, and it
would be good to get this conversion going!

Re: [PATCH v3 11/12] ext4: Stop providing .writepage hook

[Jan Kara]

On Tue 06-12-22 23:38:50, Christoph Hellwig wrote:
> On Tue, Dec 06, 2022 at 11:52:25AM +0100, Jan Kara wrote:
> > I don't expect any objection. The only reason we didn't export that
> > function when I've added it was that only blkdev code was using it and that
> > cannot be compiled as a module. Should I send a patch to 
> > 
> > I've added a patch to the series to export this function. It is attached.
> > 
> > I can also repost the whole series if these are the only changes that block
> > the inclusion.
> 
> I'd do an EXPORT_SYMBOL_GPL, but otherwise the export looks fine, and it
> would be good to get this conversion going!

OK, I've just copied how buffer_migrate_folio() is exported but I don't
mind a GPL export. I'll change it and push out new version of the series.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR