[PATCH 7/7] split up xlog_recover_process_iunlinks

[PATCH 7/7] split up xlog_recover_process_iunlinks

[Christoph Hellwig]

[-- Attachment #1: xfs-split-xlog_recover_process_iunlinks --]
[-- Type: text/plain, Size: 4604 bytes --]

Split out the body of the main loop into a separate helper to make the
code readable.


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

Index: linux-2.6-xfs/fs/xfs/xfs_log_recover.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_log_recover.c	2008-10-25 13:22:29.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_log_recover.c	2008-10-25 13:26:44.000000000 +0200
@@ -3147,6 +3147,70 @@ out_error:
 	return;
 }
 
+STATIC xfs_agino_t
+xlog_recover_process_one_iunlink(
+	struct xfs_mount		*mp,
+	xfs_agnumber_t			agno,
+	xfs_agino_t			agino,
+	int				bucket)
+{
+	struct xfs_buf			*ibp;
+	struct xfs_dinode		*dip;
+	struct xfs_inode		*ip;
+	xfs_ino_t			ino;
+	int				error;
+
+	ino = XFS_AGINO_TO_INO(mp, agno, agino);
+	error = xfs_iget(mp, NULL, ino, 0, 0, &ip, 0);
+	if (error)
+		goto fail;
+
+	/*
+	 * Get the on disk inode to find the next inode in the bucket.
+	 */
+	ASSERT(ip != NULL);
+	error = xfs_itobp(mp, NULL, ip, &dip, &ibp, 0, 0, XFS_BUF_LOCK);
+	if (error)
+		goto fail;
+
+	ASSERT(dip != NULL);
+	ASSERT(ip->i_d.di_nlink == 0);
+
+	/* setup for the next pass */
+	agino = be32_to_cpu(dip->di_next_unlinked);
+	xfs_buf_relse(ibp);
+
+	/*
+	 * Prevent any DMAPI event from being sent when the reference on
+	 * the inode is dropped.
+	 */
+	ip->i_d.di_dmevmask = 0;
+
+	/*
+	 * If this is a new inode, handle it specially.  Otherwise, just
+	 * drop our reference to the inode.  If there are no other
+	 * references, this will send the inode to xfs_inactive() which
+	 * will truncate the file and free the inode.
+	 */
+	if (ip->i_d.di_mode == 0)
+		xfs_iput_new(ip, 0);
+	else
+		IRELE(ip);
+	return agino;
+
+ fail:
+	/*
+	 * We can't read in the inode this bucket points to, or this inode
+	 * is messed up.  Just ditch this bucket of inodes.  We will lose
+	 * some inodes and space, but at least we won't hang.
+	 *
+	 * Call xlog_recover_clear_agi_bucket() to perform a transaction to
+	 * clear the inode pointer in the bucket.
+	 */
+	xlog_recover_clear_agi_bucket(mp, agno, bucket);
+	return NULLAGINO;
+}
+
 /*
  * xlog_iunlink_recover
  *
@@ -3167,11 +3231,7 @@ xlog_recover_process_iunlinks(
 	xfs_agnumber_t	agno;
 	xfs_agi_t	*agi;
 	xfs_buf_t	*agibp;
-	xfs_buf_t	*ibp;
-	xfs_dinode_t	*dip;
-	xfs_inode_t	*ip;
 	xfs_agino_t	agino;
-	xfs_ino_t	ino;
 	int		bucket;
 	int		error;
 	uint		mp_dmevmask;
@@ -3205,10 +3265,8 @@ xlog_recover_process_iunlinks(
 		agi = XFS_BUF_TO_AGI(agibp);
 
 		for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
-
 			agino = be32_to_cpu(agi->agi_unlinked[bucket]);
 			while (agino != NULLAGINO) {
-
 				/*
 				 * Release the agi buffer so that it can
 				 * be acquired in the normal course of the
@@ -3216,68 +3274,8 @@ xlog_recover_process_iunlinks(
 				 */
 				xfs_buf_relse(agibp);
 
