Re: [PATCH 05/14] xfs: per-cpu deferred inode inactivation queues

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Dave Chinner <dchinner@redhat.com>,
	linux-xfs@vger.kernel.org, hch@infradead.org
Subject: Re: [PATCH 05/14] xfs: per-cpu deferred inode inactivation queues
Date: Sun, 8 Aug 2021 07:49:00 +1000	[thread overview]
Message-ID: <20210807214900.GB3657114@dread.disaster.area> (raw)
In-Reply-To: <20210807002104.GB3601443@magnolia>

On Fri, Aug 06, 2021 at 05:21:04PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 04, 2021 at 07:06:50PM -0700, Darrick J. Wong wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> 
> <megasnip> A couple of minor changes that aren't worth reposting the
> entire series:
> 
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index b9214733d0c3..fedfa40e3cd6 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> 
> <snip>
> 
> > @@ -1767,30 +1801,276 @@ xfs_inode_mark_reclaimable(
> >  		ASSERT(0);
> >  	}
> >  
> > +	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
> > +	spin_lock(&pag->pag_ici_lock);
> > +	spin_lock(&ip->i_flags_lock);
> > +
> > +	trace_xfs_inode_set_reclaimable(ip);
> > +	ip->i_flags &= ~(XFS_NEED_INACTIVE | XFS_INACTIVATING);
> > +	ip->i_flags |= XFS_IRECLAIMABLE;
> > +	xfs_perag_set_inode_tag(pag, XFS_INO_TO_AGINO(mp, ip->i_ino),
> > +			XFS_ICI_RECLAIM_TAG);
> > +
> > +	spin_unlock(&ip->i_flags_lock);
> > +	spin_unlock(&pag->pag_ici_lock);
> > +	xfs_perag_put(pag);
> > +}
> > +
> > +/*
> > + * Free all speculative preallocations and possibly even the inode itself.
> > + * This is the last chance to make changes to an otherwise unreferenced file
> > + * before incore reclamation happens.
> > + */
> > +static void
> > +xfs_inodegc_inactivate(
> > +	struct xfs_inode	*ip)
> > +{
> > +	struct xfs_mount        *mp = ip->i_mount;
> > +
> > +	/*
> > +	* Inactivation isn't supposed to run when the fs is frozen because
> > +	* we don't want kernel threads to block on transaction allocation.
> > +	*/
> > +	ASSERT(mp->m_super->s_writers.frozen < SB_FREEZE_FS);
> > +
> 
> I solved the problems Dave was complaining about (g/390, x/517) by
> removing this ASSERT.
> 
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index 19260291ff8b..bd8abb50b33a 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -157,6 +157,48 @@ DEFINE_PERAG_REF_EVENT(xfs_perag_put);
> >  DEFINE_PERAG_REF_EVENT(xfs_perag_set_inode_tag);
> >  DEFINE_PERAG_REF_EVENT(xfs_perag_clear_inode_tag);
> >  
> > +#define XFS_STATE_FLAGS \
> > +	{ (1UL << XFS_STATE_INODEGC_ENABLED),		"inodegc" }
> 
> I've also changed the name of this to XFS_OPSTATE_STRINGS because we use
> _STRINGS everywhere else in this file.

FWIW, can we define this with the definition of the OPSTATE
variables in xfs_mount.h? THat makes it much easier to keep up to
date when we add new flags because it's obvious that there are
tracing flags that also need to be updated when we add a new state
flag...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com