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

[Gao Xiang <hsiangkao@redhat.com>]

Hi Christoph,

On Wed, Dec 09, 2020 at 08:52:46AM +0100, Christoph Hellwig wrote:
> > +	/* Initialise the newly allocated inode. */
> > +	return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
> 
> IMHO this comment is not overly helpful..

That was inherited from old version, I could get rid of in the next version....

> 
> > +	if (IS_ERR(ip)) {
> > +		error = PTR_ERR(ip);
> > +		ip = NULL;
> >  		goto out_trans_cancel;
> > +	}
> 
> And the calling convention with the ERR_PTR return does not seem to
> fit the call chain to well.  But those are minor details, so:

Yeah, I also think so, since I found many error exit paths
rely on ip == NULL.

> 
> >  STATIC int
> >  xfs_qm_qino_alloc(
> > -	xfs_mount_t	*mp,
> > -	xfs_inode_t	**ip,
> > -	uint		flags)
> > +	struct xfs_mount	*mp,
> > +	struct xfs_inode	**ipp,
> > +	unsigned int		flags)
> >  {
> >  	xfs_trans_t	*tp;
> >  	int		error;
> >  	bool		need_alloc = true;
> 
> Why do you reindent and de-typdefify the arguments, but not the local
> variables?

since I renamed *ip to *ipp (it seems that should be *ipp here), so I need to
modify this line
-	xfs_inode_t	**ip,

so I fixed the typedef, but it introduced an intent issue, so I fixed the
whole input argument block for better coding style.

That is related to the argument only, so I didn't fix the local argument
though (since I didn't touch them).

> 
> All the stuff below also seems to deal with the fact that the old return
> ip by reference calling convention seems to actually work better with
> the code base..

Yeah, so maybe I should revert back to the old code? not sure... Anyway,
I think codebase could be changed over time from a single change. Anyway,
I'm fine with either way. So I may hear your perference about this and send
out the next version (I think such cleanup can be fited in 5.11, so I can
base on this and do more work....)

Thanks,
Gao Xiang

>