[10/13] xfs: re-order AGI updates in unlink list updates
diff mbox series

Message ID 20200812092556.2567285-11-david@fromorbit.com
State Accepted
Headers show
Series
  • xfs: in memory inode unlink log items
Related show

Commit Message

Dave Chinner Aug. 12, 2020, 9:25 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

We always access and check the AGI bucket entry for the unlinked
list even if we are not going to need it either for lookup or remove
purposes. Move the code that accesses the AGI to the code that
modifes the AGI, hence keeping the AGI accesses local to the code
that needs to modify it.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode.c | 84 ++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 56 deletions(-)

Comments

Darrick J. Wong Aug. 19, 2020, 12:29 a.m. UTC | #1
On Wed, Aug 12, 2020 at 07:25:53PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We always access and check the AGI bucket entry for the unlinked
> list even if we are not going to need it either for lookup or remove
> purposes. Move the code that accesses the AGI to the code that
> modifes the AGI, hence keeping the AGI accesses local to the code
> that needs to modify it.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 84 ++++++++++++++++------------------------------
>  1 file changed, 28 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index b098e5df07e7..4f616e1b64dc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1918,44 +1918,53 @@ xfs_inactive(
>   */
>  
>  /*
> - * Point the AGI unlinked bucket at an inode and log the results.  The caller
> - * is responsible for validating the old value.
> + * Point the AGI unlinked bucket at an inode and log the results. The caller
> + * passes in the expected current agino the bucket points at via @cur_agino so
> + * we can validate that we are about to remove the inode we expect to be
> + * removing from the AGI bucket.
>   */
> -STATIC int
> +static int
>  xfs_iunlink_update_bucket(
>  	struct xfs_trans	*tp,
>  	xfs_agnumber_t		agno,
>  	struct xfs_buf		*agibp,
> -	xfs_agino_t		old_agino,
> +	xfs_agino_t		cur_agino,

Hm.  So I think I understand the new role of this function better now
that this patch moves into this function the checking of the bucket
pointer and whatnot.  Would it be difficult to merge this patch with
patch 4?

--D

>  	xfs_agino_t		new_agino)
>  {
> -	struct xlog		*log = tp->t_mountp->m_log;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	struct xlog		*log = mp->m_log;
>  	struct xfs_agi		*agi = agibp->b_addr;
> -	xfs_agino_t		old_value;
> +	xfs_agino_t		old_agino;
>  	unsigned int		bucket_index;
>  	int                     offset;
>  
> -	ASSERT(xfs_verify_agino_or_null(tp->t_mountp, agno, new_agino));
> +	ASSERT(xfs_verify_agino_or_null(mp, agno, new_agino));
>  
> +	/*
> +	 * We don't need to traverse the on disk unlinked list to find the
> +	 * previous inode in the list when removing inodes anymore, so we don't
> +	 * use multiple on-disk lists anymore. Hence we always use bucket 0
> +	 * unless we are in log recovery in which case we might be recovering an
> +	 * old filesystem that has multiple buckets.
> +	 */
>  	bucket_index = 0;
> -	/* During recovery, the old multiple bucket index can be applied */
>  	if (!log || log->l_flags & XLOG_RECOVERY_NEEDED) {
> -		ASSERT(old_agino != NULLAGINO);
> +		ASSERT(cur_agino != NULLAGINO);
>  
> -		if (be32_to_cpu(agi->agi_unlinked[0]) != old_agino)
> -			bucket_index = old_agino % XFS_AGI_UNLINKED_BUCKETS;
> +		if (be32_to_cpu(agi->agi_unlinked[0]) != cur_agino)
> +			bucket_index = cur_agino % XFS_AGI_UNLINKED_BUCKETS;
>  	}
>  
> -	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> -	trace_xfs_iunlink_update_bucket(tp->t_mountp, agno, bucket_index,
> -			old_value, new_agino);
> -
> -	/* check if the old agi_unlinked head is as expected */
> -	if (old_value != old_agino) {
> +	old_agino = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> +	if (new_agino == old_agino || cur_agino != old_agino ||
> +	    !xfs_verify_agino_or_null(mp, agno, old_agino)) {
>  		xfs_buf_mark_corrupt(agibp);
>  		return -EFSCORRUPTED;
>  	}
>  
> +	trace_xfs_iunlink_update_bucket(mp, agno, bucket_index,
> +			old_agino, new_agino);
> +
>  	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
>  	offset = offsetof(struct xfs_agi, agi_unlinked) +
>  			(sizeof(xfs_agino_t) * bucket_index);
> @@ -2032,44 +2041,25 @@ xfs_iunlink_insert_inode(
>  	struct xfs_inode	*ip)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
> -	struct xfs_agi		*agi;
>  	struct xfs_inode	*nip;
> -	xfs_agino_t		next_agino;
> +	xfs_agino_t		next_agino = NULLAGINO;
>  	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
>  	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
>  	int			error;
>  
> -	agi = agibp->b_addr;
> -
> -	/*
> -	 * We don't need to traverse the on disk unlinked list to find the
> -	 * previous inode in the list when removing inodes anymore, so we don't
> -	 * need multiple on-disk lists anymore. Hence we always use bucket 0.
> -	 * Make sure the pointer isn't garbage and that this inode isn't already
> -	 * on the list.
> -	 */
> -	next_agino = be32_to_cpu(agi->agi_unlinked[0]);
> -	if (next_agino == agino ||
> -	    !xfs_verify_agino_or_null(mp, agno, next_agino)) {
> -		xfs_buf_mark_corrupt(agibp);
> -		return -EFSCORRUPTED;
> -	}
> -
>  	nip = list_first_entry_or_null(&agibp->b_pag->pag_ici_unlink_list,
>  					struct xfs_inode, i_unlink);
>  	if (nip) {
> -		ASSERT(next_agino == XFS_INO_TO_AGINO(mp, nip->i_ino));
>  
>  		/*
>  		 * There is already another inode in the bucket, so point this
>  		 * inode to the current head of the list.
>  		 */
> +		next_agino = XFS_INO_TO_AGINO(mp, nip->i_ino);
>  		error = xfs_iunlink_update_inode(tp, ip, agno, NULLAGINO,
>  						 next_agino);
>  		if (error)
>  			return error;
> -	} else {
> -		ASSERT(next_agino == NULLAGINO);
>  	}
>  
>  	/* Point the head of the list to point to this inode. */
> @@ -2122,28 +2112,11 @@ xfs_iunlink_remove_inode(
>  	struct xfs_inode	*ip)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
> -	struct xfs_agi		*agi;
>  	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
>  	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
>  	xfs_agino_t		next_agino = NULLAGINO;
> -	xfs_agino_t		head_agino;
>  	int			error;
>  
> -	agi = agibp->b_addr;
> -
> -	/*
> -	 * We don't need to traverse the on disk unlinked list to find the
> -	 * previous inode in the list when removing inodes anymore, so we don't
> -	 * need multiple on-disk lists anymore. Hence we always use bucket 0.
> -	 * Make sure the head pointer isn't garbage.
> -	 */
> -	head_agino = be32_to_cpu(agi->agi_unlinked[0]);
> -	if (!xfs_verify_agino(mp, agno, head_agino)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> -				agi, sizeof(*agi));
> -		return -EFSCORRUPTED;
> -	}
> -
>  	/*
>  	 * Get the next agino in the list. If we are at the end of the list,
>  	 * then the previous inode's i_next_unlinked filed will get cleared.
> @@ -2165,7 +2138,6 @@ xfs_iunlink_remove_inode(
>  					struct xfs_inode, i_unlink)) {
>  		struct xfs_inode *pip = list_prev_entry(ip, i_unlink);
>  
> -		ASSERT(head_agino != agino);
>  		return xfs_iunlink_update_inode(tp, pip, agno, agino,
>  						next_agino);
>  	}
> -- 
> 2.26.2.761.g0e0b3e54be
>
Dave Chinner Aug. 19, 2020, 1:01 a.m. UTC | #2
On Tue, Aug 18, 2020 at 05:29:48PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 12, 2020 at 07:25:53PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We always access and check the AGI bucket entry for the unlinked
> > list even if we are not going to need it either for lookup or remove
> > purposes. Move the code that accesses the AGI to the code that
> > modifes the AGI, hence keeping the AGI accesses local to the code
> > that needs to modify it.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 84 ++++++++++++++++------------------------------
> >  1 file changed, 28 insertions(+), 56 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index b098e5df07e7..4f616e1b64dc 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1918,44 +1918,53 @@ xfs_inactive(
> >   */
> >  
> >  /*
> > - * Point the AGI unlinked bucket at an inode and log the results.  The caller
> > - * is responsible for validating the old value.
> > + * Point the AGI unlinked bucket at an inode and log the results. The caller
> > + * passes in the expected current agino the bucket points at via @cur_agino so
> > + * we can validate that we are about to remove the inode we expect to be
> > + * removing from the AGI bucket.
> >   */
> > -STATIC int
> > +static int
> >  xfs_iunlink_update_bucket(
> >  	struct xfs_trans	*tp,
> >  	xfs_agnumber_t		agno,
> >  	struct xfs_buf		*agibp,
> > -	xfs_agino_t		old_agino,
> > +	xfs_agino_t		cur_agino,
> 
> Hm.  So I think I understand the new role of this function better now
> that this patch moves into this function the checking of the bucket
> pointer and whatnot.  Would it be difficult to merge this patch with
> patch 4?

I really didn't want to remove the code that used the "head_agino"
for verification until I had moved all the list traversal
functionality to use the in memory unlinked list and had verified
that was correct....

I think merging them it could be done, but it will most likely
result in having to rebase and retest every subsequent patch...

Cheers,

Dave.

Patch
diff mbox series

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index b098e5df07e7..4f616e1b64dc 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1918,44 +1918,53 @@  xfs_inactive(
  */
 
 /*
- * Point the AGI unlinked bucket at an inode and log the results.  The caller
- * is responsible for validating the old value.
+ * Point the AGI unlinked bucket at an inode and log the results. The caller
+ * passes in the expected current agino the bucket points at via @cur_agino so
+ * we can validate that we are about to remove the inode we expect to be
+ * removing from the AGI bucket.
  */
-STATIC int
+static int
 xfs_iunlink_update_bucket(
 	struct xfs_trans	*tp,
 	xfs_agnumber_t		agno,
 	struct xfs_buf		*agibp,
-	xfs_agino_t		old_agino,
+	xfs_agino_t		cur_agino,
 	xfs_agino_t		new_agino)
 {
-	struct xlog		*log = tp->t_mountp->m_log;
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xlog		*log = mp->m_log;
 	struct xfs_agi		*agi = agibp->b_addr;
-	xfs_agino_t		old_value;
+	xfs_agino_t		old_agino;
 	unsigned int		bucket_index;
 	int                     offset;
 
-	ASSERT(xfs_verify_agino_or_null(tp->t_mountp, agno, new_agino));
+	ASSERT(xfs_verify_agino_or_null(mp, agno, new_agino));
 
+	/*
+	 * We don't need to traverse the on disk unlinked list to find the
+	 * previous inode in the list when removing inodes anymore, so we don't
+	 * use multiple on-disk lists anymore. Hence we always use bucket 0
+	 * unless we are in log recovery in which case we might be recovering an
+	 * old filesystem that has multiple buckets.
+	 */
 	bucket_index = 0;
-	/* During recovery, the old multiple bucket index can be applied */
 	if (!log || log->l_flags & XLOG_RECOVERY_NEEDED) {
-		ASSERT(old_agino != NULLAGINO);
+		ASSERT(cur_agino != NULLAGINO);
 
-		if (be32_to_cpu(agi->agi_unlinked[0]) != old_agino)
-			bucket_index = old_agino % XFS_AGI_UNLINKED_BUCKETS;
+		if (be32_to_cpu(agi->agi_unlinked[0]) != cur_agino)
+			bucket_index = cur_agino % XFS_AGI_UNLINKED_BUCKETS;
 	}
 
-	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
-	trace_xfs_iunlink_update_bucket(tp->t_mountp, agno, bucket_index,
-			old_value, new_agino);
-
-	/* check if the old agi_unlinked head is as expected */
-	if (old_value != old_agino) {
+	old_agino = be32_to_cpu(agi->agi_unlinked[bucket_index]);
+	if (new_agino == old_agino || cur_agino != old_agino ||
+	    !xfs_verify_agino_or_null(mp, agno, old_agino)) {
 		xfs_buf_mark_corrupt(agibp);
 		return -EFSCORRUPTED;
 	}
 
+	trace_xfs_iunlink_update_bucket(mp, agno, bucket_index,
+			old_agino, new_agino);
+
 	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
 	offset = offsetof(struct xfs_agi, agi_unlinked) +
 			(sizeof(xfs_agino_t) * bucket_index);
@@ -2032,44 +2041,25 @@  xfs_iunlink_insert_inode(
 	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
-	struct xfs_agi		*agi;
 	struct xfs_inode	*nip;
-	xfs_agino_t		next_agino;
+	xfs_agino_t		next_agino = NULLAGINO;
 	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
 	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
 	int			error;
 
-	agi = agibp->b_addr;
-
-	/*
-	 * We don't need to traverse the on disk unlinked list to find the
-	 * previous inode in the list when removing inodes anymore, so we don't
-	 * need multiple on-disk lists anymore. Hence we always use bucket 0.
-	 * Make sure the pointer isn't garbage and that this inode isn't already
-	 * on the list.
-	 */
-	next_agino = be32_to_cpu(agi->agi_unlinked[0]);
-	if (next_agino == agino ||
-	    !xfs_verify_agino_or_null(mp, agno, next_agino)) {
-		xfs_buf_mark_corrupt(agibp);
-		return -EFSCORRUPTED;
-	}
-
 	nip = list_first_entry_or_null(&agibp->b_pag->pag_ici_unlink_list,
 					struct xfs_inode, i_unlink);
 	if (nip) {
-		ASSERT(next_agino == XFS_INO_TO_AGINO(mp, nip->i_ino));
 
 		/*
 		 * There is already another inode in the bucket, so point this
 		 * inode to the current head of the list.
 		 */
+		next_agino = XFS_INO_TO_AGINO(mp, nip->i_ino);
 		error = xfs_iunlink_update_inode(tp, ip, agno, NULLAGINO,
 						 next_agino);
 		if (error)
 			return error;
-	} else {
-		ASSERT(next_agino == NULLAGINO);
 	}
 
 	/* Point the head of the list to point to this inode. */
@@ -2122,28 +2112,11 @@  xfs_iunlink_remove_inode(
 	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
-	struct xfs_agi		*agi;
 	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
 	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
 	xfs_agino_t		next_agino = NULLAGINO;
-	xfs_agino_t		head_agino;
 	int			error;
 
-	agi = agibp->b_addr;
-
-	/*
-	 * We don't need to traverse the on disk unlinked list to find the
-	 * previous inode in the list when removing inodes anymore, so we don't
-	 * need multiple on-disk lists anymore. Hence we always use bucket 0.
-	 * Make sure the head pointer isn't garbage.
-	 */
-	head_agino = be32_to_cpu(agi->agi_unlinked[0]);
-	if (!xfs_verify_agino(mp, agno, head_agino)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-				agi, sizeof(*agi));
-		return -EFSCORRUPTED;
-	}
-
 	/*
 	 * Get the next agino in the list. If we are at the end of the list,
 	 * then the previous inode's i_next_unlinked filed will get cleared.
@@ -2165,7 +2138,6 @@  xfs_iunlink_remove_inode(
 					struct xfs_inode, i_unlink)) {
 		struct xfs_inode *pip = list_prev_entry(ip, i_unlink);
 
-		ASSERT(head_agino != agino);
 		return xfs_iunlink_update_inode(tp, pip, agno, agino,
 						next_agino);
 	}