Re: [PATCH v3 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()

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

On Mon, Dec 07, 2020 at 02:49:41PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 08:15:30AM +0800, Gao Xiang wrote:
> >  /*
> > + * Initialise a newly allocated inode and return the in-core inode to the
> > + * caller locked exclusively.
> >   */
> > -static int
> > -xfs_ialloc(
> > -	xfs_trans_t	*tp,
> > -	xfs_inode_t	*pip,
> > -	umode_t		mode,
> > -	xfs_nlink_t	nlink,
> > -	dev_t		rdev,
> > -	prid_t		prid,
> > -	xfs_buf_t	**ialloc_context,
> > -	xfs_inode_t	**ipp)
> > +static struct xfs_inode *
> > +xfs_dir_ialloc_init(
> 
> This is boderline bikeshedding, but I would just call this
> xfs_init_new_inode.
> 
> >  int
> >  xfs_dir_ialloc(
> > @@ -954,83 +908,59 @@ xfs_dir_ialloc(
> >  	xfs_inode_t	**ipp)		/* pointer to inode; it will be
> >  					   locked. */
> >  {
> >  	xfs_inode_t	*ip;
> >  	xfs_buf_t	*ialloc_context = NULL;
> > +	xfs_ino_t	pino = dp ? dp->i_ino : 0;
> 
> Maybe spell out parent_inode?  pino reminds of some of the weird Windows
> code that start all variable names for pointers with a "p".
> 
> > +	/* Initialise the newly allocated inode. */
> > +	ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid);
> > +	if (IS_ERR(ip))
> > +		return PTR_ERR(ip);
> > +	*ipp = ip;
> >  	return 0;
> 
> I wonder if we should just return the inode by reference from
> xfs_dir_ialloc_init as well, as that nicely fits the calling convention
> in the caller, i.e. this could become
> 
> 	return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid, ipp);
> 
> Note with the right naming we don't really need the comment either,
> as the function name should explain everything.

/me notes that some day he will get around to a formal posting of his
giant inode allocation cleanup series that will wrap these parameters
into a struct so we don't have to pass 8 arguments around, and fix the
inode flag inheritance inconsistencies between userspace and kernel...

--D