Re: [PATCH 02/22] xfs: prepare for moving perag definitions and support to libxfs

[Dave Chinner <david@fromorbit.com>]

On Mon, May 10, 2021 at 08:53:28AM -0400, Brian Foster wrote:
> On Thu, May 06, 2021 at 05:20:34PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The perag structures really need to be defined with the rest of the
> > AG support infrastructure. The struct xfs_perag and init/teardown
> > has been placed in xfs_mount.[ch] because there are differences in
> > the structure between kernel and userspace. Mainly that userspace
> > doesn't have a lot of the internal stuff that the kernel has for
> > caches and discard and other such structures.
> > 
> > However, it makes more sense to move this to libxfs than to keep
> > this separation because we are now moving to use struct perags
> > everywhere in the code instead of passing raw agnumber_t values
> > about. Hence we shoudl really move the support infrastructure to
> > libxfs/xfs_ag.[ch].
> > 
> > To do this without breaking userspace, first we need to rearrange
> > the structures and code so that all the kernel specific code is
> > located together. This makes it simple for userspace to ifdef out
> > the all the parts it does not need, minimising the code differences
> > between kernel and userspace. The next commit will do the move...
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_mount.c | 50 ++++++++++++++++++++++++++--------------------
> >  fs/xfs/xfs_mount.h | 19 +++++++++---------
> >  2 files changed, 38 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 21c630dde476..2e6d42014346 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> ...
> > @@ -229,13 +220,27 @@ xfs_initialize_perag(
> >  		}
> >  		spin_unlock(&mp->m_perag_lock);
> >  		radix_tree_preload_end();
> > -		/* first new pag is fully initialized */
> > -		if (first_initialised == NULLAGNUMBER)
> > -			first_initialised = index;
> > +
> > +		spin_lock_init(&pag->pag_ici_lock);
> > +		spin_lock_init(&pag->pagb_lock);
> > +		spin_lock_init(&pag->pag_state_lock);
> > +		INIT_DELAYED_WORK(&pag->pag_blockgc_work, xfs_blockgc_worker);
> > +		INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
> > +		init_waitqueue_head(&pag->pagb_wait);
> > +		pag->pagb_count = 0;
> > +		pag->pagb_tree = RB_ROOT;
> > +
> > +		error = xfs_buf_hash_init(pag);
> > +		if (error)
> > +			goto out_free_pag;
> > +
> 
> There's error handling code earlier up in this function that still lands
> in out_hash_destroy, which is now before we get to the _hash_init()
> call.
> 
> >  		error = xfs_iunlink_init(pag);
> >  		if (error)
> >  			goto out_hash_destroy;
> > -		spin_lock_init(&pag->pag_state_lock);
> > +
> > +		/* first new pag is fully initialized */
> > +		if (first_initialised == NULLAGNUMBER)
> > +			first_initialised = index;
> >  	}
> >  
> >  	index = xfs_set_inode_alloc(mp, agcount);
> > @@ -249,6 +254,7 @@ xfs_initialize_perag(
> >  out_hash_destroy:
> >  	xfs_buf_hash_destroy(pag);
> >  out_free_pag:
> > +	pag = radix_tree_delete(&mp->m_perag_tree, index);
> 
> Now if we get here with an allocated pag that hasn't been inserted to
> the tree, I suspect this call would assign pag = NULL..

Yup, error handling was fubar. Fixed now.

-- 
Dave Chinner
david@fromorbit.com