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

Gao Xiang
2020-12-07T14:21:31Z

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.

Thanks,
Gao Xiang

>