[PATCH] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()

[PATCH] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()

[Jan Kara]

When an IO error happens during inode deletion run from
xlog_recover_process_iunlinks() filesystem gets shutdown. Thus any subsequent
attempt to read buffers fails. Code in xlog_recover_process_iunlinks() does not
count with the fact that read of a buffer which was read a while ago can
really fail which results in the oops on
  agi = XFS_BUF_TO_AGI(agibp);

Fix the problem by handling error from xfs_read_agi() in all cases.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_log_recover.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 0ed9ee7..3899264 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3178,11 +3178,17 @@ xlog_recover_process_iunlinks(
 
 				/*
 				 * Reacquire the agibuffer and continue around
-				 * the loop. This should never fail as we know
-				 * the buffer was good earlier on.
+				 * the loop.
 				 */
 				error = xfs_read_agi(mp, NULL, agno, &agibp);
-				ASSERT(error == 0);
+				/*
+				 * We failed to read a buffer we succeeded
+				 * reading just a while ago. Likely because the
+				 * filesystem is shutdown now. Just try the
+				 * next AG.
+				 */
+				if (error)
+					goto next_ag;
 				agi = XFS_BUF_TO_AGI(agibp);
 			}
 		}
@@ -3192,6 +3198,9 @@ xlog_recover_process_iunlinks(
 		 * go on to the next one.
 		 */
 		xfs_buf_relse(agibp);
+next_ag:
+		/* Hrm... C9x seems to require this */
+		;
 	}
 
 	mp->m_dmevmask = mp_dmevmask;
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()

[Dave Chinner]

On Tue, Mar 06, 2012 at 12:00:16PM +0100, Jan Kara wrote:
> When an IO error happens during inode deletion run from
> xlog_recover_process_iunlinks() filesystem gets shutdown. Thus any subsequent
> attempt to read buffers fails. Code in xlog_recover_process_iunlinks() does not
> count with the fact that read of a buffer which was read a while ago can
> really fail which results in the oops on
>   agi = XFS_BUF_TO_AGI(agibp);
> 
> Fix the problem by handling error from xfs_read_agi() in all cases.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/xfs/xfs_log_recover.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 0ed9ee7..3899264 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3178,11 +3178,17 @@ xlog_recover_process_iunlinks(
>  
>  				/*
>  				 * Reacquire the agibuffer and continue around
> -				 * the loop. This should never fail as we know
> -				 * the buffer was good earlier on.
> +				 * the loop.
>  				 */
>  				error = xfs_read_agi(mp, NULL, agno, &agibp);
> -				ASSERT(error == 0);
> +				/*
> +				 * We failed to read a buffer we succeeded
> +				 * reading just a while ago. Likely because the
> +				 * filesystem is shutdown now. Just try the
> +				 * next AG.
> +				 */
> +				if (error)
> +					goto next_ag;
>  				agi = XFS_BUF_TO_AGI(agibp);
>  			}
>  		}

That function is full of ugly code. We don't need to continually
lock and unlock the AGI in the inner loop. Indeed, we probably don't
even need to lock the buffer to read the AGI bucket entries because
we aren't going to be racing with anyone here. Hence all we really
need is an extra hold on the agi buffer to make sure it doesn't go
away once we've dropped the lock via xfs_buf_relse(). i.e.


	/*
	 * take an extra reference to the buffer and then release it
	 * to drop the lock so that it can be acquired in the normal
	 * course of the transaction to truncate and free each
	 * inode.  Because we are not racing with anyone else here
	 * for the AGI buffer, we don't even need to hold it locked
	 * to read the initial unlinked bucket entries out of the
	 * buffer.
	 */
	agi = XFS_BUF_TO_AGI(agibp);
	xfs_buf_hold(agibp);
	xfs_buf_relse(agibp);
	for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
		agino = be32_to_cpu(agi->agi_unlinked[bucket]);
		while (agino != NULLAGINO) {
			agino = xlog_recover_process_one_iunlink(mp,
					agno, agino, bucket);
		}
	}
	xfs_buf_rele(agibp)


Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]

