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

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
under jdb2's hands.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Hi Jan,

I love your patch! Yet something to improve:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.1-rc8 next-20221205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/ext4-Stop-using-ext4_writepage-for-writeout-of-ordered-data/20221205-203446
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20221205122928.21959-11-jack%40suse.cz
patch subject: [PATCH v3 11/12] ext4: Stop providing .writepage hook
config: s390-allmodconfig
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b256889d6555a91b973b0ea5830a693c5f32f133
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jan-Kara/ext4-Stop-using-ext4_writepage-for-writeout-of-ordered-data/20221205-203446
        git checkout b256889d6555a91b973b0ea5830a693c5f32f133
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "buffer_migrate_folio_norefs" [fs/ext4/ext4.ko] undefined!
ERROR: modpost: "devm_ioremap_resource" [drivers/dma/qcom/hdma.ko] undefined!
ERROR: modpost: "devm_ioremap_resource" [drivers/dma/fsl-edma.ko] undefined!
ERROR: modpost: "devm_ioremap_resource" [drivers/dma/idma64.ko] undefined!
ERROR: modpost: "iounmap" [drivers/tty/ipwireless/ipwireless.ko] undefined!
ERROR: modpost: "ioremap" [drivers/tty/ipwireless/ipwireless.ko] undefined!
ERROR: modpost: "devm_platform_ioremap_resource" [drivers/char/xillybus/xillybus_of.ko] undefined!
ERROR: modpost: "devm_memremap" [drivers/misc/open-dice.ko] undefined!
ERROR: modpost: "devm_memunmap" [drivers/misc/open-dice.ko] undefined!
ERROR: modpost: "iounmap" [drivers/net/ethernet/8390/pcnet_cs.ko] undefined!
WARNING: modpost: suppressed 16 unresolved symbol warnings because there were too many)

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

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

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!

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