diff mbox series

[2/3] xfs: drop IDONTCACHE on inodes when we mark them sick

Message ID 162300205695.1202529.8468586379242468573.stgit@locust (mailing list archive)
State Accepted
Headers show
Series xfs: preserve inode health reports for longer | expand

Commit Message

Darrick J. Wong June 6, 2021, 5:54 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

When we decide to mark an inode sick, clear the DONTCACHE flag so that
the incore inode will be kept around until memory pressure forces it out
of memory.  This increases the chances that the sick status will be
caught by someone compiling a health report later on.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_health.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Dave Chinner June 7, 2021, 1:40 a.m. UTC | #1 
On Sun, Jun 06, 2021 at 10:54:17AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When we decide to mark an inode sick, clear the DONTCACHE flag so that
> the incore inode will be kept around until memory pressure forces it out
> of memory.  This increases the chances that the sick status will be
> caught by someone compiling a health report later on.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_health.c |    9 +++++++++
>  1 file changed, 9 insertions(+)

LGTM.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Carlos Maiolino June 8, 2021, 2:59 p.m. UTC | #2 
Hi,

On Sun, Jun 06, 2021 at 10:54:17AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When we decide to mark an inode sick, clear the DONTCACHE flag so that
> the incore inode will be kept around until memory pressure forces it out
> of memory.  This increases the chances that the sick status will be
> caught by someone compiling a health report later on.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

The patch looks ok, so you can add:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


Now, I have a probably dumb question about this.

by removing the I_DONTCACHE flag, as you said, we are increasing the chances
that the sick status will be caught, so, in either case, it seems not reliable.
So, my dumb question is, is there reason having these inodes around will benefit
us somehow? I haven't read the whole code, but I assume, it can be used as a
fast path while scrubbing the FS?

Cheers.

> ---
>  fs/xfs/xfs_health.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 
> diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
> index 8e0cb05a7142..806be8a93ea3 100644
> --- a/fs/xfs/xfs_health.c
> +++ b/fs/xfs/xfs_health.c
> @@ -231,6 +231,15 @@ xfs_inode_mark_sick(
>  	ip->i_sick |= mask;
>  	ip->i_checked |= mask;
>  	spin_unlock(&ip->i_flags_lock);
> +
> +	/*
> +	 * Keep this inode around so we don't lose the sickness report.  Scrub
> +	 * grabs inodes with DONTCACHE assuming that most inode are ok, which
> +	 * is not the case here.
> +	 */
> +	spin_lock(&VFS_I(ip)->i_lock);
> +	VFS_I(ip)->i_state &= ~I_DONTCACHE;
> +	spin_unlock(&VFS_I(ip)->i_lock);
>  }
>  
>  /* Mark parts of an inode healed. */
>
Darrick J. Wong June 8, 2021, 3:21 p.m. UTC | #3 
On Tue, Jun 08, 2021 at 04:59:48PM +0200, Carlos Maiolino wrote:
> Hi,
> 
> On Sun, Jun 06, 2021 at 10:54:17AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > When we decide to mark an inode sick, clear the DONTCACHE flag so that
> > the incore inode will be kept around until memory pressure forces it out
> > of memory.  This increases the chances that the sick status will be
> > caught by someone compiling a health report later on.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> The patch looks ok, so you can add:
> 
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> 
> Now, I have a probably dumb question about this.
> 
> by removing the I_DONTCACHE flag, as you said, we are increasing the chances
> that the sick status will be caught, so, in either case, it seems not reliable.
> So, my dumb question is, is there reason having these inodes around will benefit
> us somehow? I haven't read the whole code, but I assume, it can be used as a
> fast path while scrubbing the FS?

Two answers to your question: In the short term, preserving the incore
inode means that a subsequent reporting run (xfs_spaceman -c 'health')
is more likely to pick up the sickness report.

In the longer term, I intend to re-enable reclamation of sick inodes
by aggregating the per-inode sick bit in the per-AG health status so
that reporting won't be interrupted by memory demand:

