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@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > > The patch looks ok, so you can add: > > > > Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx> > > > > > > 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
