remove iomap_writepage v2

remove iomap_writepage v2

[Christoph Hellwig]

Hi all,

this series removes iomap_writepage and it's callers, following what xfs
has been doing for a long time.

Changes since v1:
 - clean up a printk in gfs2

Diffstat:
 fs/gfs2/aops.c         |   26 --------------------------
 fs/gfs2/log.c          |    5 ++---
 fs/iomap/buffered-io.c |   15 ---------------
 fs/zonefs/super.c      |    8 --------
 include/linux/iomap.h  |    3 ---
 5 files changed, 2 insertions(+), 55 deletions(-)

[PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one

[Christoph Hellwig]

Use filemap_fdatawrite_wbc instead of generic_writepages in
gfs2_ail1_start_one so that the functin can also cope with address_space
operations that only implement ->writepages and to properly account
for cgroup writeback.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/log.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index f0ee3ff6f9a87..a66e3b1f6d178 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -131,7 +131,7 @@ __acquires(&sdp->sd_ail_lock)
 		if (!mapping)
 			continue;
 		spin_unlock(&sdp->sd_ail_lock);
-		ret = generic_writepages(mapping, wbc);
+		ret = filemap_fdatawrite_wbc(mapping, wbc);
 		if (need_resched()) {
 			blk_finish_plug(plug);
 			cond_resched();
@@ -222,8 +222,7 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc)
 	spin_unlock(&sdp->sd_ail_lock);
 	blk_finish_plug(&plug);
 	if (ret) {
-		gfs2_lm(sdp, "gfs2_ail1_start_one (generic_writepages) "
-			"returned: %d\n", ret);
+		gfs2_lm(sdp, "gfs2_ail1_start_one returned: %d\n", ret);
 		gfs2_withdraw(sdp);
 	}
 	trace_gfs2_ail_flush(sdp, wbc, 0);
-- 
2.30.2

Re: [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one

[Andreas Gruenbacher]

Hi Christoph,

On Tue, Jul 19, 2022 at 7:07 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Use filemap_fdatawrite_wbc instead of generic_writepages in
> gfs2_ail1_start_one so that the functin can also cope with address_space
> operations that only implement ->writepages and to properly account
> for cgroup writeback.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/gfs2/log.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index f0ee3ff6f9a87..a66e3b1f6d178 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -131,7 +131,7 @@ __acquires(&sdp->sd_ail_lock)
>                 if (!mapping)
>                         continue;
>                 spin_unlock(&sdp->sd_ail_lock);
> -               ret = generic_writepages(mapping, wbc);
> +               ret = filemap_fdatawrite_wbc(mapping, wbc);

this patch unfortunately breaks journaled data inodes.

We're in function gfs2_ail1_start_one() here, which is usually called
via gfs2_log_flush() -> empty_ail1_list() -> gfs2_ail1_start() ->
gfs2_ail1_flush() -> gfs2_ail1_start_one(), and we're going through
the list of buffer heads in the transaction to be flushed. We used to
submit each dirty buffer for I/O individually, but since commit
5ac048bb7ea6 ("GFS2: Use filemap_fdatawrite() to write back the AIL"),
we're submitting all the dirty pages in the metadata address space
this buffer head belongs to. That's slightly bizarre, but it happens
to catch exactly the same buffer heads that are in the transaction, so
we end up with the same result. From what I'm being told, this was
done as a performance optimization -- but nobody actually knows the
details anymore.

The above change means that instead of calling generic_writepages(),
we end up calling filemap_fdatawrite_wbc() -> do_writepages() ->
mapping->a_ops->writepages(). But that's something completely
different; the writepages address space operation operates is outward
facing, while we really only want to write out the dirty buffers /
pages in the underlying address space. In case of journaled data
inodes, gfs2_jdata_writepages() actually ends up trying to create a
filesystem transaction, which immediately hangs because we're in the
middle of a log flush.

So I'm tempted to revert the following two of your commits; luckily
that's independent from the iomap_writepage() removal:

  d3d71901b1ea ("gfs2: remove ->writepage")
  b2b0a5e97855 ("gfs2: stop using generic_writepages in gfs2_ail1_start_one")

I think we could go through iomap_writepages() instead of
generic_writepages() here as well,  but that's for another day.

As far as cgroup writeback goes, this is journal I/O and I don't have
the faintest idea how the accounting for that is even supposed to
work.

>                 if (need_resched()) {
>                         blk_finish_plug(plug);
>                         cond_resched();
> @@ -222,8 +222,7 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct writeback_control *wbc)
>         spin_unlock(&sdp->sd_ail_lock);
>         blk_finish_plug(&plug);
>         if (ret) {
> -               gfs2_lm(sdp, "gfs2_ail1_start_one (generic_writepages) "
> -                       "returned: %d\n", ret);
> +               gfs2_lm(sdp, "gfs2_ail1_start_one returned: %d\n", ret);
>                 gfs2_withdraw(sdp);
>         }
>         trace_gfs2_ail_flush(sdp, wbc, 0);
> --
> 2.30.2
>

Thanks,
Andreas

Re: [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one

[Christoph Hellwig]

On Wed, Jan 18, 2023 at 10:22:20PM +0100, Andreas Gruenbacher wrote:
> The above change means that instead of calling generic_writepages(),
> we end up calling filemap_fdatawrite_wbc() -> do_writepages() ->
> mapping->a_ops->writepages(). But that's something completely
> different; the writepages address space operation operates is outward
> facing, while we really only want to write out the dirty buffers /
> pages in the underlying address space. In case of journaled data
> inodes, gfs2_jdata_writepages() actually ends up trying to create a
> filesystem transaction, which immediately hangs because we're in the
> middle of a log flush.
> 
> So I'm tempted to revert the following two of your commits; luckily
> that's independent from the iomap_writepage() removal:
> 
>   d3d71901b1ea ("gfs2: remove ->writepage")
>   b2b0a5e97855 ("gfs2: stop using generic_writepages in gfs2_ail1_start_one")

generic_writepages is gone in linux-next, and I'd really like to keep
it that way.  So if you have to do this, please open code it
using write_cache_pages and a direct call to the writepage method of
choice.

> I think we could go through iomap_writepages() instead of
> generic_writepages() here as well,  but that's for another day.

Well, that would obviously be much better, and actually help with the
goal of removing ->writepage.

[PATCH 2/4] gfs2: remove ->writepage

[Christoph Hellwig]

->writepage is only used for single page writeback from memory reclaim,
and not called at all for cgroup writeback.  Follow the lead of XFS
and remove ->writepage and rely entirely on ->writepages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/aops.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 106e90a365838..0240a1a717f56 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -81,31 +81,6 @@ static int gfs2_get_block_noalloc(struct inode *inode, sector_t lblock,
 	return 0;
 }
 
-/**
- * gfs2_writepage - Write page for writeback mappings
- * @page: The page
- * @wbc: The writeback control
- */
-static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
-{
-	struct inode *inode = page->mapping->host;
-	struct gfs2_inode *ip = GFS2_I(inode);
-	struct gfs2_sbd *sdp = GFS2_SB(inode);
-	struct iomap_writepage_ctx wpc = { };
-
-	if (gfs2_assert_withdraw(sdp, gfs2_glock_is_held_excl(ip->i_gl)))
-		goto out;
-	if (current->journal_info)
-		goto redirty;
-	return iomap_writepage(page, wbc, &wpc, &gfs2_writeback_ops);
-
-redirty:
-	redirty_page_for_writepage(wbc, page);
-out:
-	unlock_page(page);
-	return 0;
-}
-
 /**
  * gfs2_write_jdata_page - gfs2 jdata-specific version of block_write_full_page
  * @page: The page to write
@@ -765,7 +740,6 @@ bool gfs2_release_folio(struct folio *folio, gfp_t gfp_mask)
 }
 
 static const struct address_space_operations gfs2_aops = {
-	.writepage = gfs2_writepage,
 	.writepages = gfs2_writepages,
 	.read_folio = gfs2_read_folio,
 	.readahead = gfs2_readahead,
-- 
2.30.2

[PATCH 3/4] zonefs: remove ->writepage

[Christoph Hellwig]

->writepage is only used for single page writeback from memory reclaim,
and not called at all for cgroup writeback.  Follow the lead of XFS
and remove ->writepage and rely entirely on ->writepages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Acked-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/super.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 053299758deb9..062c3f1da0327 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -232,13 +232,6 @@ static const struct iomap_writeback_ops zonefs_writeback_ops = {
 	.map_blocks		= zonefs_write_map_blocks,
 };
 
-static int zonefs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	struct iomap_writepage_ctx wpc = { };
-
-	return iomap_writepage(page, wbc, &wpc, &zonefs_writeback_ops);
-}
-
 static int zonefs_writepages(struct address_space *mapping,
 			     struct writeback_control *wbc)
 {
@@ -266,7 +259,6 @@ static int zonefs_swap_activate(struct swap_info_struct *sis,
 static const struct address_space_operations zonefs_file_aops = {
 	.read_folio		= zonefs_read_folio,
 	.readahead		= zonefs_readahead,
-	.writepage		= zonefs_writepage,
 	.writepages		= zonefs_writepages,
 	.dirty_folio		= filemap_dirty_folio,
 	.release_folio		= iomap_release_folio,
-- 
2.30.2

Re: [PATCH 3/4] zonefs: remove ->writepage

[Chaitanya Kulkarni]

On 7/18/22 21:13, Christoph Hellwig wrote:
> ->writepage is only used for single page writeback from memory reclaim,
> and not called at all for cgroup writeback.  Follow the lead of XFS
> and remove ->writepage and rely entirely on ->writepages.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Acked-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH 4/4] iomap: remove iomap_writepage

[Christoph Hellwig]

Unused now.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/iomap/buffered-io.c | 15 ---------------
 include/linux/iomap.h  |  3 ---
 2 files changed, 18 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d2a9f699e17ed..1bac8bda40d0c 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1518,21 +1518,6 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data)
 	return 0;
 }
 
-int
-iomap_writepage(struct page *page, struct writeback_control *wbc,
-		struct iomap_writepage_ctx *wpc,
-		const struct iomap_writeback_ops *ops)
-{
-	int ret;
-
-	wpc->ops = ops;
-	ret = iomap_do_writepage(page, wbc, wpc);
-	if (!wpc->ioend)
-		return ret;
-	return iomap_submit_ioend(wpc, wpc->ioend, ret);
-}
-EXPORT_SYMBOL_GPL(iomap_writepage);
-
 int
 iomap_writepages(struct address_space *mapping, struct writeback_control *wbc,
 		struct iomap_writepage_ctx *wpc,
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index e552097c67e0b..911888560d3eb 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -303,9 +303,6 @@ void iomap_finish_ioends(struct iomap_ioend *ioend, int error);
 void iomap_ioend_try_merge(struct iomap_ioend *ioend,
 		struct list_head *more_ioends);
 void iomap_sort_ioends(struct list_head *ioend_list);
-int iomap_writepage(struct page *page, struct writeback_control *wbc,
-		struct iomap_writepage_ctx *wpc,
-		const struct iomap_writeback_ops *ops);
 int iomap_writepages(struct address_space *mapping,
 		struct writeback_control *wbc, struct iomap_writepage_ctx *wpc,
 		const struct iomap_writeback_ops *ops);
-- 
2.30.2

Re: [PATCH 4/4] iomap: remove iomap_writepage

[Chaitanya Kulkarni]

On 7/18/22 21:13, Christoph Hellwig wrote:
> Unused now.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 4/4] iomap: remove iomap_writepage

[Darrick J. Wong]

On Tue, Jul 19, 2022 at 06:13:11AM +0200, Christoph Hellwig wrote:
> Unused now.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 15 ---------------
>  include/linux/iomap.h  |  3 ---
>  2 files changed, 18 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index d2a9f699e17ed..1bac8bda40d0c 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1518,21 +1518,6 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data)
>  	return 0;
>  }
>  
> -int
> -iomap_writepage(struct page *page, struct writeback_control *wbc,
> -		struct iomap_writepage_ctx *wpc,
> -		const struct iomap_writeback_ops *ops)
> -{
> -	int ret;
> -
> -	wpc->ops = ops;
> -	ret = iomap_do_writepage(page, wbc, wpc);
> -	if (!wpc->ioend)
> -		return ret;
> -	return iomap_submit_ioend(wpc, wpc->ioend, ret);
> -}
> -EXPORT_SYMBOL_GPL(iomap_writepage);
> -
>  int
>  iomap_writepages(struct address_space *mapping, struct writeback_control *wbc,
>  		struct iomap_writepage_ctx *wpc,
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index e552097c67e0b..911888560d3eb 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -303,9 +303,6 @@ void iomap_finish_ioends(struct iomap_ioend *ioend, int error);
>  void iomap_ioend_try_merge(struct iomap_ioend *ioend,
>  		struct list_head *more_ioends);
>  void iomap_sort_ioends(struct list_head *ioend_list);
> -int iomap_writepage(struct page *page, struct writeback_control *wbc,
> -		struct iomap_writepage_ctx *wpc,
> -		const struct iomap_writeback_ops *ops);
>  int iomap_writepages(struct address_space *mapping,
>  		struct writeback_control *wbc, struct iomap_writepage_ctx *wpc,
>  		const struct iomap_writeback_ops *ops);
> -- 
> 2.30.2
> 

Re: remove iomap_writepage v2

[Jan Kara]

Hi Christoph!

On Tue 19-07-22 06:13:07, Christoph Hellwig wrote:
> this series removes iomap_writepage and it's callers, following what xfs
> has been doing for a long time.

So this effectively means "no writeback from page reclaim for these
filesystems" AFAICT (page migration of dirty pages seems to be handled by
iomap_migrate_page()) which is going to make life somewhat harder for
memory reclaim when memory pressure is high enough that dirty pages are
reaching end of the LRU list. I don't expect this to be a problem on big
machines but it could have some undesirable effects for small ones
(embedded, small VMs). I agree per-page writeback has been a bad idea for
efficiency reasons for at least last 10-15 years and most filesystems
stopped dealing with more complex situations (like block allocation) from
->writepage() already quite a few years ago without any bug reports AFAIK.
So it all seems like a sensible idea from FS POV but are MM people on board
or at least aware of this movement in the fs land?

Added a few CC's for that.

								Honza

> Changes since v1:
>  - clean up a printk in gfs2
> 
> Diffstat:
>  fs/gfs2/aops.c         |   26 --------------------------
>  fs/gfs2/log.c          |    5 ++---
>  fs/iomap/buffered-io.c |   15 ---------------
>  fs/zonefs/super.c      |    8 --------
>  include/linux/iomap.h  |    3 ---
>  5 files changed, 2 insertions(+), 55 deletions(-)
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: remove iomap_writepage v2

[Matthew Wilcox]

On Thu, Jul 28, 2022 at 01:10:16PM +0200, Jan Kara wrote:
> Hi Christoph!
> 
> On Tue 19-07-22 06:13:07, Christoph Hellwig wrote:
> > this series removes iomap_writepage and it's callers, following what xfs
> > has been doing for a long time.
> 
> So this effectively means "no writeback from page reclaim for these
> filesystems" AFAICT (page migration of dirty pages seems to be handled by
> iomap_migrate_page()) which is going to make life somewhat harder for
> memory reclaim when memory pressure is high enough that dirty pages are
> reaching end of the LRU list. I don't expect this to be a problem on big
> machines but it could have some undesirable effects for small ones
> (embedded, small VMs). I agree per-page writeback has been a bad idea for
> efficiency reasons for at least last 10-15 years and most filesystems
> stopped dealing with more complex situations (like block allocation) from
> ->writepage() already quite a few years ago without any bug reports AFAIK.
> So it all seems like a sensible idea from FS POV but are MM people on board
> or at least aware of this movement in the fs land?

I mentioned it during my folio session at LSFMM, but didn't put a huge
emphasis on it.

For XFS, writeback should already be in progress on other pages if
we're getting to the point of trying to call ->writepage() in vmscan.
Surely this is also true for other filesystems?

Re: remove iomap_writepage v2

[Dave Chinner]

On Thu, Jul 28, 2022 at 03:18:03PM +0100, Matthew Wilcox wrote:
> On Thu, Jul 28, 2022 at 01:10:16PM +0200, Jan Kara wrote:
> > Hi Christoph!
> > 
> > On Tue 19-07-22 06:13:07, Christoph Hellwig wrote:
> > > this series removes iomap_writepage and it's callers, following what xfs
> > > has been doing for a long time.
> > 
> > So this effectively means "no writeback from page reclaim for these
> > filesystems" AFAICT (page migration of dirty pages seems to be handled by
> > iomap_migrate_page()) which is going to make life somewhat harder for
> > memory reclaim when memory pressure is high enough that dirty pages are
> > reaching end of the LRU list. I don't expect this to be a problem on big
> > machines but it could have some undesirable effects for small ones
> > (embedded, small VMs). I agree per-page writeback has been a bad idea for
> > efficiency reasons for at least last 10-15 years and most filesystems
> > stopped dealing with more complex situations (like block allocation) from
> > ->writepage() already quite a few years ago without any bug reports AFAIK.
> > So it all seems like a sensible idea from FS POV but are MM people on board
> > or at least aware of this movement in the fs land?
> 
> I mentioned it during my folio session at LSFMM, but didn't put a huge
> emphasis on it.
> 
> For XFS, writeback should already be in progress on other pages if
> we're getting to the point of trying to call ->writepage() in vmscan.
> Surely this is also true for other filesystems?

Yes.

It's definitely true for btrfs, too, because btrfs_writepage does:

static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
{
        struct inode *inode = page->mapping->host;
        int ret;

        if (current->flags & PF_MEMALLOC) {
                redirty_page_for_writepage(wbc, page);
                unlock_page(page);
                return 0;
        }
....

It also rejects all calls to write dirty pages from memory reclaim
contexts.

ext4 will also reject writepage calls from memory allocation if
block allocation is required (due to delayed allocation) or
unwritten extents need converting to written. i.e. if it has to run
blocking transactions.

So all three major filesystems will either partially or wholly
reject ->writepage calls from memory reclaim context.

IOWs, if memory reclaim is depending on ->writepage() to make
reclaim progress, it's not working as advertised on the vast
majority of production Linux systems....

The reality is that ->writepage is a relic of a bygone era of OS and
filesystem design. It was useful in the days where writing a dirty
page just involved looking up the bufferhead attached to the page to
get the disk mapping and then submitting it for IO.

Those days are long gone - filesystems have complex IO submission
paths now that have to handle delayed allocation, copy-on-write,
unwritten extents, have unbound memory demand, etc. All the
filesystems that support these 1990s era filesystem technologies
simply turn off ->writepage in memory reclaim contexts.

Hence for the vast majority of linux users (i.e. everyone using
ext4, btrfs and XFS), ->writepage no longer plays any part in memory
reclaim on their systems.

So why should we try to maintain the fiction that ->writepage is
required functionality in a filesystem when it clearly isn't?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: remove iomap_writepage v2

[Yang Shi]

On Thu, Jul 28, 2022 at 3:48 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Thu, Jul 28, 2022 at 03:18:03PM +0100, Matthew Wilcox wrote:
> > On Thu, Jul 28, 2022 at 01:10:16PM +0200, Jan Kara wrote:
> > > Hi Christoph!
> > >
> > > On Tue 19-07-22 06:13:07, Christoph Hellwig wrote:
> > > > this series removes iomap_writepage and it's callers, following what xfs
> > > > has been doing for a long time.
> > >
> > > So this effectively means "no writeback from page reclaim for these
> > > filesystems" AFAICT (page migration of dirty pages seems to be handled by
> > > iomap_migrate_page()) which is going to make life somewhat harder for
> > > memory reclaim when memory pressure is high enough that dirty pages are
> > > reaching end of the LRU list. I don't expect this to be a problem on big
> > > machines but it could have some undesirable effects for small ones
> > > (embedded, small VMs). I agree per-page writeback has been a bad idea for
> > > efficiency reasons for at least last 10-15 years and most filesystems
> > > stopped dealing with more complex situations (like block allocation) from
> > > ->writepage() already quite a few years ago without any bug reports AFAIK.
> > > So it all seems like a sensible idea from FS POV but are MM people on board
> > > or at least aware of this movement in the fs land?
> >
> > I mentioned it during my folio session at LSFMM, but didn't put a huge
> > emphasis on it.
> >
> > For XFS, writeback should already be in progress on other pages if
> > we're getting to the point of trying to call ->writepage() in vmscan.
> > Surely this is also true for other filesystems?
>
> Yes.
>
> It's definitely true for btrfs, too, because btrfs_writepage does:
>
> static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
> {
>         struct inode *inode = page->mapping->host;
>         int ret;
>
>         if (current->flags & PF_MEMALLOC) {
>                 redirty_page_for_writepage(wbc, page);
>                 unlock_page(page);
>                 return 0;
>         }
> ....
>
> It also rejects all calls to write dirty pages from memory reclaim
> contexts.

Aha, it seems even kswapd (it has PF_MEMALLOC set) is rejected too.

>
> ext4 will also reject writepage calls from memory allocation if
> block allocation is required (due to delayed allocation) or
> unwritten extents need converting to written. i.e. if it has to run
> blocking transactions.
>
> So all three major filesystems will either partially or wholly
> reject ->writepage calls from memory reclaim context.
>
> IOWs, if memory reclaim is depending on ->writepage() to make
> reclaim progress, it's not working as advertised on the vast
> majority of production Linux systems....
>
> The reality is that ->writepage is a relic of a bygone era of OS and
> filesystem design. It was useful in the days where writing a dirty
> page just involved looking up the bufferhead attached to the page to
> get the disk mapping and then submitting it for IO.
>
> Those days are long gone - filesystems have complex IO submission
> paths now that have to handle delayed allocation, copy-on-write,
> unwritten extents, have unbound memory demand, etc. All the
> filesystems that support these 1990s era filesystem technologies
> simply turn off ->writepage in memory reclaim contexts.
>
> Hence for the vast majority of linux users (i.e. everyone using
> ext4, btrfs and XFS), ->writepage no longer plays any part in memory
> reclaim on their systems.
>
> So why should we try to maintain the fiction that ->writepage is
> required functionality in a filesystem when it clearly isn't?
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>

Re: remove iomap_writepage v2

[Mel Gorman]

On Thu, Jul 28, 2022 at 01:10:16PM +0200, Jan Kara wrote:
> Hi Christoph!
> 
> On Tue 19-07-22 06:13:07, Christoph Hellwig wrote:
> > this series removes iomap_writepage and it's callers, following what xfs
> > has been doing for a long time.
> 
> So this effectively means "no writeback from page reclaim for these
> filesystems" AFAICT (page migration of dirty pages seems to be handled by
> iomap_migrate_page()) which is going to make life somewhat harder for
> memory reclaim when memory pressure is high enough that dirty pages are
> reaching end of the LRU list. I don't expect this to be a problem on big
> machines but it could have some undesirable effects for small ones
> (embedded, small VMs). I agree per-page writeback has been a bad idea for
> efficiency reasons for at least last 10-15 years and most filesystems
> stopped dealing with more complex situations (like block allocation) from
> ->writepage() already quite a few years ago without any bug reports AFAIK.
> So it all seems like a sensible idea from FS POV but are MM people on board
> or at least aware of this movement in the fs land?
> 
> Added a few CC's for that.
> 

There is some context missing because it's not clear what the full impact is
but it is definitly the case that writepage is ignored in some contexts for
common filesystems so lets assume that writepage from reclaim context always
failed as a worst case scenario. Certainly this type of change is something
linux-mm needs to be aware of because we've been blind-sided before.

I don't think it would be incredibly damaging although there *might* be
issues with small systems or cgroups. In many respects, vmscan has been
moving in this direction for a long time e.g. f84f6e2b0868 ("mm: vmscan:
do not writeback filesystem pages in kswapd except in high priority") and
e2be15f6c3ee ("mm: vmscan: stall page reclaim and writeback pages based on
dirty/writepage pages encountered"). This was roughly 10 years ago when
it was clear that FS writeback from reclaim context was fragile (iirc it
was partially due to concerns about stack depth and later concerns that a
filesystem would simply ignore the writeback request). There also is less
reliance on stalling reclaim by queueing and waiting on writeback but we now
should explicitly throttle if no progress is being made e.g. 69392a403f49
("mm/vmscan: throttle reclaim when no progress is being made") and some
follow-up fixes.

There still is a reliance on swap and shmem pages will not ignore writepage
but I assume that is still ok.

One potential caveat is if wakeup_flusher_threads() is ignored because
there is a reliance in reclaim that if all the pages at the tail of the
LRU are dirty pages not queued for writeback then wakeup_flusher_threads()
will do something so pages get marked for immediate reclaim when writeback
completes. Of course there is no guarantee that flusher threads will start
writeback on the old pages and the pages could be backed by a slow BDI
but wakeup_flusher_threads() should not be ignored.

Another caveat is that comments become misleading. Take for example the
comment "Only kswapd can writeback filesystem folios to avoid risk of
stack overflow." The wording should change to note that writepage may
do nothing at all.  There also might need to be some adjustment on when
pages get marked for immediate reclaim when they are dirty but not under
writeback. pageout() might need a tracepoint for "mapping->a_ops->writepage
== NULL" to help debug problems around a failure to queue pages for writback
although that could be done as a test patch for a bug.

There would need to be some changes made if writepage always or often failed
and there might be some premature throttling based on "NOPROGRESS" on small
systems due to dirty-not-writepage pages at the tail of the LRU but I don't
think it would be an immediate disaster, Reclaim throttling is no longer
based on the ability to queue for writeback or "congestion" state of BDIs
and some care is taken to not prematurely stall on NOPROGRESS. However,
if there was a bug related to premature stalls or excessive CPU usage
from direct reclaimers or kswapd that bisected to a change in writepage
then it should be fixed on the vmscan-side to put an emphasis on handling
"reclaim is in trouble when the bulk of reclaimable pages are FS-only,
dirty and not under writeback but ->writepage is a no-op".

-- 
Mel Gorman
SUSE Labs

Re: remove iomap_writepage v2

[Christoph Hellwig]

On Fri, Jul 29, 2022 at 10:22:16AM +0100, Mel Gorman wrote:
> There is some context missing because it's not clear what the full impact is
> but it is definitly the case that writepage is ignored in some contexts for
> common filesystems so lets assume that writepage from reclaim context always
> failed as a worst case scenario. Certainly this type of change is something
> linux-mm needs to be aware of because we've been blind-sided before.

Between willy and Johannes pushing or it I was under the strong assumption
that linux-mm knows of it..

> I don't think it would be incredibly damaging although there *might* be
> issues with small systems or cgroups. 

Johannes specifically mentioned that cgroup writeback will never call
into ->writepage anyway.

Re: remove iomap_writepage v2

[Johannes Weiner]

On Fri, Jul 29, 2022 at 04:11:45PM +0200, Christoph Hellwig wrote:
> On Fri, Jul 29, 2022 at 10:22:16AM +0100, Mel Gorman wrote:
> > There is some context missing because it's not clear what the full impact is
> > but it is definitly the case that writepage is ignored in some contexts for
> > common filesystems so lets assume that writepage from reclaim context always
> > failed as a worst case scenario. Certainly this type of change is something
> > linux-mm needs to be aware of because we've been blind-sided before.
> 
> Between willy and Johannes pushing or it I was under the strong assumption
> that linux-mm knows of it..

Yes, the context was an FS session at LSFMM. FS folks complained about
the MM relying on single-page writeouts. On the MM side we've invested
a lot into eliminating this dependency over the last decade or so, and
I would argue it's gone today. But the calls are still in the code,
and so FS folks continue to operate under the old assumption. You
can't blame them. I suggested we remove the callbacks to clarify
things and eliminate that murky corner from the FS/MM interface.

Compaction/migration is easy. It simply never calls writepage when
there is a migratepage callback - which major filesystems have.

Reclaim may still call it, but the invocation rules are so restrictive
nowadays that it's unlikely to actually help when it matters (and we
know it makes things worse in many cases).

For example, cgroup reclaim isn't ever allowed to call writepage.
This covers the small system scenario.

Whether writepage helps under OOM is not clear. OOMing systems tend to
have thundering herds of direct reclaimers, any one of which can
declare OOM if they fail, yet none of them can write pages. They
already rely on another thread to make progress. That thread can be
the flushers writing in offset order, or kswapd writing in LRU
order. You could argue that LRU order, while less efficient IO,
launders pages closer to the scanner. From an OOM perspective that
doesn't matter, though, because scanners will work through the entire
LRU list several times before giving up. They won't miss flusher
progress. That leaves reclaim efficiency - having to scan fewer pages
before potentially finding clean ones. But it's an IO bound scenario,
so arguably efficient IO would seem more important than efficient CPU.

The risk of this change, IMO, is exposing reclaim to flat out bugs in
the flusher code, or bugs in the code that matches reclaim to flushing
speed. However, a) cgroup has been relying on those for a decade. And
b) we've been treating writepage calls like bugs due to the latency
they inject into workloads, and tuned the MM to rely more on flushers
(e.g. c55e8d035b28 ("mm: vmscan: move dirty pages out of the way until
they're flushed")). So we know this stuff works at scale and with real
workloads. I think the risk of dragons there is quite low.

XFS hasn't had a ->writepage call for a while. After LSF I internally
tested dropping btrfs' callback, and the results looked good: no OOM
kills with dirty/writeback pages remaining, performance parity. Then I
went on vacation and Christoph beat me to the patch :)

I think it's a really good cleanup that makes things cleaner and more
predictable in both the fs and the mm.

> > I don't think it would be incredibly damaging although there *might* be
> > issues with small systems or cgroups. 
> 
> Johannes specifically mentioned that cgroup writeback will never call
> into ->writepage anyway.

Yes, cgroup has relied on the flushers since

commit ee72886d8ed5d9de3fa0ed3b99a7ca7702576a96
Author: Mel Gorman <mel@csn.ul.ie>
Date:   Mon Oct 31 17:07:38 2011 -0700

    mm: vmscan: do not writeback filesystem pages in direct reclaim

since cgroup reclaim == direct reclaim.

Re: remove iomap_writepage v2

[Matthew Wilcox]

On Mon, Aug 01, 2022 at 11:31:50AM -0400, Johannes Weiner wrote:
> XFS hasn't had a ->writepage call for a while. After LSF I internally
> tested dropping btrfs' callback, and the results looked good: no OOM
> kills with dirty/writeback pages remaining, performance parity. Then I
> went on vacation and Christoph beat me to the patch :)

To avoid duplicating work with you or Christoph ... it seems like the
plan is to kill ->writepage entirely soon, so there's no point in me
doing a sweep of all the filesystems to convert ->writepage to
->write_folio, correct?

I assume the plan for filesystems which have a writepage but don't have
a ->writepages (9p, adfs, affs, bfs, ecryptfs, gfs2, hostfs, jfs, minix,
nilfs2, ntfs, ocfs2, reiserfs, sysv, ubifs, udf, ufs, vboxsf) is to give
them a writepages, modelled on iomap_writepages().  Seems that adding
a block_writepages() might be a useful thing for me to do?

Re: remove iomap_writepage v2

[Andreas Grünbacher]

Am Mi., 10. Aug. 2022 um 22:57 Uhr schrieb Matthew Wilcox <willy@infradead.org>:
> On Mon, Aug 01, 2022 at 11:31:50AM -0400, Johannes Weiner wrote:
> > XFS hasn't had a ->writepage call for a while. After LSF I internally
> > tested dropping btrfs' callback, and the results looked good: no OOM
> > kills with dirty/writeback pages remaining, performance parity. Then I
> > went on vacation and Christoph beat me to the patch :)
>
> To avoid duplicating work with you or Christoph ... it seems like the
> plan is to kill ->writepage entirely soon, so there's no point in me
> doing a sweep of all the filesystems to convert ->writepage to
> ->write_folio, correct?
>
> I assume the plan for filesystems which have a writepage but don't have
> a ->writepages (9p, adfs, affs, bfs, ecryptfs, gfs2, hostfs, jfs, minix,
> nilfs2, ntfs, ocfs2, reiserfs, sysv, ubifs, udf, ufs, vboxsf) is to give
> them a writepages, modelled on iomap_writepages().  Seems that adding
> a block_writepages() might be a useful thing for me to do?

Hmm, gfs2 does have gfs2_writepages() and gfs2_jdata_writepages()
functions, so it should probably be fine.

Thanks,
Andreas

Re: remove iomap_writepage v2

[Matthew Wilcox]

On Wed, Aug 10, 2022 at 11:32:06PM +0200, Andreas Grünbacher wrote:
> Am Mi., 10. Aug. 2022 um 22:57 Uhr schrieb Matthew Wilcox <willy@infradead.org>:
> > On Mon, Aug 01, 2022 at 11:31:50AM -0400, Johannes Weiner wrote:
> > > XFS hasn't had a ->writepage call for a while. After LSF I internally
> > > tested dropping btrfs' callback, and the results looked good: no OOM
> > > kills with dirty/writeback pages remaining, performance parity. Then I
> > > went on vacation and Christoph beat me to the patch :)
> >
> > To avoid duplicating work with you or Christoph ... it seems like the
> > plan is to kill ->writepage entirely soon, so there's no point in me
> > doing a sweep of all the filesystems to convert ->writepage to
> > ->write_folio, correct?
> >
> > I assume the plan for filesystems which have a writepage but don't have
> > a ->writepages (9p, adfs, affs, bfs, ecryptfs, gfs2, hostfs, jfs, minix,
> > nilfs2, ntfs, ocfs2, reiserfs, sysv, ubifs, udf, ufs, vboxsf) is to give
> > them a writepages, modelled on iomap_writepages().  Seems that adding
> > a block_writepages() might be a useful thing for me to do?
> 
> Hmm, gfs2 does have gfs2_writepages() and gfs2_jdata_writepages()
> functions, so it should probably be fine.

Ah, it's gfs2_aspace_writepage which doesn't have a writepages
counterpart.  I haven't looked at it to understand why it's needed.
(gfs2_meta_aops and gfs2_rgrp_aops)

Re: remove iomap_writepage v2

[Christoph Hellwig]

On Wed, Aug 10, 2022 at 09:43:58PM +0100, Matthew Wilcox wrote:
> To avoid duplicating work with you or Christoph ... it seems like the
> plan is to kill ->writepage entirely soon, so there's no point in me
> doing a sweep of all the filesystems to convert ->writepage to
> ->write_folio, correct?

While I won't commit to concrete definition of "soon" I think that is
the general plan, and I don't think converting ->writepage to
->write_folio is needed.

> I assume the plan for filesystems which have a writepage but don't have
> a ->writepages (9p, adfs, affs, bfs, ecryptfs, gfs2, hostfs, jfs, minix,
> nilfs2, ntfs, ocfs2, reiserfs, sysv, ubifs, udf, ufs, vboxsf) is to give
> them a writepages,

and  ->migrate_folio if missing, yes.

> modelled on iomap_writepages().  Seems that adding
> a block_writepages() might be a useful thing for me to do?

We have mpage_writepages which basically is the ->writepages counterpart
to block_write_full_page.  The only caveat is of course that it can
use multi-block mappings from the get_bock callback, so we need to make
sure the file systems don't do anything stupid there.