diff mbox series

[2/3] nfs: remove reliance on bdi congestion

Message ID 164360183350.4233.691070075155620959.stgit@noble.brown (mailing list archive)
State New
Headers show
Series remove dependence of inode_congested() | expand

Commit Message

NeilBrown Jan. 31, 2022, 4:03 a.m. UTC
The bdi congestion tracking in not widely used and will be removed.

NFS is one of a small number of filesystems that uses it, setting just
the async (write) congestion flag at what it determines are appropriate
times.

The only remaining effect of the async flag is to cause (some)
WB_SYNC_NONE writes to be skipped.

So instead of setting the flag, set an internal flag and change:
 - .writepages to do nothing if WB_SYNC_NONE and the flag is set
 - .writepage to return AOP_WRITEPAGE_ACTIVATE if WB_SYNC_NONE
    and the flag is set.

The writepages change causes a behavioural change in that pageout() can
now return PAGE_ACTIVATE instead of PAGE_KEEP, so SetPageActive() will
be called on the page which (I think) wil further delay the next attempt
at writeout.  This might be a good thing.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/write.c            |   12 ++++++++++--
 include/linux/nfs_fs_sb.h |    1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Matthew Wilcox Jan. 31, 2022, 4:22 a.m. UTC | #1
On Mon, Jan 31, 2022 at 03:03:53PM +1100, NeilBrown wrote:
>  - .writepage to return AOP_WRITEPAGE_ACTIVATE if WB_SYNC_NONE
>     and the flag is set.

Is this actually useful?  I ask because Dave Chinner believes
the call to ->writepage in vmscan to be essentially unused.
See commit 21b4ee7029c9, and I had a followup discussion with him
on IRC:

<willy> dchinner: did you gather any stats on how often ->writepage was
	being called by pageout() before "xfs: drop ->writepage completely"
	was added?
<dchinner> willy: Never saw it on XFS in 3 years in my test environment...
<dchinner> I don't ever recall seeing the memory reclaim guards we put on
	->writepage in XFS ever firing - IIRC they'd been there for the best
	part of a decade.
<willy> not so much the WARN_ON firing but the case where it actually calls
	iomap_writepage
<dchinner> willy: I mean both - I was running with a local patch that warned
	on writepage for a long time, regardless of where it was called from.

I can believe things are different for a network filesystem, or maybe
XFS does background writeback better than other filesystems, but it
would be intriguing to be able to get rid of ->writepage altogether
(or at least from pageout(); migrate.c may be a thornier proposition).
NeilBrown Jan. 31, 2022, 4:55 a.m. UTC | #2
On Mon, 31 Jan 2022, Matthew Wilcox wrote:
> On Mon, Jan 31, 2022 at 03:03:53PM +1100, NeilBrown wrote:
> >  - .writepage to return AOP_WRITEPAGE_ACTIVATE if WB_SYNC_NONE
> >     and the flag is set.
> 
> Is this actually useful?  I ask because Dave Chinner believes
> the call to ->writepage in vmscan to be essentially unused.

He would be wrong ...  unless "essentially" means "mostly" rather than
"totally".
swap-out to NFS results in that ->writepage call.

Of course swap_writepage ignores sync_mode, so this might not be
entirely relevant.

But the main point of the patch is not to return AOP_WRITEPAGE_ACTIVATE
to vmscan.  It is to avoid writing at all when WB_SYNC_NONE and
congested.  e.g. for POSIX_FADV_DONTNEED
It is also to allow the removal of congestion tracking with minimal
changes to behaviour.

If I end up changing some dead code into different dead code, I really
don't care.  I'm not here to clean up all dead code - only the dead code
specifically related to congestion.

NeilBrown


> See commit 21b4ee7029c9, and I had a followup discussion with him
> on IRC:
> 
> <willy> dchinner: did you gather any stats on how often ->writepage was
> 	being called by pageout() before "xfs: drop ->writepage completely"
> 	was added?
> <dchinner> willy: Never saw it on XFS in 3 years in my test environment...
> <dchinner> I don't ever recall seeing the memory reclaim guards we put on
> 	->writepage in XFS ever firing - IIRC they'd been there for the best
> 	part of a decade.
> <willy> not so much the WARN_ON firing but the case where it actually calls
> 	iomap_writepage
> <dchinner> willy: I mean both - I was running with a local patch that warned
> 	on writepage for a long time, regardless of where it was called from.
> 
> I can believe things are different for a network filesystem, or maybe
> XFS does background writeback better than other filesystems, but it
> would be intriguing to be able to get rid of ->writepage altogether
> (or at least from pageout(); migrate.c may be a thornier proposition).
> 
>
Matthew Wilcox Jan. 31, 2022, 1:15 p.m. UTC | #3
On Mon, Jan 31, 2022 at 03:55:22PM +1100, NeilBrown wrote:
> On Mon, 31 Jan 2022, Matthew Wilcox wrote:
> > On Mon, Jan 31, 2022 at 03:03:53PM +1100, NeilBrown wrote:
> > >  - .writepage to return AOP_WRITEPAGE_ACTIVATE if WB_SYNC_NONE
> > >     and the flag is set.
> > 
> > Is this actually useful?  I ask because Dave Chinner believes
> > the call to ->writepage in vmscan to be essentially unused.
> 
> He would be wrong ...  unless "essentially" means "mostly" rather than
> "totally".
> swap-out to NFS results in that ->writepage call.

For writes, SWP_FS_OPS uses ->direct_IO, not ->writepage.  Confused.

