On Wed, Mar 24, 2021 at 05:53:11PM +0000, Christoph Hellwig wrote:
> 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.
Any suggestions? I'll try adding a ratelimit to see if that shuts up
fstests while preventing userspace from pounding too hard on
inactivation.
> > > 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.
Ok done.
I refactored xfs_perag_{clear,set}_reclaim_tag into a generic helper
that sets an ICI tag on the inode radix tree and the perag radix tree.
This cleaned up a bunch of redundant code, and enabled me to trim down
the inactivation patch quite a bit. Now each function that wants to set
inode flags does so directly (after taking the locks) and calls the ICI
helper to deal with the radix trees.
Also, refactoring reclaim to use xfs_inode_walk was pretty simple, and I
even integrated (rather heavily modified) code from the "void *args" ->
"eofb" and the "get rid of iter_flags" patches you posted.
> > 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.
<nod> I /think/ we handle this properly, but it's late...
--D
