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

Commit Message

Gao Xiang Dec. 7, 2020, 12:15 a.m. UTC

From: Dave Chinner <[e-mail]>

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

Reviewed-by: Darrick J. Wong <[e-mail]>
Signed-off-by: Dave Chinner <[e-mail]>
Signed-off-by: Gao Xiang <[e-mail]>
---
 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(-)

Comments

Christoph Hellwig Dec. 7, 2020, 1:56 p.m. UTC | #1

>  		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 <[e-mail]>

Gao Xiang Dec. 7, 2020, 2:33 p.m. UTC | #2

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 <[e-mail]>
>

Christoph Hellwig Dec. 7, 2020, 4:38 p.m. UTC | #3

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

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