diff mbox series

[07/18] xfs: consolidate leaf/node states in xfs_attr_set_iter

Message ID 20220509004138.762556-8-david@fromorbit.com (mailing list archive)
State Accepted, archived
Headers show
Series XFS: LARP state machine and recovery rework | expand

Commit Message

Dave Chinner May 9, 2022, 12:41 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

The operations performed from XFS_DAS_FOUND_LBLK through to
XFS_DAS_RM_LBLK are now identical to XFS_DAS_FOUND_NBLK through to
XFS_DAS_RM_NBLK. We can collapse these down into a single set of
code.

To do this, define the states that leaf and node run through as
separate sets of sequential states. Then as we move to the next
state, we can use increments rather than specific state assignments
to move through the states. This means the state progression is set
by the initial state that enters the series and we don't need to
duplicate the code anymore.

At the exit point of the series we need to select the correct leaf
or node state, but that can also be done by state increment rather
than assignment.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 127 ++++++---------------------------------
 fs/xfs/libxfs/xfs_attr.h |   9 ++-
 2 files changed, 27 insertions(+), 109 deletions(-)

Comments

Darrick J. Wong May 10, 2022, 11:20 p.m. UTC | #1
On Mon, May 09, 2022 at 10:41:27AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The operations performed from XFS_DAS_FOUND_LBLK through to
> XFS_DAS_RM_LBLK are now identical to XFS_DAS_FOUND_NBLK through to
> XFS_DAS_RM_NBLK. We can collapse these down into a single set of
> code.
> 
> To do this, define the states that leaf and node run through as
> separate sets of sequential states. Then as we move to the next
> state, we can use increments rather than specific state assignments
> to move through the states. This means the state progression is set
> by the initial state that enters the series and we don't need to
> duplicate the code anymore.
> 
> At the exit point of the series we need to select the correct leaf
> or node state, but that can also be done by state increment rather
> than assignment.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 127 ++++++---------------------------------
>  fs/xfs/libxfs/xfs_attr.h |   9 ++-
>  2 files changed, 27 insertions(+), 109 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index ab8a884af512..be580da62f08 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -394,6 +394,7 @@ xfs_attr_set_iter(
>  	struct xfs_mount		*mp = args->dp->i_mount;
>  
>  	/* State machine switch */
> +next_state:
>  	switch (attr->xattri_dela_state) {
>  	case XFS_DAS_UNINIT:
>  		ASSERT(0);
> @@ -406,6 +407,7 @@ xfs_attr_set_iter(
>  		return xfs_attr_node_addname(attr);
>  
>  	case XFS_DAS_FOUND_LBLK:
> +	case XFS_DAS_FOUND_NBLK:
>  		/*
>  		 * Find space for remote blocks and fall into the allocation
>  		 * state.
> @@ -415,9 +417,10 @@ xfs_attr_set_iter(
>  			if (error)
>  				return error;
>  		}
> -		attr->xattri_dela_state = XFS_DAS_LEAF_ALLOC_RMT;
> +		attr->xattri_dela_state++;
>  		fallthrough;
>  	case XFS_DAS_LEAF_ALLOC_RMT:
> +	case XFS_DAS_NODE_ALLOC_RMT:
>  
>  		/*
>  		 * If there was an out-of-line value, allocate the blocks we
> @@ -466,16 +469,18 @@ xfs_attr_set_iter(
>  				return error;
>  			/*
>  			 * Commit the flag value change and start the next trans
> -			 * in series.
> +			 * in series at FLIP_FLAG.
>  			 */
> -			attr->xattri_dela_state = XFS_DAS_FLIP_LFLAG;
> +			attr->xattri_dela_state++;
>  			trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
>  						       args->dp);
>  			return -EAGAIN;
>  		}
>  
> +		attr->xattri_dela_state++;
>  		fallthrough;
>  	case XFS_DAS_FLIP_LFLAG:
> +	case XFS_DAS_FLIP_NFLAG:
>  		/*
>  		 * Dismantle the "old" attribute/value pair by removing a
>  		 * "remote" value (if it exists).
> @@ -485,10 +490,10 @@ xfs_attr_set_iter(
>  		if (error)
>  			return error;
>  
> +		attr->xattri_dela_state++;
>  		fallthrough;
>  	case XFS_DAS_RM_LBLK:
> -		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
> -		attr->xattri_dela_state = XFS_DAS_RM_LBLK;
> +	case XFS_DAS_RM_NBLK:
>  		if (args->rmtblkno) {
>  			error = xfs_attr_rmtval_remove(attr);
>  			if (error == -EAGAIN)
> @@ -503,7 +508,16 @@ xfs_attr_set_iter(
>  			return -EAGAIN;
>  		}
>  
> -		fallthrough;
> +		/*
> +		 * This is the end of the shared leaf/node sequence. We need
> +		 * to continue at the next state in the sequence, but we can't
> +		 * easily just fall through. So we increment to the next state
> +		 * and then jump back to switch statement to evaluate the next
> +		 * state correctly.
> +		 */
> +		attr->xattri_dela_state++;
> +		goto next_state;
> +
>  	case XFS_DAS_RD_LEAF:
>  		/*
>  		 * This is the last step for leaf format. Read the block with
> @@ -524,106 +538,6 @@ xfs_attr_set_iter(
>  
>  		return error;
>  
> -	case XFS_DAS_FOUND_NBLK:
> -		/*
> -		 * Find space for remote blocks and fall into the allocation
> -		 * state.
> -		 */
> -		if (args->rmtblkno > 0) {
> -			error = xfs_attr_rmtval_find_space(attr);
> -			if (error)
> -				return error;
> -		}
> -
> -		attr->xattri_dela_state = XFS_DAS_NODE_ALLOC_RMT;
> -		fallthrough;
> -	case XFS_DAS_NODE_ALLOC_RMT:
> -		/*
> -		 * If there was an out-of-line value, allocate the blocks we
> -		 * identified for its storage and copy the value.  This is done
> -		 * after we create the attribute so that we don't overflow the
> -		 * maximum size of a transaction and/or hit a deadlock.
> -		 */
> -		if (args->rmtblkno > 0) {
> -			if (attr->xattri_blkcnt > 0) {
> -				error = xfs_attr_rmtval_set_blk(attr);
> -				if (error)
> -					return error;
> -				trace_xfs_attr_set_iter_return(
> -					attr->xattri_dela_state, args->dp);
> -				return -EAGAIN;
> -			}
> -
> -			error = xfs_attr_rmtval_set_value(args);
> -			if (error)
> -				return error;
> -		}
> -
> -		/*
> -		 * If this was not a rename, clear the incomplete flag and we're
> -		 * done.
> -		 */
> -		if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> -			if (args->rmtblkno > 0)
> -				error = xfs_attr3_leaf_clearflag(args);
> -			goto out;
> -		}
> -
> -		/*
> -		 * If this is an atomic rename operation, we must "flip" the
> -		 * incomplete flags on the "new" and "old" attribute/value pairs
> -		 * so that one disappears and one appears atomically.  Then we
> -		 * must remove the "old" attribute/value pair.
> -		 *
> -		 * In a separate transaction, set the incomplete flag on the
> -		 * "old" attr and clear the incomplete flag on the "new" attr.
> -		 */
> -		if (!xfs_has_larp(mp)) {
> -			error = xfs_attr3_leaf_flipflags(args);
> -			if (error)
> -				goto out;
> -			/*
> -			 * Commit the flag value change and start the next trans
> -			 * in series
> -			 */
> -			attr->xattri_dela_state = XFS_DAS_FLIP_NFLAG;
> -			trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
> -						       args->dp);
> -			return -EAGAIN;
> -		}
> -
> -		fallthrough;
> -	case XFS_DAS_FLIP_NFLAG:
> -		/*
> -		 * Dismantle the "old" attribute/value pair by removing a
> -		 * "remote" value (if it exists).
> -		 */
> -		xfs_attr_restore_rmt_blk(args);
> -
> -		error = xfs_attr_rmtval_invalidate(args);
> -		if (error)
> -			return error;
> -
> -		fallthrough;
> -	case XFS_DAS_RM_NBLK:
> -		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
> -		attr->xattri_dela_state = XFS_DAS_RM_NBLK;
> -		if (args->rmtblkno) {
> -			error = xfs_attr_rmtval_remove(attr);
> -			if (error == -EAGAIN)
> -				trace_xfs_attr_set_iter_return(
> -					attr->xattri_dela_state, args->dp);
> -
> -			if (error)
> -				return error;
> -
> -			attr->xattri_dela_state = XFS_DAS_CLR_FLAG;
> -			trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
> -						       args->dp);
> -			return -EAGAIN;
> -		}
> -
> -		fallthrough;
>  	case XFS_DAS_CLR_FLAG:
>  		/*
>  		 * The last state for node format. Look up the old attr and
> @@ -635,7 +549,6 @@ xfs_attr_set_iter(
>  		ASSERT(0);
>  		break;
>  	}
> -out:
>  	return error;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index d016af4dbf81..37db61649217 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -450,16 +450,21 @@ enum xfs_delattr_state {
>  	XFS_DAS_RMTBLK,			/* Removing remote blks */
>  	XFS_DAS_RM_NAME,		/* Remove attr name */
>  	XFS_DAS_RM_SHRINK,		/* We are shrinking the tree */
> +
> +	/* Leaf state set sequence */

