diff mbox series

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

Message ID 20201207001533.2702719-4-hsiangkao@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series xfs: some xfs_dialloc() cleanup | expand

Commit Message

Gao Xiang Dec. 7, 2020, 12:15 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

So xfs_ialloc() will only address in-core inode allocation then,
Also, rename xfs_ialloc() to xfs_dir_ialloc_init() in order to
keep everything in xfs_inode.c under the same namespace.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_inode.c | 202 +++++++++++++++------------------------------
 1 file changed, 66 insertions(+), 136 deletions(-)

Comments

Christoph Hellwig Dec. 7, 2020, 1:49 p.m. UTC | #1
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.
Gao Xiang Dec. 7, 2020, 2:19 p.m. UTC | #2
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


>
Christoph Hellwig Dec. 7, 2020, 2:21 p.m. UTC | #3
On Mon, Dec 07, 2020 at 10:19:48PM +0800, Gao Xiang wrote:
> > 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.

Yeah, parent_ino is what I mean to suggest anyway, sorry for the typo.
Darrick J. Wong Dec. 7, 2020, 4:59 p.m. UTC | #4
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
Dave Chinner Dec. 7, 2020, 8:19 p.m. UTC | #5
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.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 6672cdffcda5..22843e81bccf 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -761,68 +761,25 @@  xfs_inode_inherit_flags2(
 }
 
 /*
- * Allocate an inode on disk and return a copy of its in-core version.
- * The in-core inode is locked exclusively.  Set mode, nlink, and rdev
- * appropriately within the inode.  The uid and gid for the inode are
- * set according to the contents of the given cred structure.
- *
- * Use xfs_dialloc() to allocate the on-disk inode. If xfs_dialloc()
- * has a free inode available, call xfs_iget() to obtain the in-core
- * version of the allocated inode.  Finally, fill in the inode and
- * log its initial contents.  In this case, ialloc_context would be
- * set to NULL.
- *
- * If xfs_dialloc() does not have an available inode, it will replenish
- * its supply by doing an allocation. Since we can only do one
- * allocation within a transaction without deadlocks, we must commit
- * the current transaction before returning the inode itself.
- * In this case, therefore, we will set ialloc_context and return.
- * The caller should then commit the current transaction, start a new
- * transaction, and call xfs_ialloc() again to actually get the inode.
- *
- * To ensure that some other process does not grab the inode that
- * was allocated during the first call to xfs_ialloc(), this routine
- * also returns the [locked] bp pointing to the head of the freelist
- * as ialloc_context.  The caller should hold this buffer across
- * the commit and pass it back into this routine on the second call.
- *
- * If we are allocating quota inodes, we do not have a parent inode
- * to attach to or associate with (i.e. pip == NULL) because they
- * are not linked into the directory structure - they are attached
- * directly to the superblock - and so have no parent.
+ * 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(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*pip,
+	xfs_ino_t		ino,
+	umode_t			mode,
+	xfs_nlink_t		nlink,
+	dev_t			rdev,
+	prid_t			prid)
 {
-	struct xfs_mount *mp = tp->t_mountp;
-	xfs_ino_t	ino;
-	xfs_inode_t	*ip;
-	uint		flags;
-	int		error;
-	struct timespec64 tv;
-	struct inode	*inode;
-
-	/*
-	 * Call the space management code to pick
-	 * the on-disk inode to be allocated.
-	 */
-	error = xfs_dialloc(tp, pip ? pip->i_ino : 0, mode,
-			    ialloc_context, &ino);
-	if (error)
-		return error;
-	if (*ialloc_context || ino == NULLFSINO) {
-		*ipp = NULL;
-		return 0;
-	}
-	ASSERT(*ialloc_context == NULL);
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_inode	*ip;
+	unsigned int		flags;
+	int			error;
+	struct timespec64	tv;
+	struct inode		*inode;
 
 	/*
 	 * Protect against obviously corrupt allocation btree records. Later
@@ -833,18 +790,16 @@  xfs_ialloc(
 	 */
 	if ((pip && ino == pip->i_ino) || !xfs_verify_dir_ino(mp, ino)) {
 		xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino);
-		return -EFSCORRUPTED;
+		return ERR_PTR(-EFSCORRUPTED);
 	}
 
 	/*
-	 * Get the in-core inode with the lock held exclusively.
-	 * This is because we're setting fields here we need
-	 * to prevent others from looking at until we're done.
+	 * Get the in-core inode with the lock held exclusively to prevent
+	 * others from looking at until we're done.
 	 */
-	error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE,
-			 XFS_ILOCK_EXCL, &ip);
+	error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, &ip);
 	if (error)
-		return error;
+		return ERR_PTR(error);
 	ASSERT(ip != NULL);
 	inode = VFS_I(ip);
 	inode->i_mode = mode;