> Of course swap_writepage ignores sync_mode, so this might not be
> entirely relevant.
> 
> But the main point of the patch is not to return AOP_WRITEPAGE_ACTIVATE
> to vmscan.  It is to avoid writing at all when WB_SYNC_NONE and
> congested.  e.g. for POSIX_FADV_DONTNEED
> It is also to allow the removal of congestion tracking with minimal
> changes to behaviour.
> 
> If I end up changing some dead code into different dead code, I really
> don't care.  I'm not here to clean up all dead code - only the dead code
> specifically related to congestion.
> 
> NeilBrown
> 
> 
> > See commit 21b4ee7029c9, and I had a followup discussion with him
> > on IRC:
> > 
> > <willy> dchinner: did you gather any stats on how often ->writepage was
> > 	being called by pageout() before "xfs: drop ->writepage completely"
> > 	was added?
> > <dchinner> willy: Never saw it on XFS in 3 years in my test environment...
> > <dchinner> I don't ever recall seeing the memory reclaim guards we put on
> > 	->writepage in XFS ever firing - IIRC they'd been there for the best
> > 	part of a decade.
> > <willy> not so much the WARN_ON firing but the case where it actually calls
> > 	iomap_writepage
> > <dchinner> willy: I mean both - I was running with a local patch that warned
> > 	on writepage for a long time, regardless of where it was called from.
> > 
> > I can believe things are different for a network filesystem, or maybe
> > XFS does background writeback better than other filesystems, but it
> > would be intriguing to be able to get rid of ->writepage altogether
> > (or at least from pageout(); migrate.c may be a thornier proposition).
> > 
> >
NeilBrown Jan. 31, 2022, 9:38 p.m. UTC | #4
On Tue, 01 Feb 2022, Matthew Wilcox wrote:
> On Mon, Jan 31, 2022 at 03:55:22PM +1100, NeilBrown wrote:
> > On Mon, 31 Jan 2022, Matthew Wilcox wrote:
> > > On Mon, Jan 31, 2022 at 03:03:53PM +1100, NeilBrown wrote:
> > > >  - .writepage to return AOP_WRITEPAGE_ACTIVATE if WB_SYNC_NONE
> > > >     and the flag is set.
> > > 
> > > Is this actually useful?  I ask because Dave Chinner believes
> > > the call to ->writepage in vmscan to be essentially unused.
> > 
> > He would be wrong ...  unless "essentially" means "mostly" rather than
> > "totally".
> > swap-out to NFS results in that ->writepage call.
> 
> For writes, SWP_FS_OPS uses ->direct_IO, not ->writepage.  Confused.
> 

I shouldn't have mentioned NFS - that is an irrelevant distraction.

The "call to ->writepage in vmscan" is used, at least for swap.
For swapout it is the ->writepage from swap_aops, not the ->writepage of
any filesystem.  This is swap_writepage(), and for SWP_FS_OPS that maps
to a ->direct_IO call.

Dave may well be right that the ->writepage in vmscan never calls
xfs_writepage or many others.

To get to the ->writepage of a filesystem it would need to be called
from kswapd.  You would need to have no swap configured, and 90% of
memory consumed with anon pages so that the dirty_background_ratio
of 10% didn't kick off writeback.  Then I would expect to kswapd to
write out to a filesystem before writeback would do it.

Nonetheless, without clear evidence to the contrary, I think it is
safest to add this test to the ->writepage function for any filesystem
which currently sets the bdi async congested flag.

Thanks,
NeilBrown
diff mbox series

Patch

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 987a187bd39a..b7c6721dd36d 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -417,7 +417,7 @@  static void nfs_set_page_writeback(struct page *page)
 
 	if (atomic_long_inc_return(&nfss->writeback) >
 			NFS_CONGESTION_ON_THRESH)
-		set_bdi_congested(inode_to_bdi(inode), BLK_RW_ASYNC);
+		nfss->write_congested = 1;
 }
 
 static void nfs_end_page_writeback(struct nfs_page *req)
@@ -433,7 +433,7 @@  static void nfs_end_page_writeback(struct nfs_page *req)
 
 	end_page_writeback(req->wb_page);
 	if (atomic_long_dec_return(&nfss->writeback) < NFS_CONGESTION_OFF_THRESH)
-		clear_bdi_congested(inode_to_bdi(inode), BLK_RW_ASYNC);
+		nfss->write_congested = 0;
 }
 
 /*
@@ -672,6 +672,10 @@  static int nfs_writepage_locked(struct page *page,
 	struct inode *inode = page_file_mapping(page)->host;
 	int err;
 
+	if (wbc->sync_mode == WB_SYNC_NONE &&
+	    NFS_SERVER(inode)->write_congested)
+		return AOP_WRITEPAGE_ACTIVATE;
+
 	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGE);
 	nfs_pageio_init_write(&pgio, inode, 0,
 				false, &nfs_async_write_completion_ops);
@@ -719,6 +723,10 @@  int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 	int priority = 0;
 	int err;
 
+	if (wbc->sync_mode == WB_SYNC_NONE &&
+	    NFS_SERVER(inode)->write_congested)
+		return 0;
+
 	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGES);
 
 	if (!(mntflags & NFS_MOUNT_WRITE_EAGER) || wbc->for_kupdate ||
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index ca0959e51e81..6aa2a200676a 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -138,6 +138,7 @@  struct nfs_server {
 	struct nlm_host		*nlm_host;	/* NLM client handle */
 	struct nfs_iostats __percpu *io_stats;	/* I/O statistics */
 	atomic_long_t		writeback;	/* number of writeback pages */
+	unsigned int		write_congested;/* flag set when writeback gets too high */
 	unsigned int		flags;		/* various flags */
 
 /* The following are for internal use only. Also see uapi/linux/nfs_mount.h */