| Message ID | 20220509004138.762556-13-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | XFS: LARP state machine and recovery rework | expand |
On Mon, May 09, 2022 at 10:41:32AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Now that the full xfs_attr_set_iter() state machine always > terminates with either the state being XFS_DAS_DONE on success or > an error on failure, we can get rid of the need for it to return > -EAGAIN whenever it needs to roll the transaction before running > the next state. > > That is, we don't need to spray -EAGAIN return states everywhere, > the caller just check the state machine state for completion to > determine what action should be taken next. This greatly simplifies > the code within the state machine implementation as it now only has > to handle 0 for success or -errno for error and it doesn't need to > tell the caller to retry. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Allison Henderson<allison.henderson@oracle.com> LGTM Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_attr.c | 86 +++++++++++++++++----------------------- > fs/xfs/xfs_attr_item.c | 2 + > 2 files changed, 38 insertions(+), 50 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 5707ac4f44a7..89e68d9e22c0 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -290,7 +290,6 @@ xfs_attr_sf_addname( > */ > xfs_trans_bhold(args->trans, attr->xattri_leaf_bp); > attr->xattri_dela_state = XFS_DAS_LEAF_ADD; > - error = -EAGAIN; > out: > trace_xfs_attr_sf_addname_return(attr->xattri_dela_state, args->dp); > return error; > @@ -343,7 +342,6 @@ xfs_attr_leaf_addname( > * retry the add to the newly allocated node block. > */ > attr->xattri_dela_state = XFS_DAS_NODE_ADD; > - error = -EAGAIN; > goto out; > } > if (error) > @@ -354,20 +352,24 @@ xfs_attr_leaf_addname( > * or perform more xattr manipulations. Otherwise there is nothing more > * to do and we can return success. > */ > - if (args->rmtblkno) { > + if (args->rmtblkno) > attr->xattri_dela_state = XFS_DAS_LEAF_SET_RMT; > - error = -EAGAIN; > - } else if (args->op_flags & XFS_DA_OP_RENAME) { > + else if (args->op_flags & XFS_DA_OP_RENAME) > xfs_attr_dela_state_set_replace(attr, XFS_DAS_LEAF_REPLACE); > - error = -EAGAIN; > - } else { > + else > attr->xattri_dela_state = XFS_DAS_DONE; > - } > out: > trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); > return error; > } > > +/* > + * Add an entry to a node format attr tree. > + * > + * Note that we might still have a leaf here - xfs_attr_is_leaf() cannot tell > + * the difference between leaf + remote attr blocks and a node format tree, > + * so we may still end up having to convert from leaf to node format here. > + */ > static int > xfs_attr_node_addname( > struct xfs_attr_item *attr) > @@ -382,19 +384,26 @@ xfs_attr_node_addname( > return error; > > error = xfs_attr_node_try_addname(attr); > + if (error == -ENOSPC) { > + error = xfs_attr3_leaf_to_node(args); > + if (error) > + return error; > + /* > + * No state change, we really are in node form now > + * but we need the transaction rolled to continue. > + */ > + goto out; > + } > if (error) > return error; > > - if (args->rmtblkno) { > + if (args->rmtblkno) > attr->xattri_dela_state = XFS_DAS_NODE_SET_RMT; > - error = -EAGAIN; > - } else if (args->op_flags & XFS_DA_OP_RENAME) { > + else if (args->op_flags & XFS_DA_OP_RENAME) > xfs_attr_dela_state_set_replace(attr, XFS_DAS_NODE_REPLACE); > - error = -EAGAIN; > - } else { > + else > attr->xattri_dela_state = XFS_DAS_DONE; > - } > - > +out: > trace_xfs_attr_node_addname_return(attr->xattri_dela_state, args->dp); > return error; > } > @@ -417,10 +426,8 @@ xfs_attr_rmtval_alloc( > if (error) > return error; > /* Roll the transaction only if there is more to allocate. */ > - if (attr->xattri_blkcnt > 0) { > - error = -EAGAIN; > + if (attr->xattri_blkcnt > 0) > goto out; > - } > } > > error = xfs_attr_rmtval_set_value(args); > @@ -516,11 +523,12 @@ xfs_attr_leaf_shrink( > } > > /* > - * Set the attribute specified in @args. > - * This routine is meant to function as a delayed operation, and may return > - * -EAGAIN when the transaction needs to be rolled. Calling functions will need > - * to handle this, and recall the function until a successful error code is > - * returned. > + * Run the attribute operation specified in @attr. > + * > + * This routine is meant to function as a delayed operation and will set the > + * state to XFS_DAS_DONE when the operation is complete. Calling functions will > + * need to handle this, and recall the function until either an error or > + * XFS_DAS_DONE is detected. > */ > int > xfs_attr_set_iter( > @@ -573,7 +581,6 @@ xfs_attr_set_iter( > * We must commit the flag value change now to make it atomic > * and then we can start the next trans in series at REMOVE_OLD. > */ > - error = -EAGAIN; > attr->xattri_dela_state++; > break; > > @@ -601,8 +608,10 @@ xfs_attr_set_iter( > case XFS_DAS_LEAF_REMOVE_RMT: > case XFS_DAS_NODE_REMOVE_RMT: > error = xfs_attr_rmtval_remove(attr); > - if (error == -EAGAIN) > + if (error == -EAGAIN) { > + error = 0; > break; > + } > if (error) > return error; > > @@ -614,7 +623,6 @@ xfs_attr_set_iter( > * can't do that in the same transaction where we removed the > * remote attr blocks. > */ > - error = -EAGAIN; > attr->xattri_dela_state++; > break; > > @@ -1250,14 +1258,6 @@ xfs_attr_node_addname_find_attr( > * This will involve walking down the Btree, and may involve splitting > * leaf nodes and even splitting intermediate nodes up to and including > * the root node (a special case of an intermediate node). > - * > - * "Remote" attribute values confuse the issue and atomic rename operations > - * add a whole extra layer of confusion on top of that. > - * > - * This routine is meant to function as a delayed operation, and may return > - * -EAGAIN when the transaction needs to be rolled. Calling functions will need > - * to handle this, and recall the function until a successful error code is > - *returned. > */ > static int > xfs_attr_node_try_addname( > @@ -1279,24 +1279,10 @@ xfs_attr_node_try_addname( > /* > * Its really a single leaf node, but it had > * out-of-line values so it looked like it *might* > - * have been a b-tree. > + * have been a b-tree. Let the caller deal with this. > */ > xfs_da_state_free(state); > - state = NULL; > - error = xfs_attr3_leaf_to_node(args); > - if (error) > - goto out; > - > - /* > - * Now that we have converted the leaf to a node, we can > - * roll the transaction, and try xfs_attr3_leaf_add > - * again on re-entry. No need to set dela_state to do > - * this. dela_state is still unset by this function at > - * this point. > - */ > - trace_xfs_attr_node_addname_return( > - attr->xattri_dela_state, args->dp); > - return -EAGAIN; > + return -ENOSPC; > } > > /* > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > index 5bfb33746e37..740a55d07660 100644 > --- a/fs/xfs/xfs_attr_item.c > +++ b/fs/xfs/xfs_attr_item.c > @@ -313,6 +313,8 @@ xfs_xattri_finish_update( > case XFS_ATTR_OP_FLAGS_SET: > case XFS_ATTR_OP_FLAGS_REPLACE: > error = xfs_attr_set_iter(attr); > + if (!error && attr->xattri_dela_state != XFS_DAS_DONE) > + error = -EAGAIN; > break; > case XFS_ATTR_OP_FLAGS_REMOVE: > ASSERT(XFS_IFORK_Q(args->dp)); > -- > 2.35.1 >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 5707ac4f44a7..89e68d9e22c0 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -290,7 +290,6 @@ xfs_attr_sf_addname( */ xfs_trans_bhold(args->trans, attr->xattri_leaf_bp); attr->xattri_dela_state = XFS_DAS_LEAF_ADD; - error = -EAGAIN; out: trace_xfs_attr_sf_addname_return(attr->xattri_dela_state, args->dp); return error; @@ -343,7 +342,6 @@ xfs_attr_leaf_addname( * retry the add to the newly allocated node block. */ attr->xattri_dela_state = XFS_DAS_NODE_ADD; - error = -EAGAIN; goto out; } if (error) @@ -354,20 +352,24 @@ xfs_attr_leaf_addname( * or perform more xattr manipulations. Otherwise there is nothing more * to do and we can return success. */ - if (args->rmtblkno) { + if (args->rmtblkno) attr->xattri_dela_state = XFS_DAS_LEAF_SET_RMT; - error = -EAGAIN; - } else if (args->op_flags & XFS_DA_OP_RENAME) { + else if (args->op_flags & XFS_DA_OP_RENAME) xfs_attr_dela_state_set_replace(attr, XFS_DAS_LEAF_REPLACE); - error = -EAGAIN; - } else { + else attr->xattri_dela_state = XFS_DAS_DONE; - } out: trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); return error; } +/* + * Add an entry to a node format attr tree. + * + * Note that we might still have a leaf here - xfs_attr_is_leaf() cannot tell + * the difference between leaf + remote attr blocks and a node format tree, + * so we may still end up having to convert from leaf to node format here. + */ static int xfs_attr_node_addname( struct xfs_attr_item *attr) @@ -382,19 +384,26 @@ xfs_attr_node_addname( return error; error = xfs_attr_node_try_addname(attr); + if (error == -ENOSPC) { + error = xfs_attr3_leaf_to_node(args); + if (error) + return error; + /* + * No state change, we really are in node form now + * but we need the transaction rolled to continue. + */ + goto out; + } if (error) return error; - if (args->rmtblkno) { + if (args->rmtblkno) attr->xattri_dela_state = XFS_DAS_NODE_SET_RMT; - error = -EAGAIN; - } else if (args->op_flags & XFS_DA_OP_RENAME) { + else if (args->op_flags & XFS_DA_OP_RENAME) xfs_attr_dela_state_set_replace(attr, XFS_DAS_NODE_REPLACE); - error = -EAGAIN; - } else { + else attr->xattri_dela_state = XFS_DAS_DONE; - } - +out: trace_xfs_attr_node_addname_return(attr->xattri_dela_state, args->dp); return error; } @@ -417,10 +426,8 @@ xfs_attr_rmtval_alloc( if (error) return error; /* Roll the transaction only if there is more to allocate. */ - if (attr->xattri_blkcnt > 0) { - error = -EAGAIN; + if (attr->xattri_blkcnt > 0) goto out; - } } error = xfs_attr_rmtval_set_value(args); @@ -516,11 +523,12 @@ xfs_attr_leaf_shrink( } /* - * Set the attribute specified in @args. - * This routine is meant to function as a delayed operation, and may return - * -EAGAIN when the transaction needs to be rolled. Calling functions will need - * to handle this, and recall the function until a successful error code is - * returned. + * Run the attribute operation specified in @attr. + * + * This routine is meant to function as a delayed operation and will set the + * state to XFS_DAS_DONE when the operation is complete. Calling functions will + * need to handle this, and recall the function until either an error or + * XFS_DAS_DONE is detected. */ int xfs_attr_set_iter( @@ -573,7 +581,6 @@ xfs_attr_set_iter( * We must commit the flag value change now to make it atomic * and then we can start the next trans in series at REMOVE_OLD. */ - error = -EAGAIN; attr->xattri_dela_state++; break; @@ -601,8 +608,10 @@ xfs_attr_set_iter( case XFS_DAS_LEAF_REMOVE_RMT: case XFS_DAS_NODE_REMOVE_RMT: error = xfs_attr_rmtval_remove(attr); - if (error == -EAGAIN) + if (error == -EAGAIN) { + error = 0; break; + } if (error) return error; @@ -614,7 +623,6 @@ xfs_attr_set_iter( * can't do that in the same transaction where we removed the * remote attr blocks. */ - error = -EAGAIN; attr->xattri_dela_state++; break; @@ -1250,14 +1258,6 @@ xfs_attr_node_addname_find_attr( * This will involve walking down the Btree, and may involve splitting * leaf nodes and even splitting intermediate nodes up to and including * the root node (a special case of an intermediate node). - * - * "Remote" attribute values confuse the issue and atomic rename operations - * add a whole extra layer of confusion on top of that. - * - * This routine is meant to function as a delayed operation, and may return - * -EAGAIN when the transaction needs to be rolled. Calling functions will need - * to handle this, and recall the function until a successful error code is - *returned. */ static int xfs_attr_node_try_addname( @@ -1279,24 +1279,10 @@ xfs_attr_node_try_addname( /* * Its really a single leaf node, but it had * out-of-line values so it looked like it *might* - * have been a b-tree. + * have been a b-tree. Let the caller deal with this. */ xfs_da_state_free(state); - state = NULL; - error = xfs_attr3_leaf_to_node(args); - if (error) - goto out; - - /* - * Now that we have converted the leaf to a node, we can - * roll the transaction, and try xfs_attr3_leaf_add - * again on re-entry. No need to set dela_state to do - * this. dela_state is still unset by this function at - * this point. - */ - trace_xfs_attr_node_addname_return( - attr->xattri_dela_state, args->dp); - return -EAGAIN; + return -ENOSPC; } /* diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c index 5bfb33746e37..740a55d07660 100644 --- a/fs/xfs/xfs_attr_item.c +++ b/fs/xfs/xfs_attr_item.c @@ -313,6 +313,8 @@ xfs_xattri_finish_update( case XFS_ATTR_OP_FLAGS_SET: case XFS_ATTR_OP_FLAGS_REPLACE: error = xfs_attr_set_iter(attr); + if (!error && attr->xattri_dela_state != XFS_DAS_DONE) + error = -EAGAIN; break; case XFS_ATTR_OP_FLAGS_REMOVE: ASSERT(XFS_IFORK_Q(args->dp));