On Wed 07-03-12 12:17:16, Dave Chinner wrote:
> On Tue, Mar 06, 2012 at 12:00:16PM +0100, Jan Kara wrote:
> > When an IO error happens during inode deletion run from
> > xlog_recover_process_iunlinks() filesystem gets shutdown. Thus any subsequent
> > attempt to read buffers fails. Code in xlog_recover_process_iunlinks() does not
> > count with the fact that read of a buffer which was read a while ago can
> > really fail which results in the oops on
> >   agi = XFS_BUF_TO_AGI(agibp);
> > 
> > Fix the problem by handling error from xfs_read_agi() in all cases.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/xfs/xfs_log_recover.c |   15 ++++++++++++---
> >  1 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 0ed9ee7..3899264 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -3178,11 +3178,17 @@ xlog_recover_process_iunlinks(
> >  
> >  				/*
> >  				 * Reacquire the agibuffer and continue around
> > -				 * the loop. This should never fail as we know
> > -				 * the buffer was good earlier on.
> > +				 * the loop.
> >  				 */
> >  				error = xfs_read_agi(mp, NULL, agno, &agibp);
> > -				ASSERT(error == 0);
> > +				/*
> > +				 * We failed to read a buffer we succeeded
> > +				 * reading just a while ago. Likely because the
> > +				 * filesystem is shutdown now. Just try the
> > +				 * next AG.
> > +				 */
> > +				if (error)
> > +					goto next_ag;
> >  				agi = XFS_BUF_TO_AGI(agibp);
> >  			}
> >  		}
> 
> That function is full of ugly code. We don't need to continually
> lock and unlock the AGI in the inner loop. Indeed, we probably don't
> even need to lock the buffer to read the AGI bucket entries because
> we aren't going to be racing with anyone here. Hence all we really
> need is an extra hold on the agi buffer to make sure it doesn't go
> away once we've dropped the lock via xfs_buf_relse(). i.e.
> 
> 
> 	/*
> 	 * take an extra reference to the buffer and then release it
> 	 * to drop the lock so that it can be acquired in the normal
> 	 * course of the transaction to truncate and free each
> 	 * inode.  Because we are not racing with anyone else here
> 	 * for the AGI buffer, we don't even need to hold it locked
> 	 * to read the initial unlinked bucket entries out of the
> 	 * buffer.
> 	 */
> 	agi = XFS_BUF_TO_AGI(agibp);
> 	xfs_buf_hold(agibp);
> 	xfs_buf_relse(agibp);
> 	for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
> 		agino = be32_to_cpu(agi->agi_unlinked[bucket]);
> 		while (agino != NULLAGINO) {
> 			agino = xlog_recover_process_one_iunlink(mp,
> 					agno, agino, bucket);
> 		}
> 	}
> 	xfs_buf_rele(agibp)
  Thanks for review Dave. So something like attached patch?

								Honza

[-- Attachment #2: 0001-xfs-Fix-oops-on-IO-error-during-xlog_recover_process.patch --]
[-- Type: text/x-patch, Size: 2653 bytes --]

>From 276d5ecac71d9e6ec6ac970e594f5a49450d07e2 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 6 Mar 2012 11:39:48 +0100
Subject: [PATCH v2] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()

When an IO error happens during inode deletion run from
xlog_recover_process_iunlinks() filesystem gets shutdown. Thus any subsequent
attempt to read buffers fails. Code in xlog_recover_process_iunlinks() does not
count with the fact that read of a buffer which was read a while ago can
really fail which results in the oops on
  agi = XFS_BUF_TO_AGI(agibp);

Fix the problem by cleaning up the buffer handling in
xlog_recover_process_iunlinks(). We release buffer lock but keep buffer
reference to AG buffer. That is enough for buffer to not go away under us
and we don't have to call xfs_read_agi() all the time.

CC: stable@kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_log_recover.c |   34 ++++++++++++----------------------
 1 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 0ed9ee7..0827644 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3161,37 +3161,27 @@ xlog_recover_process_iunlinks(
 			 */
 			continue;
 		}
+		/*
+		 * Take an extra reference to the buffer and then release it
+		 * to drop the lock so that it can be acquired in the normal
+		 * course of the transaction to truncate and free each
+		 * inode.  Because we are not racing with anyone else here
+		 * for the AGI buffer, we don't even need to hold it locked
+		 * to read the initial unlinked bucket entries out of the
+		 * buffer.
+		 */
 		agi = XFS_BUF_TO_AGI(agibp);
+		xfs_buf_hold(agibp);
+		xfs_buf_relse(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
-				 * transaction to truncate and free the inode.
-				 */
-				xfs_buf_relse(agibp);
-
 				agino = xlog_recover_process_one_iunlink(mp,
 							agno, agino, bucket);
-
-				/*
-				 * Reacquire the agibuffer and continue around
-				 * the loop. This should never fail as we know
-				 * the buffer was good earlier on.
-				 */
-				error = xfs_read_agi(mp, NULL, agno, &agibp);
-				ASSERT(error == 0);
-				agi = XFS_BUF_TO_AGI(agibp);
 			}
 		}
-
-		/*
-		 * Release the buffer for the current agi so we can
-		 * go on to the next one.
-		 */
-		xfs_buf_relse(agibp);
+		xfs_buf_rele(agibp);
 	}
 
 	mp->m_dmevmask = mp_dmevmask;
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs