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

[Carlos Maiolino <cmaiolino@redhat.com>]

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
> > 
> 

-- 
Carlos