I think this comment should note that the state increment operations of
xfs_attr_set_iter requires that the exact order of the values
FOUND_[LN]BLK through RM_[LN]BLK must be preserved exactly.

Question: Are we supposed to be able to dela_state++ our way from
RM_LBLK to RD_LEAF and from RM_NBLK to CLR_FLAG?

With that comment added,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  	XFS_DAS_FOUND_LBLK,		/* We found leaf blk for attr */
>  	XFS_DAS_LEAF_ALLOC_RMT,		/* We are allocating remote blocks */
> -	XFS_DAS_FOUND_NBLK,		/* We found node blk for attr */
> -	XFS_DAS_NODE_ALLOC_RMT,		/* We are allocating remote blocks */
>  	XFS_DAS_FLIP_LFLAG,		/* Flipped leaf INCOMPLETE attr flag */
>  	XFS_DAS_RM_LBLK,		/* A rename is removing leaf blocks */
>  	XFS_DAS_RD_LEAF,		/* Read in the new leaf */
> +
> +	/* Node state set sequence, must match leaf state above */
> +	XFS_DAS_FOUND_NBLK,		/* We found node blk for attr */
> +	XFS_DAS_NODE_ALLOC_RMT,		/* We are allocating remote blocks */
>  	XFS_DAS_FLIP_NFLAG,		/* Flipped node INCOMPLETE attr flag */
>  	XFS_DAS_RM_NBLK,		/* A rename is removing node blocks */
>  	XFS_DAS_CLR_FLAG,		/* Clear incomplete flag */
> +
>  	XFS_DAS_DONE,			/* finished operation */
>  };
>  
> -- 
> 2.35.1
>
Dave Chinner May 11, 2022, 1:09 a.m. UTC | #2
On Tue, May 10, 2022 at 04:20:19PM -0700, Darrick J. Wong wrote:
> On Mon, May 09, 2022 at 10:41:27AM +1000, Dave Chinner wrote:
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index d016af4dbf81..37db61649217 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -450,16 +450,21 @@ enum xfs_delattr_state {
> >  	XFS_DAS_RMTBLK,			/* Removing remote blks */
> >  	XFS_DAS_RM_NAME,		/* Remove attr name */
> >  	XFS_DAS_RM_SHRINK,		/* We are shrinking the tree */
> > +
> > +	/* Leaf state set sequence */
> 
> I think this comment should note that the state increment operations of
> xfs_attr_set_iter requires that the exact order of the values
> FOUND_[LN]BLK through RM_[LN]BLK must be preserved exactly.

Happens later, as you've already noticed, when....

> Question: Are we supposed to be able to dela_state++ our way from
> RM_LBLK to RD_LEAF and from RM_NBLK to CLR_FLAG?

... we are finally able to dela_state++ our way right through the
leaf/node operations.

> With that comment added,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks!

-Dave.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index ab8a884af512..be580da62f08 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -394,6 +394,7 @@  xfs_attr_set_iter(
 	struct xfs_mount		*mp = args->dp->i_mount;
 
 	/* State machine switch */
+next_state:
 	switch (attr->xattri_dela_state) {
 	case XFS_DAS_UNINIT:
 		ASSERT(0);
@@ -406,6 +407,7 @@  xfs_attr_set_iter(
 		return xfs_attr_node_addname(attr);
 
 	case XFS_DAS_FOUND_LBLK:
+	case XFS_DAS_FOUND_NBLK:
 		/*
 		 * Find space for remote blocks and fall into the allocation
 		 * state.
@@ -415,9 +417,10 @@  xfs_attr_set_iter(
 			if (error)
 				return error;
 		}
-		attr->xattri_dela_state = XFS_DAS_LEAF_ALLOC_RMT;
+		attr->xattri_dela_state++;
 		fallthrough;
 	case XFS_DAS_LEAF_ALLOC_RMT:
+	case XFS_DAS_NODE_ALLOC_RMT:
 
 		/*
 		 * If there was an out-of-line value, allocate the blocks we
@@ -466,16 +469,18 @@  xfs_attr_set_iter(
 				return error;
 			/*
 			 * Commit the flag value change and start the next trans
-			 * in series.
+			 * in series at FLIP_FLAG.
 			 */
-			attr->xattri_dela_state = XFS_DAS_FLIP_LFLAG;
+			attr->xattri_dela_state++;
 			trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
 						       args->dp);
 			return -EAGAIN;
 		}
 
+		attr->xattri_dela_state++;
 		fallthrough;
 	case XFS_DAS_FLIP_LFLAG:
+	case XFS_DAS_FLIP_NFLAG:
 		/*
 		 * Dismantle the "old" attribute/value pair by removing a
 		 * "remote" value (if it exists).
@@ -485,10 +490,10 @@  xfs_attr_set_iter(
 		if (error)
 			return error;
 
+		attr->xattri_dela_state++;
 		fallthrough;
 	case XFS_DAS_RM_LBLK:
-		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
-		attr->xattri_dela_state = XFS_DAS_RM_LBLK;
+	case XFS_DAS_RM_NBLK:
 		if (args->rmtblkno) {
 			error = xfs_attr_rmtval_remove(attr);
 			if (error == -EAGAIN)
@@ -503,7 +508,16 @@  xfs_attr_set_iter(
 			return -EAGAIN;
 		}
 
-		fallthrough;
+		/*
+		 * This is the end of the shared leaf/node sequence. We need
+		 * to continue at the next state in the sequence, but we can't
+		 * easily just fall through. So we increment to the next state
+		 * and then jump back to switch statement to evaluate the next
+		 * state correctly.
+		 */
+		attr->xattri_dela_state++;
+		goto next_state;
+
 	case XFS_DAS_RD_LEAF:
 		/*
 		 * This is the last step for leaf format. Read the block with
@@ -524,106 +538,6 @@  xfs_attr_set_iter(
 
 		return error;
 
-	case XFS_DAS_FOUND_NBLK:
-		/*
-		 * Find space for remote blocks and fall into the allocation
-		 * state.
-		 */
-		if (args->rmtblkno > 0) {
-			error = xfs_attr_rmtval_find_space(attr);
-			if (error)
-				return error;
-		}
-
-		attr->xattri_dela_state = XFS_DAS_NODE_ALLOC_RMT;
-		fallthrough;
-	case XFS_DAS_NODE_ALLOC_RMT:
-		/*
-		 * If there was an out-of-line value, allocate the blocks we
-		 * identified for its storage and copy the value.  This is done
-		 * after we create the attribute so that we don't overflow the
-		 * maximum size of a transaction and/or hit a deadlock.
-		 */
-		if (args->rmtblkno > 0) {
-			if (attr->xattri_blkcnt > 0) {
-				error = xfs_attr_rmtval_set_blk(attr);
-				if (error)
-					return error;
-				trace_xfs_attr_set_iter_return(
-					attr->xattri_dela_state, args->dp);
-				return -EAGAIN;
-			}
-
-			error = xfs_attr_rmtval_set_value(args);
-			if (error)
-				return error;
-		}
-
-		/*
-		 * If this was not a rename, clear the incomplete flag and we're
-		 * done.
-		 */
-		if (!(args->op_flags & XFS_DA_OP_RENAME)) {
-			if (args->rmtblkno > 0)
-				error = xfs_attr3_leaf_clearflag(args);
-			goto out;
-		}
-
-		/*
-		 * If this is an atomic rename operation, we must "flip" the
-		 * incomplete flags on the "new" and "old" attribute/value pairs
-		 * so that one disappears and one appears atomically.  Then we
-		 * must remove the "old" attribute/value pair.
-		 *
-		 * In a separate transaction, set the incomplete flag on the
-		 * "old" attr and clear the incomplete flag on the "new" attr.
-		 */
-		if (!xfs_has_larp(mp)) {
-			error = xfs_attr3_leaf_flipflags(args);
-			if (error)
-				goto out;
-			/*
-			 * Commit the flag value change and start the next trans
-			 * in series
-			 */
-			attr->xattri_dela_state = XFS_DAS_FLIP_NFLAG;
-			trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
-						       args->dp);
-			return -EAGAIN;
-		}
-
-		fallthrough;
-	case XFS_DAS_FLIP_NFLAG:
-		/*
-		 * Dismantle the "old" attribute/value pair by removing a
-		 * "remote" value (if it exists).
-		 */
-		xfs_attr_restore_rmt_blk(args);
-
-		error = xfs_attr_rmtval_invalidate(args);
-		if (error)
-			return error;
-
-		fallthrough;
-	case XFS_DAS_RM_NBLK:
-		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
-		attr->xattri_dela_state = XFS_DAS_RM_NBLK;
-		if (args->rmtblkno) {
-			error = xfs_attr_rmtval_remove(attr);
-			if (error == -EAGAIN)
-				trace_xfs_attr_set_iter_return(
-					attr->xattri_dela_state, args->dp);
-
-			if (error)
-				return error;
-
-			attr->xattri_dela_state = XFS_DAS_CLR_FLAG;
-			trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
-						       args->dp);
-			return -EAGAIN;
-		}
-
-		fallthrough;
 	case XFS_DAS_CLR_FLAG:
 		/*
 		 * The last state for node format. Look up the old attr and
@@ -635,7 +549,6 @@  xfs_attr_set_iter(
 		ASSERT(0);
 		break;
 	}
-out:
 	return error;
 }
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index d016af4dbf81..37db61649217 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -450,16 +450,21 @@  enum xfs_delattr_state {
 	XFS_DAS_RMTBLK,			/* Removing remote blks */
 	XFS_DAS_RM_NAME,		/* Remove attr name */
 	XFS_DAS_RM_SHRINK,		/* We are shrinking the tree */
+
+	/* Leaf state set sequence */
 	XFS_DAS_FOUND_LBLK,		/* We found leaf blk for attr */
 	XFS_DAS_LEAF_ALLOC_RMT,		/* We are allocating remote blocks */
-	XFS_DAS_FOUND_NBLK,		/* We found node blk for attr */
-	XFS_DAS_NODE_ALLOC_RMT,		/* We are allocating remote blocks */
 	XFS_DAS_FLIP_LFLAG,		/* Flipped leaf INCOMPLETE attr flag */
 	XFS_DAS_RM_LBLK,		/* A rename is removing leaf blocks */
 	XFS_DAS_RD_LEAF,		/* Read in the new leaf */
+
+	/* Node state set sequence, must match leaf state above */
+	XFS_DAS_FOUND_NBLK,		/* We found node blk for attr */
+	XFS_DAS_NODE_ALLOC_RMT,		/* We are allocating remote blocks */
 	XFS_DAS_FLIP_NFLAG,		/* Flipped node INCOMPLETE attr flag */
 	XFS_DAS_RM_NBLK,		/* A rename is removing node blocks */
 	XFS_DAS_CLR_FLAG,		/* Clear incomplete flag */
+
 	XFS_DAS_DONE,			/* finished operation */
 };