[3/8] xfs: refactor AGI unlinked bucket updates

Commit Message

Darrick J. Wong Jan. 31, 2019, 11:18 p.m. UTC

From: Darrick J. Wong <[e-mail]>

Split the AGI unlinked bucket updates into a separate function.  No
functional changes.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/xfs_inode.c |   69 ++++++++++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_trace.h |   26 ++++++++++++++++++++
 2 files changed, 76 insertions(+), 19 deletions(-)

Comments

Brian Foster Feb. 1, 2019, 7 p.m. UTC | #1

On Thu, Jan 31, 2019 at 03:18:09PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> Split the AGI unlinked bucket updates into a separate function.  No
> functional changes.
> 
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  fs/xfs/xfs_inode.c |   69 ++++++++++++++++++++++++++++++++++++++--------------
>  fs/xfs/xfs_trace.h |   26 ++++++++++++++++++++
>  2 files changed, 76 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 98355f5f9253..3c33136b21ef 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1880,6 +1880,46 @@ xfs_inactive(
>  	xfs_qm_dqdetach(ip);
>  }
>  
> +/*
> + * Point the AGI unlinked bucket at an inode and log the results.  The caller
> + * is responsible for validating the old value.
> + */
> +STATIC int
> +xfs_iunlink_update_bucket(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*agibp,
> +	unsigned int		bucket_index,
> +	xfs_agino_t		new_agino)
> +{
> +	struct xfs_agi		*agi = XFS_BUF_TO_AGI(agibp);
> +	xfs_agino_t		old_value;
> +	int			offset;
> +
> +	ASSERT(new_agino == NULLAGINO ||
> +	       xfs_verify_agino(tp->t_mountp, be32_to_cpu(agi->agi_seqno),
> +				new_agino));
> +
> +	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> +	trace_xfs_iunlink_update_bucket(tp->t_mountp,
> +			be32_to_cpu(agi->agi_seqno), bucket_index, old_value,
> +			new_agino);
> +

Might as well pass agno as a param, particularly since the callers
already have it.

> +	/*
> +	 * We should never find the head of the list already set to the value
> +	 * passed in because either we're adding or removing ourselves from the
> +	 * head of the list.
> +	 */
> +	if (old_value == new_agino)
> +		return -EFSCORRUPTED;
> +

This seems a little weird in the NULLAGINO case. That probably should
still never happen, but doesn't seem indicative of corruption on its
own. *shrug* not a big deal.

Brian

> +	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
> +	offset = offsetof(struct xfs_agi, agi_unlinked) +
> +			(sizeof(xfs_agino_t) * bucket_index);
> +	xfs_trans_log_buf(tp, agibp, offset,
> +			(offset + sizeof(xfs_agino_t) - 1));
> +	return 0;
> +}
> +
>  /*
>   * This is called when the inode's link count goes to 0 or we are creating a
>   * tmpfile via O_TMPFILE. In the case of a tmpfile, @ignore_linkcount will be
> @@ -1958,15 +1998,10 @@ xfs_iunlink(
>  		xfs_inobp_check(mp, ibp);
>  	}
>  
> -	/*
> -	 * Point the bucket head pointer at the inode being inserted.
> -	 */
> -	ASSERT(agino != 0);
> -	agi->agi_unlinked[bucket_index] = cpu_to_be32(agino);
> -	offset = offsetof(xfs_agi_t, agi_unlinked) +
> -		(sizeof(xfs_agino_t) * bucket_index);
> -	xfs_trans_log_buf(tp, agibp, offset,
> -			  (offset + sizeof(xfs_agino_t) - 1));
> +	/* Point the head of the list to point to this inode. */
> +	error = xfs_iunlink_update_bucket(tp, agibp, bucket_index, agino);
> +	if (error)
> +		goto out_unlock;
>  	pag->pagi_unlinked_count++;
>  
>  out_unlock:
> @@ -2062,16 +2097,12 @@ xfs_iunlink_remove(
>  		} else {
>  			xfs_trans_brelse(tp, ibp);
>  		}
> -		/*
> -		 * Point the bucket head pointer at the next inode.
> -		 */
> -		ASSERT(next_agino != 0);
> -		ASSERT(next_agino != agino);
> -		agi->agi_unlinked[bucket_index] = cpu_to_be32(next_agino);
> -		offset = offsetof(xfs_agi_t, agi_unlinked) +
> -			(sizeof(xfs_agino_t) * bucket_index);
> -		xfs_trans_log_buf(tp, agibp, offset,
> -				  (offset + sizeof(xfs_agino_t) - 1));
> +
> +		/* Point the head of the list to the next unlinked inode. */
> +		error = xfs_iunlink_update_bucket(tp, agibp, bucket_index,
> +				next_agino);
> +		if (error)
> +			goto out_unlock;
>  	} else {
>  		/*
>  		 * We need to search the list for the inode being freed.
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 6fcc893dfc91..c10478e7e49a 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3371,6 +3371,32 @@ DEFINE_TRANS_EVENT(xfs_trans_roll);
>  DEFINE_TRANS_EVENT(xfs_trans_add_item);
>  DEFINE_TRANS_EVENT(xfs_trans_free_items);
>  
> +TRACE_EVENT(xfs_iunlink_update_bucket,
> +	TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, unsigned int bucket,
> +		 xfs_agino_t old_ptr, xfs_agino_t new_ptr),
> +	TP_ARGS(mp, agno, bucket, old_ptr, new_ptr),
> +	TP_STRUCT__entry(
> +		__field(dev_t, dev)
> +		__field(xfs_agnumber_t, agno)
> +		__field(unsigned int, bucket)
> +		__field(xfs_agino_t, old_ptr)
> +		__field(xfs_agino_t, new_ptr)
> +	),
> +	TP_fast_assign(
> +		__entry->dev = mp->m_super->s_dev;
> +		__entry->agno = agno;
> +		__entry->bucket = bucket;
> +		__entry->old_ptr = old_ptr;
> +		__entry->new_ptr = new_ptr;
> +	),
> +	TP_printk("dev %d:%d agno %u bucket %u old 0x%x new 0x%x",
> +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> +		  __entry->agno,
> +		  __entry->bucket,
> +		  __entry->old_ptr,
> +		  __entry->new_ptr)
> +);
> +
>  #endif /* _TRACE_XFS_H */
>  
>  #undef TRACE_INCLUDE_PATH
>

Christoph Hellwig Feb. 2, 2019, 4:21 p.m. UTC | #2

> +	struct xfs_agi		*agi = XFS_BUF_TO_AGI(agibp);
> +	xfs_agino_t		old_value;
> +	int			offset;
> +
> +	ASSERT(new_agino == NULLAGINO ||
> +	       xfs_verify_agino(tp->t_mountp, be32_to_cpu(agi->agi_seqno),
> +				new_agino));
> +
> +	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> +	trace_xfs_iunlink_update_bucket(tp->t_mountp,
> +			be32_to_cpu(agi->agi_seqno), bucket_index, old_value,
> +			new_agino);

As already mentioned by Brian I'd rather pass the agno rather than
recalculating it twice.

> +	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
> +	offset = offsetof(struct xfs_agi, agi_unlinked) +
> +			(sizeof(xfs_agino_t) * bucket_index);
> +	xfs_trans_log_buf(tp, agibp, offset,
> +			(offset + sizeof(xfs_agino_t) - 1));

No need for the braces here.

Darrick J. Wong Feb. 2, 2019, 7:50 p.m. UTC | #3

On Fri, Feb 01, 2019 at 02:00:43PM -0500, Brian Foster wrote:
> On Thu, Jan 31, 2019 at 03:18:09PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <[e-mail]>
> > 
> > Split the AGI unlinked bucket updates into a separate function.  No
> > functional changes.
> > 
> > Signed-off-by: Darrick J. Wong <[e-mail]>
> > ---
> >  fs/xfs/xfs_inode.c |   69 ++++++++++++++++++++++++++++++++++++++--------------
> >  fs/xfs/xfs_trace.h |   26 ++++++++++++++++++++
> >  2 files changed, 76 insertions(+), 19 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 98355f5f9253..3c33136b21ef 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1880,6 +1880,46 @@ xfs_inactive(
> >  	xfs_qm_dqdetach(ip);
> >  }
> >  
> > +/*
> > + * Point the AGI unlinked bucket at an inode and log the results.  The caller
> > + * is responsible for validating the old value.
> > + */
> > +STATIC int
> > +xfs_iunlink_update_bucket(
> > +	struct xfs_trans	*tp,
> > +	struct xfs_buf		*agibp,
> > +	unsigned int		bucket_index,
> > +	xfs_agino_t		new_agino)
> > +{
> > +	struct xfs_agi		*agi = XFS_BUF_TO_AGI(agibp);
> > +	xfs_agino_t		old_value;
> > +	int			offset;
> > +
> > +	ASSERT(new_agino == NULLAGINO ||
> > +	       xfs_verify_agino(tp->t_mountp, be32_to_cpu(agi->agi_seqno),
> > +				new_agino));
> > +
> > +	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> > +	trace_xfs_iunlink_update_bucket(tp->t_mountp,
> > +			be32_to_cpu(agi->agi_seqno), bucket_index, old_value,
> > +			new_agino);
> > +
> 
> Might as well pass agno as a param, particularly since the callers
> already have it.

Will fix.

> > +	/*
> > +	 * We should never find the head of the list already set to the value
> > +	 * passed in because either we're adding or removing ourselves from the
> > +	 * head of the list.
> > +	 */
> > +	if (old_value == new_agino)
> > +		return -EFSCORRUPTED;
> > +
> 
> This seems a little weird in the NULLAGINO case. That probably should
> still never happen, but doesn't seem indicative of corruption on its
> own. *shrug* not a big deal.

<nod> We shouldn't ever be pulling the last item off an unlinked bucket
only to find that the bucket was empty, but if we do then it's a sign
that something has gone seriously wrong with our locking or something.

--D

> Brian
> 
> > +	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
> > +	offset = offsetof(struct xfs_agi, agi_unlinked) +
> > +			(sizeof(xfs_agino_t) * bucket_index);
> > +	xfs_trans_log_buf(tp, agibp, offset,
> > +			(offset + sizeof(xfs_agino_t) - 1));
> > +	return 0;
> > +}
> > +
> >  /*
> >   * This is called when the inode's link count goes to 0 or we are creating a
> >   * tmpfile via O_TMPFILE. In the case of a tmpfile, @ignore_linkcount will be
> > @@ -1958,15 +1998,10 @@ xfs_iunlink(
> >  		xfs_inobp_check(mp, ibp);
> >  	}
> >  
> > -	/*
> > -	 * Point the bucket head pointer at the inode being inserted.
> > -	 */
> > -	ASSERT(agino != 0);
> > -	agi->agi_unlinked[bucket_index] = cpu_to_be32(agino);
> > -	offset = offsetof(xfs_agi_t, agi_unlinked) +
> > -		(sizeof(xfs_agino_t) * bucket_index);
> > -	xfs_trans_log_buf(tp, agibp, offset,
> > -			  (offset + sizeof(xfs_agino_t) - 1));
> > +	/* Point the head of the list to point to this inode. */
> > +	error = xfs_iunlink_update_bucket(tp, agibp, bucket_index, agino);
> > +	if (error)
> > +		goto out_unlock;
> >  	pag->pagi_unlinked_count++;
> >  
> >  out_unlock:
> > @@ -2062,16 +2097,12 @@ xfs_iunlink_remove(
> >  		} else {
> >  			xfs_trans_brelse(tp, ibp);
> >  		}
> > -		/*
> > -		 * Point the bucket head pointer at the next inode.
> > -		 */
> > -		ASSERT(next_agino != 0);
> > -		ASSERT(next_agino != agino);
> > -		agi->agi_unlinked[bucket_index] = cpu_to_be32(next_agino);
> > -		offset = offsetof(xfs_agi_t, agi_unlinked) +
> > -			(sizeof(xfs_agino_t) * bucket_index);
> > -		xfs_trans_log_buf(tp, agibp, offset,
> > -				  (offset + sizeof(xfs_agino_t) - 1));
> > +
> > +		/* Point the head of the list to the next unlinked inode. */
> > +		error = xfs_iunlink_update_bucket(tp, agibp, bucket_index,
> > +				next_agino);
> > +		if (error)
> > +			goto out_unlock;
> >  	} else {
> >  		/*
> >  		 * We need to search the list for the inode being freed.
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index 6fcc893dfc91..c10478e7e49a 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -3371,6 +3371,32 @@ DEFINE_TRANS_EVENT(xfs_trans_roll);
> >  DEFINE_TRANS_EVENT(xfs_trans_add_item);
> >  DEFINE_TRANS_EVENT(xfs_trans_free_items);
> >  
> > +TRACE_EVENT(xfs_iunlink_update_bucket,
> > +	TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, unsigned int bucket,
> > +		 xfs_agino_t old_ptr, xfs_agino_t new_ptr),
> > +	TP_ARGS(mp, agno, bucket, old_ptr, new_ptr),
> > +	TP_STRUCT__entry(
> > +		__field(dev_t, dev)
> > +		__field(xfs_agnumber_t, agno)
> > +		__field(unsigned int, bucket)
> > +		__field(xfs_agino_t, old_ptr)
> > +		__field(xfs_agino_t, new_ptr)
> > +	),
> > +	TP_fast_assign(
> > +		__entry->dev = mp->m_super->s_dev;
> > +		__entry->agno = agno;
> > +		__entry->bucket = bucket;
> > +		__entry->old_ptr = old_ptr;
> > +		__entry->new_ptr = new_ptr;
> > +	),
> > +	TP_printk("dev %d:%d agno %u bucket %u old 0x%x new 0x%x",
> > +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> > +		  __entry->agno,
> > +		  __entry->bucket,
> > +		  __entry->old_ptr,
> > +		  __entry->new_ptr)
> > +);
> > +
> >  #endif /* _TRACE_XFS_H */
> >  
> >  #undef TRACE_INCLUDE_PATH
> >

Darrick J. Wong Feb. 2, 2019, 7:51 p.m. UTC | #4

On Sat, Feb 02, 2019 at 08:21:21AM -0800, Christoph Hellwig wrote:
> > +	struct xfs_agi		*agi = XFS_BUF_TO_AGI(agibp);
> > +	xfs_agino_t		old_value;
> > +	int			offset;
> > +
> > +	ASSERT(new_agino == NULLAGINO ||
> > +	       xfs_verify_agino(tp->t_mountp, be32_to_cpu(agi->agi_seqno),
> > +				new_agino));
> > +
> > +	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> > +	trace_xfs_iunlink_update_bucket(tp->t_mountp,
> > +			be32_to_cpu(agi->agi_seqno), bucket_index, old_value,
> > +			new_agino);
> 
> As already mentioned by Brian I'd rather pass the agno rather than
> recalculating it twice.
> 
> > +	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
> > +	offset = offsetof(struct xfs_agi, agi_unlinked) +
> > +			(sizeof(xfs_agino_t) * bucket_index);
> > +	xfs_trans_log_buf(tp, agibp, offset,
> > +			(offset + sizeof(xfs_agino_t) - 1));
> 
> No need for the braces here.

Will fix.

--D

Patch

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 98355f5f9253..3c33136b21ef 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1880,6 +1880,46 @@  xfs_inactive(
 	xfs_qm_dqdetach(ip);
 }
 
+/*
+ * Point the AGI unlinked bucket at an inode and log the results.  The caller
+ * is responsible for validating the old value.
+ */
+STATIC int
+xfs_iunlink_update_bucket(
+	struct xfs_trans	*tp,
+	struct xfs_buf		*agibp,
+	unsigned int		bucket_index,
+	xfs_agino_t		new_agino)
+{
+	struct xfs_agi		*agi = XFS_BUF_TO_AGI(agibp);
+	xfs_agino_t		old_value;
+	int			offset;
+
+	ASSERT(new_agino == NULLAGINO ||
+	       xfs_verify_agino(tp->t_mountp, be32_to_cpu(agi->agi_seqno),
+				new_agino));
+
+	old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
+	trace_xfs_iunlink_update_bucket(tp->t_mountp,
+			be32_to_cpu(agi->agi_seqno), bucket_index, old_value,
+			new_agino);
+
+	/*
+	 * We should never find the head of the list already set to the value
+	 * passed in because either we're adding or removing ourselves from the
+	 * head of the list.
+	 */
+	if (old_value == new_agino)
+		return -EFSCORRUPTED;
+
+	agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
+	offset = offsetof(struct xfs_agi, agi_unlinked) +
+			(sizeof(xfs_agino_t) * bucket_index);
+	xfs_trans_log_buf(tp, agibp, offset,
+			(offset + sizeof(xfs_agino_t) - 1));
+	return 0;
+}
+
 /*
  * This is called when the inode's link count goes to 0 or we are creating a
  * tmpfile via O_TMPFILE. In the case of a tmpfile, @ignore_linkcount will be
@@ -1958,15 +1998,10 @@  xfs_iunlink(
 		xfs_inobp_check(mp, ibp);
 	}
 
-	/*
-	 * Point the bucket head pointer at the inode being inserted.
-	 */
-	ASSERT(agino != 0);
-	agi->agi_unlinked[bucket_index] = cpu_to_be32(agino);
-	offset = offsetof(xfs_agi_t, agi_unlinked) +
-		(sizeof(xfs_agino_t) * bucket_index);
-	xfs_trans_log_buf(tp, agibp, offset,
-			  (offset + sizeof(xfs_agino_t) - 1));
+	/* Point the head of the list to point to this inode. */
+	error = xfs_iunlink_update_bucket(tp, agibp, bucket_index, agino);
+	if (error)
+		goto out_unlock;
 	pag->pagi_unlinked_count++;
 
 out_unlock:
@@ -2062,16 +2097,12 @@  xfs_iunlink_remove(
 		} else {
 			xfs_trans_brelse(tp, ibp);
 		}
-		/*
-		 * Point the bucket head pointer at the next inode.
-		 */
-		ASSERT(next_agino != 0);
-		ASSERT(next_agino != agino);
-		agi->agi_unlinked[bucket_index] = cpu_to_be32(next_agino);
-		offset = offsetof(xfs_agi_t, agi_unlinked) +
-			(sizeof(xfs_agino_t) * bucket_index);
-		xfs_trans_log_buf(tp, agibp, offset,
-				  (offset + sizeof(xfs_agino_t) - 1));
+
+		/* Point the head of the list to the next unlinked inode. */
+		error = xfs_iunlink_update_bucket(tp, agibp, bucket_index,
+				next_agino);
+		if (error)
+			goto out_unlock;
 	} else {
 		/*
 		 * We need to search the list for the inode being freed.
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 6fcc893dfc91..c10478e7e49a 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3371,6 +3371,32 @@  DEFINE_TRANS_EVENT(xfs_trans_roll);
 DEFINE_TRANS_EVENT(xfs_trans_add_item);
 DEFINE_TRANS_EVENT(xfs_trans_free_items);
 
+TRACE_EVENT(xfs_iunlink_update_bucket,
+	TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, unsigned int bucket,
+		 xfs_agino_t old_ptr, xfs_agino_t new_ptr),
+	TP_ARGS(mp, agno, bucket, old_ptr, new_ptr),
+	TP_STRUCT__entry(
+		__field(dev_t, dev)
+		__field(xfs_agnumber_t, agno)
+		__field(unsigned int, bucket)
+		__field(xfs_agino_t, old_ptr)
+		__field(xfs_agino_t, new_ptr)
+	),
+	TP_fast_assign(
+		__entry->dev = mp->m_super->s_dev;
+		__entry->agno = agno;
+		__entry->bucket = bucket;
+		__entry->old_ptr = old_ptr;
+		__entry->new_ptr = new_ptr;
+	),
+	TP_printk("dev %d:%d agno %u bucket %u old 0x%x new 0x%x",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->agno,
+		  __entry->bucket,
+		  __entry->old_ptr,
+		  __entry->new_ptr)
+);
+
 #endif /* _TRACE_XFS_H */
 
 #undef TRACE_INCLUDE_PATH