Re: [PATCH 18/22] xfs: collapse AG selection for inode allocation

[Dave Chinner <david@fromorbit.com>]

On Wed, May 12, 2021 at 04:11:14PM -0700, Darrick J. Wong wrote:
> On Thu, May 06, 2021 at 05:20:50PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > xfs_dialloc_select_ag() does a lot of repetitive work. It first
> > calls xfs_ialloc_ag_select() to select the AG to start allocation
> > attempts in, which can do up to two entire loops across the perags
> > that inodes can be allocated in. This is simply checking if there is
> > spce available to allocate inodes in an AG, and it returns when it
> > finds the first candidate AG.
> > 
> > xfs_dialloc_select_ag() then does it's own iterative walk across
> > all the perags locking the AGIs and trying to allocate inodes from
> > the locked AG. It also doesn't limit the search to mp->m_maxagi,
> > so it will walk all AGs whether they can allocate inodes or not.
> > 
> > Hence if we are really low on inodes, we could do almost 3 entire
> > walks across the whole perag range before we find an allocation
> > group we can allocate inodes in or report ENOSPC.
> > 
> > Because xfs_ialloc_ag_select() returns on the first candidate AG it
> > finds, we can simply do these checks directly in
> > xfs_dialloc_select_ag() before we lock and try to allocate inodes.
> > This reduces the inode allocation pass down to 2 perag sweeps at
> > most - one for aligned inode cluster allocation and if we can't
> > allocate full, aligned inode clusters anywhere we'll do another pass
> > trying to do sparse inode cluster allocation.
> > 
> > This also removes a big chunk of duplicate code.
> 
> Soooooo... we did an AG walk to pick the optimal starting point of an AG
> walk?  Heh.

yup.

> > @@ -1734,16 +1616,23 @@ xfs_dialloc_select_ag(
> >  	struct xfs_perag	*pag;
> >  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> >  	bool			okalloc = true;
> > +	int			needspace;
> > +	int			flags;
> >  
> >  	*IO_agbp = NULL;
> >  
> >  	/*
> > -	 * We do not have an agbp, so select an initial allocation
> > -	 * group for inode allocation.
> > +	 * Files of these types need at least one block if length > 0
> > +	 * (and they won't fit in the inode, but that's hard to figure out).
> 
> Uh, what is length here?  Seeing as most directories and symlinks
> probably end up in local format, is this needspace computation really
> true?

Ah, that's the exact text from the comment in
xfs_ialloc_ag_select() above the needspace calculation - I didn't
actually change it at all. :/

> I guess it doesn't hurt to be cautious and assume that directories can
> expand and that people are aggressively symlinking.  But maybe this
> comment could be rephrased to something like:
> 
> 	/*
> 	 * Directories, symlinks, and regular files frequently allocate
> 	 * at least one block, so factor that potential expansion when
> 	 * we examine whether an AG has enough space for file creation.
> 	 */
> 	needspace = S_ISDIR(mode)...;
> 
> With that clarified,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Changed.

-Dave.
-- 
Dave Chinner
david@fromorbit.com