[1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=indirect-health-reporting

(I haven't rebased that part in quite a while though.)

--D

> 
> Cheers.
> 
> > ---
> >  fs/xfs/xfs_health.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
> > index 8e0cb05a7142..806be8a93ea3 100644
> > --- a/fs/xfs/xfs_health.c
> > +++ b/fs/xfs/xfs_health.c
> > @@ -231,6 +231,15 @@ xfs_inode_mark_sick(
> >  	ip->i_sick |= mask;
> >  	ip->i_checked |= mask;
> >  	spin_unlock(&ip->i_flags_lock);
> > +
> > +	/*
> > +	 * Keep this inode around so we don't lose the sickness report.  Scrub
> > +	 * grabs inodes with DONTCACHE assuming that most inode are ok, which
> > +	 * is not the case here.
> > +	 */
> > +	spin_lock(&VFS_I(ip)->i_lock);
> > +	VFS_I(ip)->i_state &= ~I_DONTCACHE;
> > +	spin_unlock(&VFS_I(ip)->i_lock);
> >  }
> >  
> >  /* Mark parts of an inode healed. */
> > 
> 
> -- 
> Carlos
>
Carlos Maiolino June 9, 2021, 8:55 a.m. UTC | #4 
On Tue, Jun 08, 2021 at 08:21:46AM -0700, Darrick J. Wong wrote:
> On Tue, Jun 08, 2021 at 04:59:48PM +0200, Carlos Maiolino wrote:
> > Hi,
> > 
> > On Sun, Jun 06, 2021 at 10:54:17AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > When we decide to mark an inode sick, clear the DONTCACHE flag so that
> > > the incore inode will be kept around until memory pressure forces it out
> > > of memory.  This increases the chances that the sick status will be
> > > caught by someone compiling a health report later on.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > 
> > The patch looks ok, so you can add:
> > 
> > Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> > 
> > 
> > Now, I have a probably dumb question about this.
> > 
> > by removing the I_DONTCACHE flag, as you said, we are increasing the chances
> > that the sick status will be caught, so, in either case, it seems not reliable.
> > So, my dumb question is, is there reason having these inodes around will benefit
> > us somehow? I haven't read the whole code, but I assume, it can be used as a
> > fast path while scrubbing the FS?
> 
> Two answers to your question: In the short term, preserving the incore
> inode means that a subsequent reporting run (xfs_spaceman -c 'health')
> is more likely to pick up the sickness report.
> 
> In the longer term, I intend to re-enable reclamation of sick inodes
> by aggregating the per-inode sick bit in the per-AG health status so
> that reporting won't be interrupted by memory demand:
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=indirect-health-reporting
> 
> (I haven't rebased that part in quite a while though.)

Thanks!

> 
> --D
> 
> > 
> > Cheers.
> > 
> > > ---
> > >  fs/xfs/xfs_health.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > 
> > > diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
> > > index 8e0cb05a7142..806be8a93ea3 100644
> > > --- a/fs/xfs/xfs_health.c
> > > +++ b/fs/xfs/xfs_health.c
> > > @@ -231,6 +231,15 @@ xfs_inode_mark_sick(
> > >  	ip->i_sick |= mask;
> > >  	ip->i_checked |= mask;
> > >  	spin_unlock(&ip->i_flags_lock);
> > > +
> > > +	/*
> > > +	 * Keep this inode around so we don't lose the sickness report.  Scrub
> > > +	 * grabs inodes with DONTCACHE assuming that most inode are ok, which
> > > +	 * is not the case here.
> > > +	 */
> > > +	spin_lock(&VFS_I(ip)->i_lock);
> > > +	VFS_I(ip)->i_state &= ~I_DONTCACHE;
> > > +	spin_unlock(&VFS_I(ip)->i_lock);
> > >  }
> > >  
> > >  /* Mark parts of an inode healed. */
> > > 
> > 
> > -- 
> > Carlos
> > 
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
index 8e0cb05a7142..806be8a93ea3 100644
--- a/fs/xfs/xfs_health.c
+++ b/fs/xfs/xfs_health.c
@@ -231,6 +231,15 @@  xfs_inode_mark_sick(
 	ip->i_sick |= mask;
 	ip->i_checked |= mask;
 	spin_unlock(&ip->i_flags_lock);
+
+	/*
+	 * Keep this inode around so we don't lose the sickness report.  Scrub
+	 * grabs inodes with DONTCACHE assuming that most inode are ok, which
+	 * is not the case here.
+	 */
+	spin_lock(&VFS_I(ip)->i_lock);
+	VFS_I(ip)->i_state &= ~I_DONTCACHE;
+	spin_unlock(&VFS_I(ip)->i_lock);
 }
 
 /* Mark parts of an inode healed. */