diff mbox series

[2/3] xfs: allow scrubbers to pause background reclaim

Message ID 155546521227.176278.16067700926705972688.stgit@magnolia (mailing list archive)
State Accepted, archived
Headers show
Series xfs: scrub filesystem summary counters | expand

Commit Message

Darrick J. Wong April 17, 2019, 1:40 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

The forthcoming summary counter patch races with regular filesystem
activity to compute rough expected values for the counters.  This design
was chosen to avoid having to freeze the entire filesystem to check the
counters, but while that's running we'd prefer to minimize background
reclamation activity to reduce the perturbations to the incore free
block count.  Therefore, provide a way for scrubbers to disable
background posteof and cowblock reclamation.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/common.c |   18 ++++++++++++++++++
 fs/xfs/scrub/common.h |    2 ++
 fs/xfs/scrub/scrub.c  |    2 ++
 fs/xfs/scrub/scrub.h  |    1 +
 4 files changed, 23 insertions(+)

Comments

Dave Chinner April 17, 2019, 9:52 p.m. UTC | #1 
On Tue, Apr 16, 2019 at 06:40:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The forthcoming summary counter patch races with regular filesystem
> activity to compute rough expected values for the counters.  This design
> was chosen to avoid having to freeze the entire filesystem to check the
> counters, but while that's running we'd prefer to minimize background
> reclamation activity to reduce the perturbations to the incore free
> block count.  Therefore, provide a way for scrubbers to disable
> background posteof and cowblock reclamation.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/common.c |   18 ++++++++++++++++++
>  fs/xfs/scrub/common.h |    2 ++
>  fs/xfs/scrub/scrub.c  |    2 ++
>  fs/xfs/scrub/scrub.h  |    1 +
>  4 files changed, 23 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index 7076d5c98151..a406a22a734f 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -894,3 +894,21 @@ xchk_ilock_inverted(
>  	}
>  	return -EDEADLOCK;
>  }
> +
> +/* Pause background reclamation and inactivation. */
> +void
> +xchk_disable_reclaim(
> +	struct xfs_scrub	*sc)
> +{
> +	sc->flags |= XCHK_RECLAIM_DISABLED;
> +	xfs_icache_disable_reclaim(sc->mp);
> +}

Hmmm. Poorly named function.  "reclaim" in the context of
xfs_icache.c means inode cache reclaim...

We're not actually disabling inode reclaim here, we
are disabling background block reaping.

