[PATCH v3 0/6] xfs: some xfs_dialloc() cleanup

[PATCH v3 0/6] xfs: some xfs_dialloc() cleanup

[Gao Xiang]

Hi folks,

This is v3 of the following patchset
https://lore.kernel.org/r/20201124155130.40848-1-hsiangkao@redhat.com
, which tends to simplify xfs_dialloc() logic.

This version includes Dave's original patch
https://lore.kernel.org/r/20201124221623.GC2842436@dread.disaster.area

to avoid the original double call of xfs_dialloc() and confusing
ialloc_context with some split in order for better review and minor
modification (e.g. ino isn't passed in xfs_ialloc()).

I'm not quite sure what's messy ENOSPC mentioned in the original
patch since return 0 and *ipp = NULL in xfs_dir_ialloc() would cause
NULL-dereference in its callers, so I leave this part alone, although
I think noroom has no use at all (it can be cleaned up as well with
a following patch) (at a glance, the final shape looks almost ok...)

I dropped [PATCH v1 3/3] since xfs_ialloc_select_ag() already looks
simple enough (comments about this are welcome... I can re-add this
if needed.)

I ran xfstests -g auto with the whole series and it seems no
noticable strange happening, yet I'm not quite sure if it may still
have potential issues...

Thanks for your time.

changes since v2:
 - use struct xfs_dquot_acct * instead of void * on dqinfo (Darrick);
 - rename xfs_ialloc() to xfs_dir_ialloc_init() (Dave);
 - fix a temporary state compile error due to (ialloc_context ->
   &ialloc_context) on [PATCH v2 3/6];
 - collect more RVBs from the reply of v2;
 - Cc Eric to confirm dqinfo;

changes since v1:
 - add Dave's patch with spilt and minor update;
 - update comments above xfs_ialloc_ag_alloc() suggested by Darrick;
 - collect RVBs to
    "xfs: convert noroom, okalloc in xfs_dialloc() to bool"
    "xfs: kill ialloced in xfs_dialloc()"
   since no real logic changes ("(!error)" to "(error==0)" suggested
   by Darrick has been updated).

Thanks,
Gao Xiang

Dave Chinner (4):
  xfs: introduce xfs_dialloc_roll()
  xfs: move on-disk inode allocation out of xfs_ialloc()
  xfs: move xfs_dialloc_roll() into xfs_dialloc()
  xfs: spilt xfs_dialloc() into 2 functions

Gao Xiang (2):
  xfs: convert noroom, okalloc in xfs_dialloc() to bool
  xfs: kill ialloced in xfs_dialloc()

 fs/xfs/libxfs/xfs_ialloc.c | 173 ++++++++++++++-------------
 fs/xfs/libxfs/xfs_ialloc.h |  36 +++---
 fs/xfs/xfs_inode.c         | 237 +++++++++----------------------------
 3 files changed, 163 insertions(+), 283 deletions(-)

-- 
2.18.4

[PATCH v3 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool

[Gao Xiang]

Boolean is preferred for such use.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ialloc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 974e71bc4a3a..45cf7e55f5ee 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1716,11 +1716,11 @@ xfs_dialloc(
 	xfs_agnumber_t		agno;
 	int			error;
 	int			ialloced;
-	int			noroom = 0;
+	bool			noroom = false;
 	xfs_agnumber_t		start_agno;
 	struct xfs_perag	*pag;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
-	int			okalloc = 1;
+	bool			okalloc = true;
 
 	if (*IO_agbp) {
 		/*
@@ -1753,8 +1753,8 @@ xfs_dialloc(
 	if (igeo->maxicount &&
 	    percpu_counter_read_positive(&mp->m_icount) + igeo->ialloc_inos
 							> igeo->maxicount) {
-		noroom = 1;
-		okalloc = 0;
+		noroom = true;
+		okalloc = false;
 	}
 
 	/*
-- 
2.18.4

[PATCH v3 2/6] xfs: introduce xfs_dialloc_roll()

[Gao Xiang]

From: Dave Chinner <dchinner@redhat.com>

Introduce a helper to make the on-disk inode allocation rolling
logic clearer in preparation of the following cleanup.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ialloc.c | 45 ++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_ialloc.h |  6 +++++
 fs/xfs/xfs_inode.c         | 39 +--------------------------------
 3 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 45cf7e55f5ee..3d2862e3ff41 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1682,6 +1682,51 @@ xfs_dialloc_ag(
 	return error;
 }
 
+int
+xfs_dialloc_roll(
+	struct xfs_trans	**tpp,
+	struct xfs_buf		*agibp)
+{
+	struct xfs_trans	*tp = *tpp;
+	struct xfs_dquot_acct	*dqinfo = NULL;
+	unsigned int		tflags = 0;
+	int			error;
+
+	/*
+	 * Hold to on to the agibp across the commit so no other allocation can
+	 * come in and take the free inodes we just allocated for our caller.
+	 */
+	xfs_trans_bhold(tp, agibp);
+
+	/*
+	 * We want the quota changes to be associated with the next transaction,
+	 * NOT this one. So, detach the dqinfo from this and attach it to the
+	 * next transaction.
+	 */
+	if (tp->t_dqinfo) {
+		dqinfo = tp->t_dqinfo;
+		tp->t_dqinfo = NULL;
+		tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
+		tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
+	}
+
+	error = xfs_trans_roll(&tp);
+
+	/* Re-attach the quota info that we detached from prev trx. */
+	if (dqinfo) {
+		tp->t_dqinfo = dqinfo;
+		tp->t_flags |= tflags;
+	}
+
+	*tpp = tp;
+	if (error) {
+		xfs_buf_relse(agibp);
+		return error;
+	}
+	xfs_trans_bjoin(tp, agibp);
+	return 0;
+}
+
 /*
  * Allocate an inode on disk.
  *
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 72b3468b97b1..a145e2a72530 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -32,6 +32,12 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
 	return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
 }
 
+/* XXX: will be removed in the following patch */
+int
+xfs_dialloc_roll(
+	struct xfs_trans	**tpp,
+	struct xfs_buf		*agibp);
+
 /*
  * Allocate an inode on disk.
  * Mode is used to tell whether the new inode will need space, and whether
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2bfbcf28b1bd..6672cdffcda5 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -958,8 +958,6 @@ xfs_dir_ialloc(
 	xfs_inode_t	*ip;
 	xfs_buf_t	*ialloc_context = NULL;
 	int		code;
-	void		*dqinfo;
-	uint		tflags;
 
 	tp = *tpp;
 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -1003,46 +1001,12 @@ xfs_dir_ialloc(
 	 * to succeed the second time.
 	 */
 	if (ialloc_context) {
-		/*
-		 * Normally, xfs_trans_commit releases all the locks.
-		 * We call bhold to hang on to the ialloc_context across
-		 * the commit.  Holding this buffer prevents any other
-		 * processes from doing any allocations in this
-		 * allocation group.
-		 */
-		xfs_trans_bhold(tp, ialloc_context);
-
-		/*
-		 * We want the quota changes to be associated with the next
-		 * transaction, NOT this one. So, detach the dqinfo from this
-		 * and attach it to the next transaction.
-		 */
-		dqinfo = NULL;
-		tflags = 0;
-		if (tp->t_dqinfo) {
-			dqinfo = (void *)tp->t_dqinfo;
-			tp->t_dqinfo = NULL;
-			tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
-			tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
-		}
-
-		code = xfs_trans_roll(&tp);
-
-		/*
-		 * Re-attach the quota info that we detached from prev trx.
-		 */
-		if (dqinfo) {
-			tp->t_dqinfo = dqinfo;
-			tp->t_flags |= tflags;
-		}
-
+		code = xfs_dialloc_roll(&tp, ialloc_context);
 		if (code) {
-			xfs_buf_relse(ialloc_context);
 			*tpp = tp;
 			*ipp = NULL;
 			return code;
 		}
-		xfs_trans_bjoin(tp, ialloc_context);
 
 		/*
 		 * Call ialloc again. Since we've locked out all
@@ -1062,7 +1026,6 @@ xfs_dir_ialloc(
 			return code;
 		}
 		ASSERT(!ialloc_context && ip);
-
 	}
 
 	*ipp = ip;
-- 
2.18.4

Re: [PATCH v3 2/6] xfs: introduce xfs_dialloc_roll()

[Christoph Hellwig]

> +	/*
> +	 * We want the quota changes to be associated with the next transaction,
> +	 * NOT this one. So, detach the dqinfo from this and attach it to the
> +	 * next transaction.
> +	 */
> +	if (tp->t_dqinfo) {
> +		dqinfo = tp->t_dqinfo;
> +		tp->t_dqinfo = NULL;
> +		tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> +		tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);

No need for the braces here.

> +	if (error) {
> +		xfs_buf_relse(agibp);
> +		return error;
> +	}

I haven't looked over the other patches if there is a good reason for
it, but to me keeping the xfs_buf_relse in the caller would seem more
natural.

>  
> +/* XXX: will be removed in the following patch */
> +int

I don't think the comment is very helpful.  As of this patch the
declaration is obviously needed, and that is all that matters.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 2/6] xfs: introduce xfs_dialloc_roll()

[Gao Xiang]

On Mon, Dec 07, 2020 at 02:43:50PM +0100, Christoph Hellwig wrote:
> > +	/*
> > +	 * We want the quota changes to be associated with the next transaction,
> > +	 * NOT this one. So, detach the dqinfo from this and attach it to the
> > +	 * next transaction.
> > +	 */
> > +	if (tp->t_dqinfo) {
> > +		dqinfo = tp->t_dqinfo;
> > +		tp->t_dqinfo = NULL;
> > +		tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> > +		tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
> 
> No need for the braces here.

Ok, will fix.

> 
> > +	if (error) {
> > +		xfs_buf_relse(agibp);
> > +		return error;
> > +	}
> 
> I haven't looked over the other patches if there is a good reason for
> it, but to me keeping the xfs_buf_relse in the caller would seem more
> natural.

This part is inherited from Dave's original patch, but I guess I could
move this to all callers if needed, no strong opinion from myself.

> 
> >  
> > +/* XXX: will be removed in the following patch */
> > +int
> 
> I don't think the comment is very helpful.  As of this patch the
> declaration is obviously needed, and that is all that matters.

Ok, will remove it.

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

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

[Gao Xiang]

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(-)

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;
 }
 
-- 
2.18.4

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

[Christoph Hellwig]

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.

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

[Gao Xiang]

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


> 

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

[Christoph Hellwig]

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.

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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

[PATCH v3 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc()

[Gao Xiang]

From: Dave Chinner <dchinner@redhat.com>

Get rid of the confusing ialloc_context and failure handling around
xfs_dialloc() by moving xfs_dialloc_roll() into xfs_dialloc().

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/libxfs/xfs_ialloc.c | 57 ++++++++++++--------------------------
 fs/xfs/libxfs/xfs_ialloc.h | 22 +--------------
 fs/xfs/xfs_inode.c         | 35 ++---------------------
 3 files changed, 22 insertions(+), 92 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 3d2862e3ff41..b00bbd680177 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1682,7 +1682,7 @@ xfs_dialloc_ag(
 	return error;
 }
 
-int
+static int
 xfs_dialloc_roll(
 	struct xfs_trans	**tpp,
 	struct xfs_buf		*agibp)
@@ -1733,30 +1733,18 @@ xfs_dialloc_roll(
  * Mode is used to tell whether the new inode will need space, and whether it
  * is a directory.
  *
- * This function is designed to be called twice if it has to do an allocation
- * to make more free inodes.  On the first call, *IO_agbp should be set to NULL.
- * If an inode is available without having to performn an allocation, an inode
- * number is returned.  In this case, *IO_agbp is set to NULL.  If an allocation
- * needs to be done, xfs_dialloc returns the current AGI buffer in *IO_agbp.
- * The caller should then commit the current transaction, allocate a
- * new transaction, and call xfs_dialloc() again, passing in the previous value
- * of *IO_agbp.  IO_agbp should be held across the transactions. Since the AGI
- * buffer is locked across the two calls, the second call is guaranteed to have
- * a free inode available.
- *
  * Once we successfully pick an inode its number is returned and the on-disk
  * data structures are updated.  The inode itself is not read in, since doing so
  * would break ordering constraints with xfs_reclaim.
  */
 int
 xfs_dialloc(
-	struct xfs_trans	*tp,
+	struct xfs_trans	**tpp,
 	xfs_ino_t		parent,
 	umode_t			mode,
-	struct xfs_buf		**IO_agbp,
 	xfs_ino_t		*inop)
 {
-	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_mount	*mp = (*tpp)->t_mountp;
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno;
 	int			error;
@@ -1767,21 +1755,11 @@ xfs_dialloc(
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
 	bool			okalloc = true;
 
-	if (*IO_agbp) {
-		/*
-		 * If the caller passes in a pointer to the AGI buffer,
-		 * continue where we left off before.  In this case, we
-		 * know that the allocation group has free inodes.
-		 */
-		agbp = *IO_agbp;
-		goto out_alloc;
-	}
-
 	/*
 	 * We do not have an agbp, so select an initial allocation
 	 * group for inode allocation.
 	 */
-	start_agno = xfs_ialloc_ag_select(tp, parent, mode);
+	start_agno = xfs_ialloc_ag_select(*tpp, parent, mode);
 	if (start_agno == NULLAGNUMBER) {
 		*inop = NULLFSINO;
 		return 0;
@@ -1816,7 +1794,7 @@ xfs_dialloc(
 		}
 
 		if (!pag->pagi_init) {
-			error = xfs_ialloc_pagi_init(mp, tp, agno);
+			error = xfs_ialloc_pagi_init(mp, *tpp, agno);
 			if (error)
 				goto out_error;
 		}
@@ -1831,7 +1809,7 @@ xfs_dialloc(
 		 * Then read in the AGI buffer and recheck with the AGI buffer
 		 * lock held.
 		 */
-		error = xfs_ialloc_read_agi(mp, tp, agno, &agbp);
+		error = xfs_ialloc_read_agi(mp, *tpp, agno, &agbp);
 		if (error)
 			goto out_error;
 
@@ -1844,9 +1822,9 @@ xfs_dialloc(
 			goto nextag_relse_buffer;
 
 
-		error = xfs_ialloc_ag_alloc(tp, agbp, &ialloced);
+		error = xfs_ialloc_ag_alloc(*tpp, agbp, &ialloced);
 		if (error) {
-			xfs_trans_brelse(tp, agbp);
+			xfs_trans_brelse(*tpp, agbp);
 
 			if (error != -ENOSPC)
 				goto out_error;
@@ -1858,21 +1836,23 @@ xfs_dialloc(
 
 		if (ialloced) {
 			/*
-			 * We successfully allocated some inodes, return
-			 * the current context to the caller so that it
-			 * can commit the current transaction and call
-			 * us again where we left off.
+			 * We successfully allocated some inodes, roll the
+			 * transaction so they can allocate one of the free
+			 * inodes we just prepared for them.
 			 */
 			ASSERT(pag->pagi_freecount > 0);
 			xfs_perag_put(pag);
 
-			*IO_agbp = agbp;
+			error = xfs_dialloc_roll(tpp, agbp);
+			if (error)
+				return error;
+
 			*inop = NULLFSINO;
-			return 0;
+			goto out_alloc;
 		}
 
 nextag_relse_buffer:
-		xfs_trans_brelse(tp, agbp);
+		xfs_trans_brelse(*tpp, agbp);
 nextag:
 		xfs_perag_put(pag);
 		if (++agno == mp->m_sb.sb_agcount)
@@ -1884,8 +1864,7 @@ xfs_dialloc(
 	}
 
 out_alloc:
-	*IO_agbp = NULL;
-	return xfs_dialloc_ag(tp, agbp, parent, inop);
+	return xfs_dialloc_ag(*tpp, agbp, parent, inop);
 out_error:
 	xfs_perag_put(pag);
 	return error;
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index a145e2a72530..13810ffe4af9 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -32,40 +32,20 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
 	return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
 }
 
-/* XXX: will be removed in the following patch */
-int
-xfs_dialloc_roll(
-	struct xfs_trans	**tpp,
-	struct xfs_buf		*agibp);
-
 /*
  * Allocate an inode on disk.
  * Mode is used to tell whether the new inode will need space, and whether
  * it is a directory.
  *
- * To work within the constraint of one allocation per transaction,
- * xfs_dialloc() is designed to be called twice if it has to do an
- * allocation to make more free inodes.  If an inode is
- * available without an allocation, agbp would be set to the current
- * agbp and alloc_done set to false.
- * If an allocation needed to be done, agbp would be set to the
- * inode header of the allocation group and alloc_done set to true.
- * The caller should then commit the current transaction and allocate a new
- * transaction.  xfs_dialloc() should then be called again with
- * the agbp value returned from the previous call.
- *
  * Once we successfully pick an inode its number is returned and the
  * on-disk data structures are updated.  The inode itself is not read
  * in, since doing so would break ordering constraints with xfs_reclaim.
- *
- * *agbp should be set to NULL on the first call, *alloc_done set to FALSE.
  */
 int					/* error */
 xfs_dialloc(
-	struct xfs_trans *tp,		/* transaction pointer */
+	struct xfs_trans **tpp,		/* double pointer of transaction */
 	xfs_ino_t	parent,		/* parent inode (directory) */
 	umode_t		mode,		/* mode bits for new inode */
-	struct xfs_buf	**agbp,		/* buf for a.g. inode header */
 	xfs_ino_t	*inop);		/* inode number allocated */
 
 /*
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 22843e81bccf..78ecfdf77320 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -909,7 +909,6 @@ xfs_dir_ialloc(
 					   locked. */
 {
 	xfs_inode_t	*ip;
-	xfs_buf_t	*ialloc_context = NULL;
 	xfs_ino_t	pino = dp ? dp->i_ino : 0;
 	xfs_ino_t	ino;
 	int		error;
@@ -919,40 +918,12 @@ xfs_dir_ialloc(
 
 	/*
 	 * 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.
-	 */
-	error = xfs_dialloc(*tpp, pino, mode, &ialloc_context, &ino);
+	 * allocated.
+	 */
+	error = xfs_dialloc(tpp, pino, mode, &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_dialloc() again.  It is guaranteed
-	 * to succeed the second time.
-	 */
-	if (ialloc_context) {
-		error = xfs_dialloc_roll(tpp, ialloc_context);
-		if (error)
-			return error;
-		/*
-		 * Call dialloc again. Since we've locked out all other
-		 * allocations in this allocation group, this call should
-		 * always succeed.
-		 */
-		error = xfs_dialloc(*tpp, pino, mode, &ialloc_context, &ino);
-		if (error)
-			return error;
-		ASSERT(!ialloc_context);
-	}
-
 	if (ino == NULLFSINO)
 		return -ENOSPC;
 
-- 
2.18.4

Re: [PATCH v3 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc()

[Christoph Hellwig]

>  
>  		if (ialloced) {
>  			/*
> +			 * We successfully allocated some inodes, roll the
> +			 * transaction so they can allocate one of the free
> +			 * inodes we just prepared for them.

Maybe:

			/*
			 * We successfully allocated space for an inode cluster
			 * in this AG.  Roll the transaction so that we can
			 * allocate one of the new inodes.
			 */

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc()

[Gao Xiang]

On Mon, Dec 07, 2020 at 02:53:36PM +0100, Christoph Hellwig wrote:
> >  
> >  		if (ialloced) {
> >  			/*
> > +			 * We successfully allocated some inodes, roll the
> > +			 * transaction so they can allocate one of the free
> > +			 * inodes we just prepared for them.
> 
> Maybe:
> 
> 			/*
> 			 * We successfully allocated space for an inode cluster
> 			 * in this AG.  Roll the transaction so that we can
> 			 * allocate one of the new inodes.
> 			 */

Okay, will update.

Thanks,
Gao Xiang

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

[PATCH v3 5/6] xfs: spilt xfs_dialloc() into 2 functions

[Gao Xiang]

From: Dave Chinner <dchinner@redhat.com>

This patch explicitly separates free inode chunk allocation and
inode allocation into two individual high level operations.

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/libxfs/xfs_ialloc.c | 59 +++++++++++++++++---------------------
 fs/xfs/libxfs/xfs_ialloc.h | 20 +++++++++----
 fs/xfs/xfs_inode.c         | 19 ++++++++----
 3 files changed, 55 insertions(+), 43 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index b00bbd680177..527f17f09bd3 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1570,7 +1570,7 @@ xfs_dialloc_ag_update_inobt(
  * The caller selected an AG for us, and made sure that free inodes are
  * available.
  */
-STATIC int
+int
 xfs_dialloc_ag(
 	struct xfs_trans	*tp,
 	struct xfs_buf		*agbp,
@@ -1728,21 +1728,22 @@ xfs_dialloc_roll(
 }
 
 /*
- * Allocate an inode on disk.
+ * Select and prepare an AG for inode allocation.
  *
- * Mode is used to tell whether the new inode will need space, and whether it
- * is a directory.
+ * Mode is used to tell whether the new inode is a directory and hence where to
+ * locate it.
  *
- * Once we successfully pick an inode its number is returned and the on-disk
- * data structures are updated.  The inode itself is not read in, since doing so
- * would break ordering constraints with xfs_reclaim.
+ * This function will ensure that the selected AG has free inodes available to
+ * allocate from. The selected AGI will be returned locked to the caller, and it
+ * will allocate more free inodes if required. If no free inodes are found or
+ * can be allocated, no AGI will be returned.
  */
 int
-xfs_dialloc(
+xfs_dialloc_select_ag(
 	struct xfs_trans	**tpp,
 	xfs_ino_t		parent,
 	umode_t			mode,
-	xfs_ino_t		*inop)
+	struct xfs_buf		**IO_agbp)
 {
 	struct xfs_mount	*mp = (*tpp)->t_mountp;
 	struct xfs_buf		*agbp;
@@ -1755,15 +1756,15 @@ xfs_dialloc(
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
 	bool			okalloc = true;
 
+	*IO_agbp = NULL;
+
 	/*
 	 * We do not have an agbp, so select an initial allocation
 	 * group for inode allocation.
 	 */
 	start_agno = xfs_ialloc_ag_select(*tpp, parent, mode);
-	if (start_agno == NULLAGNUMBER) {
-		*inop = NULLFSINO;
+	if (start_agno == NULLAGNUMBER)
 		return 0;
-	}
 
 	/*
 	 * If we have already hit the ceiling of inode blocks then clear
@@ -1796,7 +1797,7 @@ xfs_dialloc(
 		if (!pag->pagi_init) {
 			error = xfs_ialloc_pagi_init(mp, *tpp, agno);
 			if (error)
-				goto out_error;
+				break;
 		}
 
 		/*
@@ -1811,11 +1812,12 @@ xfs_dialloc(
 		 */
 		error = xfs_ialloc_read_agi(mp, *tpp, agno, &agbp);
 		if (error)
-			goto out_error;
+			break;
 
 		if (pag->pagi_freecount) {
 			xfs_perag_put(pag);
-			goto out_alloc;
+			*IO_agbp = agbp;
+			return 0;
 		}
 
 		if (!okalloc)
@@ -1826,19 +1828,17 @@ xfs_dialloc(
 		if (error) {
 			xfs_trans_brelse(*tpp, agbp);
 
-			if (error != -ENOSPC)
-				goto out_error;
-
-			xfs_perag_put(pag);
-			*inop = NULLFSINO;
-			return 0;
+			if (error == -ENOSPC)
+				error = 0;
+			break;
 		}
 
 		if (ialloced) {
 			/*
-			 * We successfully allocated some inodes, roll the
-			 * transaction so they can allocate one of the free
-			 * inodes we just prepared for them.
+			 * We successfully allocated some inodes, so roll the
+			 * transaction and return the locked AGI buffer to the
+			 * caller so they can allocate one of the free inodes we
+			 * just prepared for them.
 			 */
 			ASSERT(pag->pagi_freecount > 0);
 			xfs_perag_put(pag);
@@ -1847,8 +1847,8 @@ xfs_dialloc(
 			if (error)
 				return error;
 
-			*inop = NULLFSINO;
-			goto out_alloc;
+			*IO_agbp = agbp;
+			return 0;
 		}
 
 nextag_relse_buffer:
@@ -1857,15 +1857,10 @@ xfs_dialloc(
 		xfs_perag_put(pag);
 		if (++agno == mp->m_sb.sb_agcount)
 			agno = 0;
-		if (agno == start_agno) {
-			*inop = NULLFSINO;
+		if (agno == start_agno)
 			return noroom ? -ENOSPC : 0;
-		}
 	}
 
-out_alloc:
-	return xfs_dialloc_ag(*tpp, agbp, parent, inop);
-out_error:
 	xfs_perag_put(pag);
 	return error;
 }
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 13810ffe4af9..3511086a7ae1 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -37,16 +37,26 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
  * Mode is used to tell whether the new inode will need space, and whether
  * it is a directory.
  *
- * Once we successfully pick an inode its number is returned and the
- * on-disk data structures are updated.  The inode itself is not read
- * in, since doing so would break ordering constraints with xfs_reclaim.
+ * There are two phases to inode allocation: selecting an AG and ensuring
+ * that it contains free inodes, followed by allocating one of the free
+ * inodes. xfs_dialloc_select_ag() does the former and returns a locked AGI
+ * to the caller, ensuring that followup call to xfs_dialloc_ag() will
+ * have free inodes to allocate from. xfs_dialloc_ag() will return the inode
+ * number of the free inode we allocated.
  */
 int					/* error */
-xfs_dialloc(
+xfs_dialloc_select_ag(
 	struct xfs_trans **tpp,		/* double pointer of transaction */
 	xfs_ino_t	parent,		/* parent inode (directory) */
 	umode_t		mode,		/* mode bits for new inode */
-	xfs_ino_t	*inop);		/* inode number allocated */
+	struct xfs_buf	**IO_agbp);
+
+int
+xfs_dialloc_ag(
+	struct xfs_trans	*tp,
+	struct xfs_buf		*agbp,
+	xfs_ino_t		parent,
+	xfs_ino_t		*inop);
 
 /*
  * Free disk inode.  Carefully avoids touching the incore inode, all
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 78ecfdf77320..90dadf862b3a 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -908,10 +908,11 @@ xfs_dir_ialloc(
 	xfs_inode_t	**ipp)		/* pointer to inode; it will be
 					   locked. */
 {
-	xfs_inode_t	*ip;
-	xfs_ino_t	pino = dp ? dp->i_ino : 0;
-	xfs_ino_t	ino;
-	int		error;
+	struct xfs_buf		*agibp;
+	struct xfs_inode	*ip;
+	xfs_ino_t		pino = dp ? dp->i_ino : 0;
+	xfs_ino_t		ino;
+	int			error;
 
 	ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
 	*ipp = NULL;
@@ -920,13 +921,19 @@ xfs_dir_ialloc(
 	 * Call the space management code to pick the on-disk inode to be
 	 * allocated.
 	 */
-	error = xfs_dialloc(tpp, pino, mode, &ino);
+	error = xfs_dialloc_select_ag(tpp, pino, mode, &agibp);
 	if (error)
 		return error;
 
-	if (ino == NULLFSINO)
+	if (!agibp)
 		return -ENOSPC;
 
+	/* Allocate an inode from the selected AG */
+	error = xfs_dialloc_ag(*tpp, agibp, pino, &ino);
+	if (error)
+		return error;
+	ASSERT(ino != NULLFSINO);
+
 	/* Initialise the newly allocated inode. */
 	ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid);
 	if (IS_ERR(ip))
-- 
2.18.4

Re: [PATCH v3 5/6] xfs: spilt xfs_dialloc() into 2 functions

[Christoph Hellwig]

>  		if (pag->pagi_freecount) {
>  			xfs_perag_put(pag);
> +			*IO_agbp = agbp;
> +			return 0;

I think assigning *IO_agbp would benefit from a little consolidation.
Set it to NULL in the normal unsuccessful return, and add a found_ag
label that assigns agbp and returns 0.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 5/6] xfs: spilt xfs_dialloc() into 2 functions

[Gao Xiang]

Hi Christoph,

On Mon, Dec 07, 2020 at 02:56:42PM +0100, Christoph Hellwig wrote:
> >  		if (pag->pagi_freecount) {
> >  			xfs_perag_put(pag);
> > +			*IO_agbp = agbp;
> > +			return 0;
> 
> I think assigning *IO_agbp would benefit from a little consolidation.
> Set it to NULL in the normal unsuccessful return, and add a found_ag
> label that assigns agbp and returns 0.

Just to confirm the main idea, I think it might be:

*IO_agbp = NULL;  at first,

and combine all such assignment
> > +			*IO_agbp = agbp;
> > +			return 0;
>

into a new found_ag lebel, and use goto found_ag; for such cases.
Do I understand correctly? If that is correct, will update
in the next version.

Thanks,
Gao Xiang

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Re: [PATCH v3 5/6] xfs: spilt xfs_dialloc() into 2 functions

[Christoph Hellwig]

On Mon, Dec 07, 2020 at 10:33:00PM +0800, Gao Xiang wrote:
> Hi Christoph,
> 
> On Mon, Dec 07, 2020 at 02:56:42PM +0100, Christoph Hellwig wrote:
> > >  		if (pag->pagi_freecount) {
> > >  			xfs_perag_put(pag);
> > > +			*IO_agbp = agbp;
> > > +			return 0;
> > 
> > I think assigning *IO_agbp would benefit from a little consolidation.
> > Set it to NULL in the normal unsuccessful return, and add a found_ag
> > label that assigns agbp and returns 0.
> 
> Just to confirm the main idea, I think it might be:
> 
> *IO_agbp = NULL;  at first,
> 
> and combine all such assignment
> > > +			*IO_agbp = agbp;
> > > +			return 0;
> >
> 
> into a new found_ag lebel, and use goto found_ag; for such cases.
> Do I understand correctly? If that is correct, will update
> in the next version.

I would also move

	*IO_agbp = NULL;

to just before the

	return error;

to match the assignment for the successful case, but that isn't
the important part.  I think the important part is to have on
central place for the sucessful return.

[PATCH v3 6/6] xfs: kill ialloced in xfs_dialloc()

[Gao Xiang]

It's enough to just use return code, and get rid of an argument.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ialloc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 527f17f09bd3..a0e6e333eea2 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -607,13 +607,13 @@ xfs_inobt_insert_sprec(
 
 /*
  * Allocate new inodes in the allocation group specified by agbp.
- * Return 0 for success, else error code.
+ * Returns 0 if inodes were allocated in this AG; 1 if there was no space
+ * in this AG; or the usual negative error code.
  */
 STATIC int
 xfs_ialloc_ag_alloc(
 	struct xfs_trans	*tp,
-	struct xfs_buf		*agbp,
-	int			*alloc)
+	struct xfs_buf		*agbp)
 {
 	struct xfs_agi		*agi;
 	struct xfs_alloc_arg	args;
@@ -795,10 +795,9 @@ xfs_ialloc_ag_alloc(
 		allocmask = (1 << (newlen / XFS_INODES_PER_HOLEMASK_BIT)) - 1;
 	}
 
-	if (args.fsbno == NULLFSBLOCK) {
-		*alloc = 0;
-		return 0;
-	}
+	if (args.fsbno == NULLFSBLOCK)
+		return 1;
+
 	ASSERT(args.len == args.minlen);
 
 	/*
@@ -903,7 +902,6 @@ xfs_ialloc_ag_alloc(
 	 */
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_ICOUNT, (long)newlen);
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, (long)newlen);
-	*alloc = 1;
 	return 0;
 }
 
@@ -1749,7 +1747,6 @@ xfs_dialloc_select_ag(
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno;
 	int			error;
-	int			ialloced;
 	bool			noroom = false;
 	xfs_agnumber_t		start_agno;
 	struct xfs_perag	*pag;
@@ -1823,17 +1820,14 @@ xfs_dialloc_select_ag(
 		if (!okalloc)
 			goto nextag_relse_buffer;
 
-
-		error = xfs_ialloc_ag_alloc(*tpp, agbp, &ialloced);
-		if (error) {
+		error = xfs_ialloc_ag_alloc(*tpp, agbp);
+		if (error < 0) {
 			xfs_trans_brelse(*tpp, agbp);
 
 			if (error == -ENOSPC)
 				error = 0;
 			break;
-		}
-
-		if (ialloced) {
+		} else if (error == 0) {
 			/*
 			 * We successfully allocated some inodes, so roll the
 			 * transaction and return the locked AGI buffer to the
-- 
2.18.4

Re: [PATCH v3 6/6] xfs: kill ialloced in xfs_dialloc()

[Christoph Hellwig]

> +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> +		if (error < 0) {
>  			xfs_trans_brelse(*tpp, agbp);
>  
>  			if (error == -ENOSPC)
>  				error = 0;
>  			break;
> +		} else if (error == 0) {

No need for the else after the break.

Re: [PATCH v3 6/6] xfs: kill ialloced in xfs_dialloc()

[Gao Xiang]

On Mon, Dec 07, 2020 at 02:57:19PM +0100, Christoph Hellwig wrote:
> > +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> > +		if (error < 0) {
> >  			xfs_trans_brelse(*tpp, agbp);
> >  
> >  			if (error == -ENOSPC)
> >  				error = 0;
> >  			break;
> > +		} else if (error == 0) {
> 
> No need for the else after the break.

Personally, I'd like to save a line by using "} else if {"
for such case (and tell readers about these two judgments),
and for any cases, compilers will do their best.

Yet, if you like, I could add an extra line for this.
Will update in the next version.

Thanks,
Gao Xiang

> 

Re: [PATCH v3 6/6] xfs: kill ialloced in xfs_dialloc()

[Dave Chinner]

On Mon, Dec 07, 2020 at 10:24:48PM +0800, Gao Xiang wrote:
> On Mon, Dec 07, 2020 at 02:57:19PM +0100, Christoph Hellwig wrote:
> > > +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> > > +		if (error < 0) {
> > >  			xfs_trans_brelse(*tpp, agbp);
> > >  
> > >  			if (error == -ENOSPC)
> > >  				error = 0;
> > >  			break;
> > > +		} else if (error == 0) {
> > 
> > No need for the else after the break.
> 
> Personally, I'd like to save a line by using "} else if {"
> for such case (and tell readers about these two judgments),
> and for any cases, compilers will do their best.

And extra line is not an issue, and the convention we use everywhere
is to elide the "else" whereever possible. e.g. we do:

	if (foo)
		return false;
	if (!bar)
		return true;
	if (baz)
		return false;
	return true;

Rather than if() {} else if() {} else if() {} else {}. The elses in
these cases mainly obfuscate the actual logic flow...

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 6/6] xfs: kill ialloced in xfs_dialloc()

[Gao Xiang]

On Tue, Dec 08, 2020 at 07:23:45AM +1100, Dave Chinner wrote:
> On Mon, Dec 07, 2020 at 10:24:48PM +0800, Gao Xiang wrote:
> > On Mon, Dec 07, 2020 at 02:57:19PM +0100, Christoph Hellwig wrote:
> > > > +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> > > > +		if (error < 0) {
> > > >  			xfs_trans_brelse(*tpp, agbp);
> > > >  
> > > >  			if (error == -ENOSPC)
> > > >  				error = 0;
> > > >  			break;
> > > > +		} else if (error == 0) {
> > > 
> > > No need for the else after the break.
> > 
> > Personally, I'd like to save a line by using "} else if {"
> > for such case (and tell readers about these two judgments),
> > and for any cases, compilers will do their best.
> 
> And extra line is not an issue, and the convention we use everywhere
> is to elide the "else" whereever possible. e.g. we do:
> 
> 	if (foo)
> 		return false;
> 	if (!bar)
> 		return true;
> 	if (baz)
> 		return false;
> 	return true;
> 
> Rather than if() {} else if() {} else if() {} else {}. The elses in
> these cases mainly obfuscate the actual logic flow...

(I mean no need to to use else if on irrelevant relationship as well)
Anyway, let me update it later...

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
>