diff mbox

[008/119] xfs: separate freelist fixing into a separate helper

Message ID 146612632363.12839.15504324533944246285.stgit@birch.djwong.org (mailing list archive)
State New, archived
Headers show

Commit Message

Darrick J. Wong June 17, 2016, 1:18 a.m. UTC 
From: Dave Chinner <david@fromorbit.com>

Break up xfs_free_extent() into a helper that fixes the freelist.
This helper will be used subsequently to ensure the freelist during
deferred rmap processing.

Signed-off-by: Dave Chinner <david@fromorbit.com>
[darrick: refactor to put this at the head of the patchset]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |   82 +++++++++++++++++++++++++++++----------------
 fs/xfs/libxfs/xfs_alloc.h |    2 +
 2 files changed, 55 insertions(+), 29 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig June 17, 2016, 11:52 a.m. UTC | #1 
> +/* Ensure that the freelist is at full capacity. */
> +int
> +xfs_free_extent_fix_freelist(
> +	struct xfs_trans	*tp,
> +	xfs_agnumber_t		agno,
> +	struct xfs_buf		**agbp)
>  {
> -	xfs_alloc_arg_t	args;
> -	int		error;
> +	xfs_alloc_arg_t		args;

Use struct xfs_alloc_arg if you change this anyway.

> +	int			error;
>  
> -	ASSERT(len != 0);
>  	memset(&args, 0, sizeof(xfs_alloc_arg_t));

Same here.

> -	if (args.agbno + len >
> -			be32_to_cpu(XFS_BUF_TO_AGF(args.agbp)->agf_length)) {
> -		error = -EFSCORRUPTED;
> -		goto error0;
> -	}
> +	XFS_WANT_CORRUPTED_GOTO(mp,
> +			agbno + len <= be32_to_cpu(XFS_BUF_TO_AGF(agbp)->agf_length),
> +			err);

This introduces an overly long line.

But except for these nitpicks this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner June 21, 2016, 12:48 a.m. UTC | #2 
On Fri, Jun 17, 2016 at 04:52:04AM -0700, Christoph Hellwig wrote:
> > +/* Ensure that the freelist is at full capacity. */
> > +int
> > +xfs_free_extent_fix_freelist(
> > +	struct xfs_trans	*tp,
> > +	xfs_agnumber_t		agno,
> > +	struct xfs_buf		**agbp)
> >  {
> > -	xfs_alloc_arg_t	args;
> > -	int		error;
> > +	xfs_alloc_arg_t		args;
> 
> Use struct xfs_alloc_arg if you change this anyway.
> 
> > +	int			error;
> >  
> > -	ASSERT(len != 0);
> >  	memset(&args, 0, sizeof(xfs_alloc_arg_t));
> 
> Same here.
> 
> > -	if (args.agbno + len >
> > -			be32_to_cpu(XFS_BUF_TO_AGF(args.agbp)->agf_length)) {
> > -		error = -EFSCORRUPTED;
> > -		goto error0;
> > -	}
> > +	XFS_WANT_CORRUPTED_GOTO(mp,
> > +			agbno + len <= be32_to_cpu(XFS_BUF_TO_AGF(agbp)->agf_length),
> > +			err);
> 
> This introduces an overly long line.
> 
> But except for these nitpicks this looks fine:

I'll clean them up on commit.

-Dave.
Dave Chinner June 21, 2016, 1:40 a.m. UTC | #3 
On Thu, Jun 16, 2016 at 06:18:43PM -0700, Darrick J. Wong wrote:
> From: Dave Chinner <david@fromorbit.com>
> 
> Break up xfs_free_extent() into a helper that fixes the freelist.
> This helper will be used subsequently to ensure the freelist during
> deferred rmap processing.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>

Just noticed - should be from/sob dchinner@redhat.com. I'll fix this
up, too.

Cheers,

Dave.
diff mbox

Patch

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 58bdca7..1c76a0e 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2660,55 +2660,79 @@  error0:
 	return error;
 }
 
-/*
- * Free an extent.
- * Just break up the extent address and hand off to xfs_free_ag_extent
- * after fixing up the freelist.
- */
-int				/* error */
-xfs_free_extent(
-	xfs_trans_t	*tp,	/* transaction pointer */
-	xfs_fsblock_t	bno,	/* starting block number of extent */
-	xfs_extlen_t	len)	/* length of extent */
+/* Ensure that the freelist is at full capacity. */
+int
+xfs_free_extent_fix_freelist(
+	struct xfs_trans	*tp,
+	xfs_agnumber_t		agno,
+	struct xfs_buf		**agbp)
 {
-	xfs_alloc_arg_t	args;
-	int		error;
+	xfs_alloc_arg_t		args;
+	int			error;
 
-	ASSERT(len != 0);
 	memset(&args, 0, sizeof(xfs_alloc_arg_t));
 	args.tp = tp;
 	args.mp = tp->t_mountp;
+	args.agno = agno;
 
 	/*
 	 * validate that the block number is legal - the enables us to detect
 	 * and handle a silent filesystem corruption rather than crashing.
 	 */
-	args.agno = XFS_FSB_TO_AGNO(args.mp, bno);
 	if (args.agno >= args.mp->m_sb.sb_agcount)
 		return -EFSCORRUPTED;
 
-	args.agbno = XFS_FSB_TO_AGBNO(args.mp, bno);
-	if (args.agbno >= args.mp->m_sb.sb_agblocks)
-		return -EFSCORRUPTED;
-
 	args.pag = xfs_perag_get(args.mp, args.agno);
 	ASSERT(args.pag);
 
 	error = xfs_alloc_fix_freelist(&args, XFS_ALLOC_FLAG_FREEING);
 	if (error)
-		goto error0;
+		goto out;
+
+	*agbp = args.agbp;
+out:
+	xfs_perag_put(args.pag);
+	return error;
+}
+
+/*
+ * Free an extent.
+ * Just break up the extent address and hand off to xfs_free_ag_extent
+ * after fixing up the freelist.
+ */
+int				/* error */
+xfs_free_extent(
+	struct xfs_trans	*tp,	/* transaction pointer */
+	xfs_fsblock_t		bno,	/* starting block number of extent */
+	xfs_extlen_t		len)	/* length of extent */
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_buf		*agbp;
+	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
+	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
+	int			error;
+
+	ASSERT(len != 0);
+
+	error = xfs_free_extent_fix_freelist(tp, agno, &agbp);
+	if (error)
+		return error;
+
+	XFS_WANT_CORRUPTED_GOTO(mp, agbno < mp->m_sb.sb_agblocks, err);
 
 	/* validate the extent size is legal now we have the agf locked */
-	if (args.agbno + len >
-			be32_to_cpu(XFS_BUF_TO_AGF(args.agbp)->agf_length)) {
-		error = -EFSCORRUPTED;
-		goto error0;
-	}
+	XFS_WANT_CORRUPTED_GOTO(mp,
+			agbno + len <= be32_to_cpu(XFS_BUF_TO_AGF(agbp)->agf_length),
+			err);
 
-	error = xfs_free_ag_extent(tp, args.agbp, args.agno, args.agbno, len, 0);
-	if (!error)
-		xfs_extent_busy_insert(tp, args.agno, args.agbno, len, 0);
-error0:
-	xfs_perag_put(args.pag);
+	error = xfs_free_ag_extent(tp, agbp, agno, agbno, len, 0);
+	if (error)
+		goto err;
+
+	xfs_extent_busy_insert(tp, agno, agbno, len, 0);
+	return 0;
+
+err:
+	xfs_trans_brelse(tp, agbp);
 	return error;
 }
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 92a66ba..cf268b2 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -229,5 +229,7 @@  xfs_alloc_get_rec(
 int xfs_read_agf(struct xfs_mount *mp, struct xfs_trans *tp,
 			xfs_agnumber_t agno, int flags, struct xfs_buf **bpp);
 int xfs_alloc_fix_freelist(struct xfs_alloc_arg *args, int flags);
+int xfs_free_extent_fix_freelist(struct xfs_trans *tp, xfs_agnumber_t agno,
+		struct xfs_buf **agbp);
 
 #endif	/* __XFS_ALLOC_H__ */