-				ino = XFS_AGINO_TO_INO(mp, agno, agino);
-				error = xfs_iget(mp, NULL, ino, 0, 0, &ip, 0);
-				ASSERT(error || (ip != NULL));
-
-				if (!error) {
-					/*
-					 * Get the on disk inode to find the
-					 * next inode in the bucket.
-					 */
-					error = xfs_itobp(mp, NULL, ip, &dip,
-							&ibp, 0, 0,
-							XFS_BUF_LOCK);
-					ASSERT(error || (dip != NULL));
-				}
-
-				if (!error) {
-					ASSERT(ip->i_d.di_nlink == 0);
-
-					/* setup for the next pass */
-					agino = be32_to_cpu(
-							dip->di_next_unlinked);
-					xfs_buf_relse(ibp);
-					/*
-					 * Prevent any DMAPI event from
-					 * being sent when the
-					 * reference on the inode is
-					 * dropped.
-					 */
-					ip->i_d.di_dmevmask = 0;
-
-					/*
-					 * If this is a new inode, handle
-					 * it specially.  Otherwise,
-					 * just drop our reference to the
-					 * inode.  If there are no
-					 * other references, this will
-					 * send the inode to
-					 * xfs_inactive() which will
-					 * truncate the file and free
-					 * the inode.
-					 */
-					if (ip->i_d.di_mode == 0)
-						xfs_iput_new(ip, 0);
-					else
-						IRELE(ip);
-				} else {
-					/*
-					 * We can't read in the inode
-					 * this bucket points to, or
-					 * this inode is messed up.  Just
-					 * ditch this bucket of inodes.  We
-					 * will lose some inodes and space,
-					 * but at least we won't hang.  Call
-					 * xlog_recover_clear_agi_bucket()
-					 * to perform a transaction to clear
-					 * the inode pointer in the bucket.
-					 */
-					xlog_recover_clear_agi_bucket(mp, agno,
-							bucket);
-
-					agino = NULLAGINO;
-				}
+				agino = xlog_recover_process_one_iunlink(mp,
+							agno, agino, bucket);
 
 				/*
 				 * Reacquire the agibuffer and continue around

-- 

Re: [PATCH 7/7] split up xlog_recover_process_iunlinks

[Dave Chinner]

On Mon, Oct 27, 2008 at 09:39:38AM -0400, Christoph Hellwig wrote:
> Split out the body of the main loop into a separate helper to make the
> code readable.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

.....

> Index: linux-2.6-xfs/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_log_recover.c	2008-10-25 13:22:29.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_log_recover.c	2008-10-25 13:26:44.000000000 +0200
> @@ -3147,6 +3147,70 @@ out_error:
>  	return;
>  }
>  
> +STATIC xfs_agino_t
> +xlog_recover_process_one_iunlink(
> +	struct xfs_mount		*mp,
> +	xfs_agnumber_t			agno,
> +	xfs_agino_t			agino,
> +	int				bucket)
> +{
> +	struct xfs_buf			*ibp;
> +	struct xfs_dinode		*dip;
> +	struct xfs_inode		*ip;
> +	xfs_ino_t			ino;
> +	int				error;
> +
> +	ino = XFS_AGINO_TO_INO(mp, agno, agino);
> +	error = xfs_iget(mp, NULL, ino, 0, 0, &ip, 0);
> +	if (error)
> +		goto fail;
> +
> +	/*
> +	 * Get the on disk inode to find the next inode in the bucket.
> +	 */
> +	ASSERT(ip != NULL);
> +	error = xfs_itobp(mp, NULL, ip, &dip, &ibp, 0, 0, XFS_BUF_LOCK);
> +	if (error)
> +		goto fail;

Jumping to 'fail' at this point leaks the xfs_inode returned from
xfs_iget(). Hmmmm - it appears the original code leaked too....

Also, I don't think we need the assert for ip, either.  If it's NULL
then we'll panic immediately on entering xfs_itobp().

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] split up xlog_recover_process_iunlinks

[Christoph Hellwig]

> > +	error = xfs_iget(mp, NULL, ino, 0, 0, &ip, 0);
> > +	if (error)
> > +		goto fail;
> > +
> > +	/*
> > +	 * Get the on disk inode to find the next inode in the bucket.
> > +	 */
> > +	ASSERT(ip != NULL);
> > +	error = xfs_itobp(mp, NULL, ip, &dip, &ibp, 0, 0, XFS_BUF_LOCK);
> > +	if (error)
> > +		goto fail;
> 
> Jumping to 'fail' at this point leaks the xfs_inode returned from
> xfs_iget(). Hmmmm - it appears the original code leaked too....
> 
> Also, I don't think we need the assert for ip, either.  If it's NULL
> then we'll panic immediately on entering xfs_itobp().

Yeah.  I'll fix this in a separate patch as this one is just suposed to
be purely refactoring.

Re: [PATCH 7/7] split up xlog_recover_process_iunlinks

[Dave Chinner]

On Tue, Oct 28, 2008 at 05:14:46AM -0400, Christoph Hellwig wrote:
> > > +	error = xfs_iget(mp, NULL, ino, 0, 0, &ip, 0);
> > > +	if (error)
> > > +		goto fail;
> > > +
> > > +	/*
> > > +	 * Get the on disk inode to find the next inode in the bucket.
> > > +	 */
> > > +	ASSERT(ip != NULL);
> > > +	error = xfs_itobp(mp, NULL, ip, &dip, &ibp, 0, 0, XFS_BUF_LOCK);
> > > +	if (error)
> > > +		goto fail;
> > 
> > Jumping to 'fail' at this point leaks the xfs_inode returned from
> > xfs_iget(). Hmmmm - it appears the original code leaked too....
> > 
> > Also, I don't think we need the assert for ip, either.  If it's NULL
> > then we'll panic immediately on entering xfs_itobp().
> 
> Yeah.  I'll fix this in a separate patch as this one is just suposed to
> be purely refactoring.

Ok, sounds fair.

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com