@@ -926,20 +881,19 @@  xfs_ialloc(
 
 	/* now that we have an i_mode we can setup the inode structure */
 	xfs_setup_inode(ip);
-
-	*ipp = ip;
-	return 0;
+	return ip;
 }
 
 /*
- * Allocates a new inode from disk and return a pointer to the
- * incore copy. This routine will internally commit the current
- * transaction and allocate a new one if the Space Manager needed
- * to do an allocation to replenish the inode free-list.
- *
- * This routine is designed to be called from xfs_create and
- * xfs_create_dir.
+ * Allocates a new inode from disk and return a pointer to the incore copy. This
+ * routine will internally commit the current transaction and allocate a new one
+ * if we needed to allocate more on-disk free inodes to perform the requested
+ * operation.
  *
+ * If we are allocating quota inodes, we do not have a parent inode to attach to
+ * or associate with (i.e. dp == NULL) because they are not linked into the
+ * directory structure - they are attached directly to the superblock - and so
+ * have no parent.
  */
 int
 xfs_dir_ialloc(
@@ -954,83 +908,59 @@  xfs_dir_ialloc(
 	xfs_inode_t	**ipp)		/* pointer to inode; it will be
 					   locked. */
 {
-	xfs_trans_t	*tp;
 	xfs_inode_t	*ip;
 	xfs_buf_t	*ialloc_context = NULL;
-	int		code;
-
-	tp = *tpp;
-	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
+	xfs_ino_t	pino = dp ? dp->i_ino : 0;
+	xfs_ino_t	ino;
+	int		error;
 
-	/*
-	 * xfs_ialloc will return a pointer to an incore inode if
-	 * the Space Manager has an available inode on the free
-	 * list. Otherwise, it will do an allocation and replenish
-	 * the freelist.  Since we can only do one allocation per
-	 * transaction without deadlocks, we will need to commit the
-	 * current transaction and start a new one.  We will then
-	 * need to call xfs_ialloc again to get the inode.
-	 *
-	 * If xfs_ialloc did an allocation to replenish the freelist,
-	 * it returns the bp containing the head of the freelist as
-	 * ialloc_context. We will hold a lock on it across the
-	 * transaction commit so that no other process can steal
-	 * the inode(s) that we've just allocated.
-	 */
-	code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
-			&ip);
+	ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
+	*ipp = NULL;
 
 	/*
-	 * Return an error if we were unable to allocate a new inode.
-	 * This should only happen if we run out of space on disk or
-	 * encounter a disk error.
+	 * Call the space management code to pick the on-disk inode to be
+	 * allocated and replenish the freelist.  Since we can only do one
+	 * allocation per transaction without deadlocks, we will need to
+	 * commit the current transaction and start a new one.
+	 * If xfs_dialloc did an allocation to replenish the freelist, it
+	 * returns the bp containing the head of the freelist as
+	 * ialloc_context. We will hold a lock on it across the transaction
+	 * commit so that no other process can steal the inode(s) that we've
+	 * just allocated.
 	 */
-	if (code) {
-		*ipp = NULL;
-		return code;
-	}
-	if (!ialloc_context && !ip) {
-		*ipp = NULL;
-		return -ENOSPC;
-	}
+	error = xfs_dialloc(*tpp, pino, mode, &ialloc_context, &ino);
+	if (error)
+		return error;
 
 	/*
 	 * If the AGI buffer is non-NULL, then we were unable to get an
 	 * inode in one operation.  We need to commit the current
-	 * transaction and call xfs_ialloc() again.  It is guaranteed
+	 * transaction and call xfs_dialloc() again.  It is guaranteed
 	 * to succeed the second time.
 	 */
 	if (ialloc_context) {
-		code = xfs_dialloc_roll(&tp, ialloc_context);
-		if (code) {
-			*tpp = tp;
-			*ipp = NULL;
-			return code;
-		}
-
-		/*
-		 * Call ialloc again. Since we've locked out all
-		 * other allocations in this allocation group,
-		 * this call should always succeed.
-		 */
-		code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
-				  &ialloc_context, &ip);
-
+		error = xfs_dialloc_roll(tpp, ialloc_context);
+		if (error)
+			return error;
 		/*
-		 * If we get an error at this point, return to the caller
-		 * so that the current transaction can be aborted.
+		 * Call dialloc again. Since we've locked out all other
+		 * allocations in this allocation group, this call should
+		 * always succeed.
 		 */
-		if (code) {
-			*tpp = tp;
-			*ipp = NULL;
-			return code;
-		}
-		ASSERT(!ialloc_context && ip);
+		error = xfs_dialloc(*tpp, pino, mode, &ialloc_context, &ino);
+		if (error)
+			return error;
+		ASSERT(!ialloc_context);
 	}
 
-	*ipp = ip;
-	*tpp = tp;
+	if (ino == NULLFSINO)
+		return -ENOSPC;
 
+	/* 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;
 }