Can we change this all to be named more appropriately (including the
icache.c functions? Then it is all fine because I don't look at
it and think "that'll break under memory pressure"....

Cheers,

Dave.
Darrick J. Wong April 17, 2019, 10:29 p.m. UTC | #2 
On Thu, Apr 18, 2019 at 07:52:28AM +1000, Dave Chinner wrote:
> On Tue, Apr 16, 2019 at 06:40:12PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The forthcoming summary counter patch races with regular filesystem
> > activity to compute rough expected values for the counters.  This design
> > was chosen to avoid having to freeze the entire filesystem to check the
> > counters, but while that's running we'd prefer to minimize background
> > reclamation activity to reduce the perturbations to the incore free
> > block count.  Therefore, provide a way for scrubbers to disable
> > background posteof and cowblock reclamation.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/common.c |   18 ++++++++++++++++++
> >  fs/xfs/scrub/common.h |    2 ++
> >  fs/xfs/scrub/scrub.c  |    2 ++
> >  fs/xfs/scrub/scrub.h  |    1 +
> >  4 files changed, 23 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> > index 7076d5c98151..a406a22a734f 100644
> > --- a/fs/xfs/scrub/common.c
> > +++ b/fs/xfs/scrub/common.c
> > @@ -894,3 +894,21 @@ xchk_ilock_inverted(
> >  	}
> >  	return -EDEADLOCK;
> >  }
> > +
> > +/* Pause background reclamation and inactivation. */
> > +void
> > +xchk_disable_reclaim(
> > +	struct xfs_scrub	*sc)
> > +{
> > +	sc->flags |= XCHK_RECLAIM_DISABLED;
> > +	xfs_icache_disable_reclaim(sc->mp);
> > +}
> 
> Hmmm. Poorly named function.  "reclaim" in the context of
> xfs_icache.c means inode cache reclaim...
> 
> We're not actually disabling inode reclaim here, we
> are disabling background block reaping.
> 
> Can we change this all to be named more appropriately (including the
> icache.c functions? Then it is all fine because I don't look at
> it and think "that'll break under memory pressure"....

Ah, reaping!  That's the word I was looking for. :(

xfs_icache_disable_speculative_reaping
xchk_disable_speculative_reaping

Sort of a long name but eh not that many people will use it.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner April 17, 2019, 10:45 p.m. UTC | #3 
On Wed, Apr 17, 2019 at 03:29:53PM -0700, Darrick J. Wong wrote:
> On Thu, Apr 18, 2019 at 07:52:28AM +1000, Dave Chinner wrote:
> > On Tue, Apr 16, 2019 at 06:40:12PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > The forthcoming summary counter patch races with regular filesystem
> > > activity to compute rough expected values for the counters.  This design
> > > was chosen to avoid having to freeze the entire filesystem to check the
> > > counters, but while that's running we'd prefer to minimize background
> > > reclamation activity to reduce the perturbations to the incore free
> > > block count.  Therefore, provide a way for scrubbers to disable
> > > background posteof and cowblock reclamation.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/scrub/common.c |   18 ++++++++++++++++++
> > >  fs/xfs/scrub/common.h |    2 ++
> > >  fs/xfs/scrub/scrub.c  |    2 ++
> > >  fs/xfs/scrub/scrub.h  |    1 +
> > >  4 files changed, 23 insertions(+)
> > > 
> > > 
> > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> > > index 7076d5c98151..a406a22a734f 100644
> > > --- a/fs/xfs/scrub/common.c
> > > +++ b/fs/xfs/scrub/common.c
> > > @@ -894,3 +894,21 @@ xchk_ilock_inverted(
> > >  	}
> > >  	return -EDEADLOCK;
> > >  }
> > > +
> > > +/* Pause background reclamation and inactivation. */
> > > +void
> > > +xchk_disable_reclaim(
> > > +	struct xfs_scrub	*sc)
> > > +{
> > > +	sc->flags |= XCHK_RECLAIM_DISABLED;
> > > +	xfs_icache_disable_reclaim(sc->mp);
> > > +}
> > 
> > Hmmm. Poorly named function.  "reclaim" in the context of
> > xfs_icache.c means inode cache reclaim...
> > 
> > We're not actually disabling inode reclaim here, we
> > are disabling background block reaping.
> > 
> > Can we change this all to be named more appropriately (including the
> > icache.c functions? Then it is all fine because I don't look at
> > it and think "that'll break under memory pressure"....
> 
> Ah, reaping!  That's the word I was looking for. :(
> 
> xfs_icache_disable_speculative_reaping
> xchk_disable_speculative_reaping

disable_block_reaping?

but, really, purple or pink at this point...

Cheers,

Dave.
diff mbox series

Patch

diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 7076d5c98151..a406a22a734f 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -894,3 +894,21 @@  xchk_ilock_inverted(
 	}
 	return -EDEADLOCK;
 }
+
+/* Pause background reclamation and inactivation. */
+void
+xchk_disable_reclaim(
+	struct xfs_scrub	*sc)
+{
+	sc->flags |= XCHK_RECLAIM_DISABLED;
+	xfs_icache_disable_reclaim(sc->mp);
+}
+
+/* Unpause background reclamation and inactivation. */
+void
+xchk_enable_reclaim(
+	struct xfs_scrub	*sc)
+{
+	xfs_icache_enable_reclaim(sc->mp);
+	sc->flags &= ~XCHK_RECLAIM_DISABLED;
+}
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index e26a430bd466..2288f45a5606 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -137,5 +137,7 @@  static inline bool xchk_skip_xref(struct xfs_scrub_metadata *sm)
 
 int xchk_metadata_inode_forks(struct xfs_scrub *sc);
 int xchk_ilock_inverted(struct xfs_inode *ip, uint lock_mode);
+void xchk_disable_reclaim(struct xfs_scrub *sc);
+void xchk_enable_reclaim(struct xfs_scrub *sc);
 
 #endif	/* __XFS_SCRUB_COMMON_H__ */
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 93b0f075a4d3..421c22a0bf39 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -187,6 +187,8 @@  xchk_teardown(
 			xfs_irele(sc->ip);
 		sc->ip = NULL;
 	}
+	if (sc->flags & XCHK_RECLAIM_DISABLED)
+		xchk_enable_reclaim(sc);
 	if (sc->flags & XCHK_HAS_QUOTAOFFLOCK) {
 		mutex_unlock(&sc->mp->m_quotainfo->qi_quotaofflock);
 		sc->flags &= ~XCHK_HAS_QUOTAOFFLOCK;
diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index 1b280f8f185a..1f6de7bbb9f5 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -80,6 +80,7 @@  struct xfs_scrub {
 /* XCHK state flags grow up from zero, XREP state flags grown down from 2^31 */
 #define XCHK_TRY_HARDER		(1 << 0)  /* can't get resources, try again */
 #define XCHK_HAS_QUOTAOFFLOCK	(1 << 1)  /* we hold the quotaoff lock */
+#define XCHK_RECLAIM_DISABLED	(1 << 2)  /* background reclaim is paused */
 #define XREP_ALREADY_FIXED	(1 << 31) /* checking our repair work */
 
 /* Metadata scrubbers */