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

From: Jan Kara <jack@suse.cz>
To: Theodore Ts'o <tytso@mit.edu>
Cc: Jan Kara <jack@suse.cz>,
	linux-ext4@vger.kernel.org, Christoph Hellwig <hch@infradead.org>,
	"Ritesh Harjani (IBM)" <ritesh.list@gmail.com>,
	Christoph Hellwig <hch@lst.de>,
	Matthew Wilcox <willy@infradead.org>,
	linux-mm@kvack.org
Subject: Re: [PATCH v3 11/12] ext4: Stop providing .writepage hook
Date: Tue, 6 Dec 2022 11:52:25 +0100	[thread overview]
Message-ID: <20221206105225.nr734teqlkueqdph@quack3> (raw)
In-Reply-To: <Y460RpKTCDuPKWmN@mit.edu>

[-- 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

