Re: [PATCH 06/11] xfs: deferred inode inactivation

From: Christoph Hellwig <hch@infradead.org>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 06/11] xfs: deferred inode inactivation
Date: Wed, 24 Mar 2021 17:53:11 +0000	[thread overview]
Message-ID: <20210324175311.GA2773443@infradead.org> (raw)
In-Reply-To: <20210323040037.GI22100@magnolia>

On Mon, Mar 22, 2021 at 09:00:37PM -0700, Darrick J. Wong wrote:
> Hmm, maybe this could maintain an approxiate liar counter and only flush
> inactivation when the liar counter would cause us to be off by more than
> some configurable amount?  The fstests that care about free space
> accounting are not going to be happy since they are measured with very
> tight tolerances.

Yes, I think some kind of fuzzy logic instead of the heavy weight flush
on supposedly light weight operations.

> > static void
> > xfs_inode_clear_tag(
> > 	struct xfs_perag	*pag,
> > 	xfs_ino_t		ino,
> > 	int			tag)
> > {
> > 	struct xfs_mount	*mp = pag->pag_mount;
> > 
> > 	lockdep_assert_held(&pag->pag_ici_lock);
> > 	radix_tree_tag_clear(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, ino),
> > 				tag);
> > 	switch(tag) {
> > 	case XFS_ICI_INACTIVE_TAG:
> > 		if (--pag->pag_ici_inactive)
> > 			return;
> > 		break;
> > 	case XFS_ICI_RECLAIM_TAG:
> > 		if (--pag->pag_ici_reclaim)
> > 			return;
> > 		break;
> > 	default:
> > 		ASSERT(0);
> > 		return;
> > 	}
> > 
> > 	spin_lock(&mp->m_perag_lock);
> > 	radix_tree_tag_clear(&mp->m_perag_tree, pag->pag_agno, tag);
> > 	spin_unlock(&mp->m_perag_lock);
> > }
> > 
> > As a followup patch? The set tag case looks similarly easy to make
> > generic...
> 
> Yeah.  At this point I might as well just clean all of this up for the
> next revision of this series, because as I said earlier I had thought
> that you were still working on a second rework of reclaim.  Now that I
> know you're not, I'll hack away at this twisty pile too.

If the separate tags aren't going to disappear entirely: it would be nice
to move the counters (or any other duplicated variable) into an array
index by the tax, which would clean the above and similar code even more.

> We don't actually stop background gc transactions or other internal
> updates on readonly filesystems -- the ro part means only that we don't
> let /userspace/ change anything directly.  If you open a file readonly,
> unlink it, freeze the fs, and close the file, we'll still free it.

Note that there are two different read-only concepts in Linux:

 1) the read-only mount, as reflected in the vfsmount.  For this your
    description above is spot-on
 2) the read-only superblock, as indicated by the sb flag.  This is
    usually due to an read-only block device, and we must not write
    anything to the device, as that typically will lead to an I/O error.