Re: [PATCH 05/10] xfs: turn ag header initialisation into a table driven operation

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, May 12, 2018 at 12:03:31PM +1000, Dave Chinner wrote:
> On Fri, May 11, 2018 at 05:55:35PM -0700, Darrick J. Wong wrote:
> > On Sat, May 12, 2018 at 08:51:02AM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > There's still more cookie cutter code in setting up each AG header.
> > > Separate all the variables into a simple structure and iterate a
> > > table of header definitions to initialise everything.
> > > 
> > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >  fs/xfs/xfs_fsops.c | 158 +++++++++++++++++++--------------------------
> > >  1 file changed, 66 insertions(+), 92 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > index 4dbb7b41aeca..263f9b5da991 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
> > > @@ -283,12 +283,13 @@ xfs_agiblock_init(
> > >  		agi->agi_unlinked[bucket] = cpu_to_be32(NULLAGINO);
> > >  }
> > >  
> > > +typedef void (*aghdr_init_work_f)(struct xfs_mount *mp, struct xfs_buf *bp,
> > > +				  struct aghdr_init_data *id);
> > >  static int
> > >  xfs_growfs_init_aghdr(
> > >  	struct xfs_mount	*mp,
> > >  	struct aghdr_init_data	*id,
> > > -	void			(*work)(struct xfs_mount *, struct xfs_buf *,
> > > -					struct aghdr_init_data *),
> > > +	aghdr_init_work_f	work,
> > >  	const struct xfs_buf_ops *ops)
> > >  
> > >  {
> > > @@ -305,6 +306,16 @@ xfs_growfs_init_aghdr(
> > >  	return 0;
> > >  }
> > >  
> > > +struct xfs_aghdr_grow_data {
> > > +	xfs_daddr_t		daddr;
> > > +	size_t			numblks;
> > > +	const struct xfs_buf_ops *ops;
> > > +	aghdr_init_work_f	work;
> > > +	xfs_btnum_t		type;
> > > +	int			numrecs;
> > > +	bool			need_init;
> > > +};
> > > +
> > >  /*
> > >   * Write new AG headers to disk. Non-transactional, but written
> > >   * synchronously so they are completed prior to the growfs transaction
> > > @@ -316,104 +327,67 @@ xfs_grow_ag_headers(
> > >  	struct aghdr_init_data	*id)
> > >  
> > >  {
> > > +	struct xfs_aghdr_grow_data aghdr_data[] = {
> > > +		/* AGF */
> > > +		{ XFS_AG_DADDR(mp, id->agno, XFS_AGF_DADDR(mp)),
> > > +		  XFS_FSS_TO_BB(mp, 1), &xfs_agf_buf_ops,
> > > +		  &xfs_agfblock_init, 0, 0, true },
> > 
> > Any chance I could get you to convert these to something associative
> > and easier to check? e.g.
> > 
> > 	/* AGF */
> > 	{
> > 		.daddr		= XFS_AG_DADDR(mp, id->agno, XFS_AGF_DADDR(mp)),
> > 		.numblks	= XFS_FSS_TO_BB(mp, 1),
> > 		.ops		= &xfs_agf_buf_ops,
> > 		.work		= &xfs_agfblock_init,
> > 		.type		= 0,
> > 		.numrecs	= 0,
> > 		.need_init	= true,
> > 	},
> 
> I can, if you don't care it will take 100 lines of code instead of
> 30.

I don't. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html