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

[Dave Chinner <david@fromorbit.com>]

On Mon, Dec 07, 2020 at 10:19:48PM +0800, Gao Xiang wrote:
> 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.
> 
> (See below...)
> 
> > 
> > >  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".
> 
> Ok, yet pino is somewhat common, as I saw it in f2fs and jffs2 before.
> I know you mean 'Hungarian naming conventions'.
> 
> If you don't like pino. How about parent_ino? since parent_inode occurs me
> about "struct inode *" or something like this (a pointer around some inode),
> rather than an inode number.
> 
> > 
> > > +	/* 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.
> 
> Okay, the name was from Dave to unify the prefix (namespace)... I think it'd
> be better to get Dave's idea about this as well. As of me, I'm fine with
> either way.

I'm fine with that.

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