| Message ID | 20220509004138.762556-14-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:33AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We need to merge the add and remove code paths to enable safe > recovery of replace operations. Hoist the initial remove states from > xfs_attr_remove_iter into xfs_attr_set_iter. We will make use of > them in the next patches. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Allison Henderson<allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 139 ++++++++++++++++++++++----------------- > fs/xfs/libxfs/xfs_attr.h | 4 ++ > fs/xfs/xfs_trace.h | 3 + > 3 files changed, 84 insertions(+), 62 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 89e68d9e22c0..a6a9b1f8dce6 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -451,6 +451,68 @@ xfs_attr_rmtval_alloc( > return error; > } > > +/* > + * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers > + * for later deletion of the entry. > + */ > +static int > +xfs_attr_leaf_mark_incomplete( > + struct xfs_da_args *args, > + struct xfs_da_state *state) > +{ > + int error; > + > + /* > + * Fill in disk block numbers in the state structure > + * so that we can get the buffers back after we commit > + * several transactions in the following calls. > + */ > + error = xfs_attr_fillstate(state); > + if (error) > + return error; > + > + /* > + * Mark the attribute as INCOMPLETE > + */ > + return xfs_attr3_leaf_setflag(args); > +} > + > +/* > + * Initial setup for xfs_attr_node_removename. Make sure the attr is there and > + * the blocks are valid. Attr keys with remote blocks will be marked > + * incomplete. > + */ > +static > +int xfs_attr_node_removename_setup( > + struct xfs_attr_item *attr) > +{ > + struct xfs_da_args *args = attr->xattri_da_args; > + struct xfs_da_state **state = &attr->xattri_da_state; > + int error; > + > + error = xfs_attr_node_hasname(args, state); > + if (error != -EEXIST) > + goto out; > + error = 0; > + > + ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > + ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > + XFS_ATTR_LEAF_MAGIC); > + > + if (args->rmtblkno > 0) { > + error = xfs_attr_leaf_mark_incomplete(args, *state); > + if (error) > + goto out; > + > + error = xfs_attr_rmtval_invalidate(args); > + } > +out: > + if (error) > + xfs_da_state_free(*state); > + > + return error; > +} > + > /* > * Remove the original attr we have just replaced. This is dependent on the > * original lookup and insert placing the old attr in args->blkno/args->index > @@ -550,6 +612,21 @@ xfs_attr_set_iter( > case XFS_DAS_NODE_ADD: > return xfs_attr_node_addname(attr); > > + case XFS_DAS_SF_REMOVE: I think attr removal is broken until the next patch, which adds xfs_attr_init_remove_state to set dela_state to DAS_*_REMOVE, right? The rest of the state machine changes look solid. With that moved up by one patch to fix the bisection issue, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + attr->xattri_dela_state = XFS_DAS_DONE; > + return xfs_attr_sf_removename(args); > + case XFS_DAS_LEAF_REMOVE: > + attr->xattri_dela_state = XFS_DAS_DONE; > + return xfs_attr_leaf_removename(args); > + case XFS_DAS_NODE_REMOVE: > + error = xfs_attr_node_removename_setup(attr); > + if (error) > + return error; > + attr->xattri_dela_state = XFS_DAS_NODE_REMOVE_RMT; > + if (args->rmtblkno == 0) > + attr->xattri_dela_state++; > + break; > + > case XFS_DAS_LEAF_SET_RMT: > case XFS_DAS_NODE_SET_RMT: > error = xfs_attr_rmtval_find_space(attr); > @@ -1351,68 +1428,6 @@ xfs_attr_node_remove_attr( > } > > > -/* > - * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers > - * for later deletion of the entry. > - */ > -STATIC int > -xfs_attr_leaf_mark_incomplete( > - struct xfs_da_args *args, > - struct xfs_da_state *state) > -{ > - int error; > - > - /* > - * Fill in disk block numbers in the state structure > - * so that we can get the buffers back after we commit > - * several transactions in the following calls. > - */ > - error = xfs_attr_fillstate(state); > - if (error) > - return error; > - > - /* > - * Mark the attribute as INCOMPLETE > - */ > - return xfs_attr3_leaf_setflag(args); > -} > - > -/* > - * Initial setup for xfs_attr_node_removename. Make sure the attr is there and > - * the blocks are valid. Attr keys with remote blocks will be marked > - * incomplete. > - */ > -STATIC > -int xfs_attr_node_removename_setup( > - struct xfs_attr_item *attr) > -{ > - struct xfs_da_args *args = attr->xattri_da_args; > - struct xfs_da_state **state = &attr->xattri_da_state; > - int error; > - > - error = xfs_attr_node_hasname(args, state); > - if (error != -EEXIST) > - goto out; > - error = 0; > - > - ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > - ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > - XFS_ATTR_LEAF_MAGIC); > - > - if (args->rmtblkno > 0) { > - error = xfs_attr_leaf_mark_incomplete(args, *state); > - if (error) > - goto out; > - > - error = xfs_attr_rmtval_invalidate(args); > - } > -out: > - if (error) > - xfs_da_state_free(*state); > - > - return error; > -} > - > STATIC int > xfs_attr_node_removename( > struct xfs_da_args *args, > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index c318260f17d4..7ea7c7fa31ac 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -451,6 +451,10 @@ enum xfs_delattr_state { > XFS_DAS_RM_NAME, /* Remove attr name */ > XFS_DAS_RM_SHRINK, /* We are shrinking the tree */ > > + XFS_DAS_SF_REMOVE, /* Initial shortform set iter state */ > + XFS_DAS_LEAF_REMOVE, /* Initial leaf form set iter state */ > + XFS_DAS_NODE_REMOVE, /* Initial node form set iter state */ > + > /* Leaf state set/replace sequence */ > XFS_DAS_LEAF_SET_RMT, /* set a remote xattr from a leaf */ > XFS_DAS_LEAF_ALLOC_RMT, /* We are allocating remote blocks */ > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h > index 260760ce2d05..01b047d86cd1 100644 > --- a/fs/xfs/xfs_trace.h > +++ b/fs/xfs/xfs_trace.h > @@ -4136,6 +4136,9 @@ TRACE_DEFINE_ENUM(XFS_DAS_NODE_ADD); > TRACE_DEFINE_ENUM(XFS_DAS_RMTBLK); > TRACE_DEFINE_ENUM(XFS_DAS_RM_NAME); > TRACE_DEFINE_ENUM(XFS_DAS_RM_SHRINK); > +TRACE_DEFINE_ENUM(XFS_DAS_SF_REMOVE); > +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REMOVE); > +TRACE_DEFINE_ENUM(XFS_DAS_NODE_REMOVE); > TRACE_DEFINE_ENUM(XFS_DAS_LEAF_SET_RMT); > TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ALLOC_RMT); > TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REPLACE); > -- > 2.35.1 >
On Tue, May 10, 2022 at 04:37:36PM -0700, Darrick J. Wong wrote: > On Mon, May 09, 2022 at 10:41:33AM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > We need to merge the add and remove code paths to enable safe > > recovery of replace operations. Hoist the initial remove states from > > xfs_attr_remove_iter into xfs_attr_set_iter. We will make use of > > them in the next patches. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Reviewed-by: Allison Henderson<allison.henderson@oracle.com> > > --- > > fs/xfs/libxfs/xfs_attr.c | 139 ++++++++++++++++++++++----------------- > > fs/xfs/libxfs/xfs_attr.h | 4 ++ > > fs/xfs/xfs_trace.h | 3 + > > 3 files changed, 84 insertions(+), 62 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > index 89e68d9e22c0..a6a9b1f8dce6 100644 > > --- a/fs/xfs/libxfs/xfs_attr.c > > +++ b/fs/xfs/libxfs/xfs_attr.c > > @@ -451,6 +451,68 @@ xfs_attr_rmtval_alloc( > > return error; > > } > > > > +/* > > + * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers > > + * for later deletion of the entry. > > + */ > > +static int > > +xfs_attr_leaf_mark_incomplete( > > + struct xfs_da_args *args, > > + struct xfs_da_state *state) > > +{ > > + int error; > > + > > + /* > > + * Fill in disk block numbers in the state structure > > + * so that we can get the buffers back after we commit > > + * several transactions in the following calls. > > + */ > > + error = xfs_attr_fillstate(state); > > + if (error) > > + return error; > > + > > + /* > > + * Mark the attribute as INCOMPLETE > > + */ > > + return xfs_attr3_leaf_setflag(args); > > +} > > + > > +/* > > + * Initial setup for xfs_attr_node_removename. Make sure the attr is there and > > + * the blocks are valid. Attr keys with remote blocks will be marked > > + * incomplete. > > + */ > > +static > > +int xfs_attr_node_removename_setup( > > + struct xfs_attr_item *attr) > > +{ > > + struct xfs_da_args *args = attr->xattri_da_args; > > + struct xfs_da_state **state = &attr->xattri_da_state; > > + int error; > > + > > + error = xfs_attr_node_hasname(args, state); > > + if (error != -EEXIST) > > + goto out; > > + error = 0; > > + > > + ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > > + ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > > + XFS_ATTR_LEAF_MAGIC); > > + > > + if (args->rmtblkno > 0) { > > + error = xfs_attr_leaf_mark_incomplete(args, *state); > > + if (error) > > + goto out; > > + > > + error = xfs_attr_rmtval_invalidate(args); > > + } > > +out: > > + if (error) > > + xfs_da_state_free(*state); > > + > > + return error; > > +} > > + > > /* > > * Remove the original attr we have just replaced. This is dependent on the > > * original lookup and insert placing the old attr in args->blkno/args->index > > @@ -550,6 +612,21 @@ xfs_attr_set_iter( > > case XFS_DAS_NODE_ADD: > > return xfs_attr_node_addname(attr); > > > > + case XFS_DAS_SF_REMOVE: > > I think attr removal is broken until the next patch, which adds > xfs_attr_init_remove_state to set dela_state to DAS_*_REMOVE, right? Aha, I was looking at the wrong tab, the old xfs_attr_remove_iter code is still in place, so ignore this question... > The rest of the state machine changes look solid. > > With that moved up by one patch to fix the bisection issue, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> ..and go straight to the RVB tag. --D > > --D > > > + attr->xattri_dela_state = XFS_DAS_DONE; > > + return xfs_attr_sf_removename(args); > > + case XFS_DAS_LEAF_REMOVE: > > + attr->xattri_dela_state = XFS_DAS_DONE; > > + return xfs_attr_leaf_removename(args); > > + case XFS_DAS_NODE_REMOVE: > > + error = xfs_attr_node_removename_setup(attr); > > + if (error) > > + return error; > > + attr->xattri_dela_state = XFS_DAS_NODE_REMOVE_RMT; > > + if (args->rmtblkno == 0) > > + attr->xattri_dela_state++; > > + break; > > + > > case XFS_DAS_LEAF_SET_RMT: > > case XFS_DAS_NODE_SET_RMT: > > error = xfs_attr_rmtval_find_space(attr); > > @@ -1351,68 +1428,6 @@ xfs_attr_node_remove_attr( > > } > > > > > > -/* > > - * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers > > - * for later deletion of the entry. > > - */ > > -STATIC int > > -xfs_attr_leaf_mark_incomplete( > > - struct xfs_da_args *args, > > - struct xfs_da_state *state) > > -{ > > - int error; > > - > > - /* > > - * Fill in disk block numbers in the state structure > > - * so that we can get the buffers back after we commit > > - * several transactions in the following calls. > > - */ > > - error = xfs_attr_fillstate(state); > > - if (error) > > - return error; > > - > > - /* > > - * Mark the attribute as INCOMPLETE > > - */ > > - return xfs_attr3_leaf_setflag(args); > > -} > > - > > -/* > > - * Initial setup for xfs_attr_node_removename. Make sure the attr is there and > > - * the blocks are valid. Attr keys with remote blocks will be marked > > - * incomplete. > > - */ > > -STATIC > > -int xfs_attr_node_removename_setup( > > - struct xfs_attr_item *attr) > > -{ > > - struct xfs_da_args *args = attr->xattri_da_args; > > - struct xfs_da_state **state = &attr->xattri_da_state; > > - int error; > > - > > - error = xfs_attr_node_hasname(args, state); > > - if (error != -EEXIST) > > - goto out; > > - error = 0; > > - > > - ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > > - ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > > - XFS_ATTR_LEAF_MAGIC); > > - > > - if (args->rmtblkno > 0) { > > - error = xfs_attr_leaf_mark_incomplete(args, *state); > > - if (error) > > - goto out; > > - > > - error = xfs_attr_rmtval_invalidate(args); > > - } > > -out: > > - if (error) > > - xfs_da_state_free(*state); > > - > > - return error; > > -} > > - > > STATIC int > > xfs_attr_node_removename( > > struct xfs_da_args *args, > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > index c318260f17d4..7ea7c7fa31ac 100644 > > --- a/fs/xfs/libxfs/xfs_attr.h > > +++ b/fs/xfs/libxfs/xfs_attr.h > > @@ -451,6 +451,10 @@ enum xfs_delattr_state { > > XFS_DAS_RM_NAME, /* Remove attr name */ > > XFS_DAS_RM_SHRINK, /* We are shrinking the tree */ > > > > + XFS_DAS_SF_REMOVE, /* Initial shortform set iter state */ > > + XFS_DAS_LEAF_REMOVE, /* Initial leaf form set iter state */ > > + XFS_DAS_NODE_REMOVE, /* Initial node form set iter state */ > > + > > /* Leaf state set/replace sequence */ > > XFS_DAS_LEAF_SET_RMT, /* set a remote xattr from a leaf */ > > XFS_DAS_LEAF_ALLOC_RMT, /* We are allocating remote blocks */ > > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h > > index 260760ce2d05..01b047d86cd1 100644 > > --- a/fs/xfs/xfs_trace.h > > +++ b/fs/xfs/xfs_trace.h > > @@ -4136,6 +4136,9 @@ TRACE_DEFINE_ENUM(XFS_DAS_NODE_ADD); > > TRACE_DEFINE_ENUM(XFS_DAS_RMTBLK); > > TRACE_DEFINE_ENUM(XFS_DAS_RM_NAME); > > TRACE_DEFINE_ENUM(XFS_DAS_RM_SHRINK); > > +TRACE_DEFINE_ENUM(XFS_DAS_SF_REMOVE); > > +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REMOVE); > > +TRACE_DEFINE_ENUM(XFS_DAS_NODE_REMOVE); > > TRACE_DEFINE_ENUM(XFS_DAS_LEAF_SET_RMT); > > TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ALLOC_RMT); > > TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REPLACE); > > -- > > 2.35.1 > >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 89e68d9e22c0..a6a9b1f8dce6 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -451,6 +451,68 @@ xfs_attr_rmtval_alloc( return error; } +/* + * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers + * for later deletion of the entry. + */ +static int +xfs_attr_leaf_mark_incomplete( + struct xfs_da_args *args, + struct xfs_da_state *state) +{ + int error; + + /* + * Fill in disk block numbers in the state structure + * so that we can get the buffers back after we commit + * several transactions in the following calls. + */ + error = xfs_attr_fillstate(state); + if (error) + return error; + + /* + * Mark the attribute as INCOMPLETE + */ + return xfs_attr3_leaf_setflag(args); +} + +/* + * Initial setup for xfs_attr_node_removename. Make sure the attr is there and + * the blocks are valid. Attr keys with remote blocks will be marked + * incomplete. + */ +static +int xfs_attr_node_removename_setup( + struct xfs_attr_item *attr) +{ + struct xfs_da_args *args = attr->xattri_da_args; + struct xfs_da_state **state = &attr->xattri_da_state; + int error; + + error = xfs_attr_node_hasname(args, state); + if (error != -EEXIST) + goto out; + error = 0; + + ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); + ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == + XFS_ATTR_LEAF_MAGIC); + + if (args->rmtblkno > 0) { + error = xfs_attr_leaf_mark_incomplete(args, *state); + if (error) + goto out; + + error = xfs_attr_rmtval_invalidate(args); + } +out: + if (error) + xfs_da_state_free(*state); + + return error; +} + /* * Remove the original attr we have just replaced. This is dependent on the * original lookup and insert placing the old attr in args->blkno/args->index @@ -550,6 +612,21 @@ xfs_attr_set_iter( case XFS_DAS_NODE_ADD: return xfs_attr_node_addname(attr); + case XFS_DAS_SF_REMOVE: + attr->xattri_dela_state = XFS_DAS_DONE; + return xfs_attr_sf_removename(args); + case XFS_DAS_LEAF_REMOVE: + attr->xattri_dela_state = XFS_DAS_DONE; + return xfs_attr_leaf_removename(args); + case XFS_DAS_NODE_REMOVE: + error = xfs_attr_node_removename_setup(attr); + if (error) + return error; + attr->xattri_dela_state = XFS_DAS_NODE_REMOVE_RMT; + if (args->rmtblkno == 0) + attr->xattri_dela_state++; + break; + case XFS_DAS_LEAF_SET_RMT: case XFS_DAS_NODE_SET_RMT: error = xfs_attr_rmtval_find_space(attr); @@ -1351,68 +1428,6 @@ xfs_attr_node_remove_attr( } -/* - * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers - * for later deletion of the entry. - */ -STATIC int -xfs_attr_leaf_mark_incomplete( - struct xfs_da_args *args, - struct xfs_da_state *state) -{ - int error; - - /* - * Fill in disk block numbers in the state structure - * so that we can get the buffers back after we commit - * several transactions in the following calls. - */ - error = xfs_attr_fillstate(state); - if (error) - return error; - - /* - * Mark the attribute as INCOMPLETE - */ - return xfs_attr3_leaf_setflag(args); -} - -/* - * Initial setup for xfs_attr_node_removename. Make sure the attr is there and - * the blocks are valid. Attr keys with remote blocks will be marked - * incomplete. - */ -STATIC -int xfs_attr_node_removename_setup( - struct xfs_attr_item *attr) -{ - struct xfs_da_args *args = attr->xattri_da_args; - struct xfs_da_state **state = &attr->xattri_da_state; - int error; - - error = xfs_attr_node_hasname(args, state); - if (error != -EEXIST) - goto out; - error = 0; - - ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); - ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == - XFS_ATTR_LEAF_MAGIC); - - if (args->rmtblkno > 0) { - error = xfs_attr_leaf_mark_incomplete(args, *state); - if (error) - goto out; - - error = xfs_attr_rmtval_invalidate(args); - } -out: - if (error) - xfs_da_state_free(*state); - - return error; -} - STATIC int xfs_attr_node_removename( struct xfs_da_args *args, diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index c318260f17d4..7ea7c7fa31ac 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -451,6 +451,10 @@ enum xfs_delattr_state { XFS_DAS_RM_NAME, /* Remove attr name */ XFS_DAS_RM_SHRINK, /* We are shrinking the tree */ + XFS_DAS_SF_REMOVE, /* Initial shortform set iter state */ + XFS_DAS_LEAF_REMOVE, /* Initial leaf form set iter state */ + XFS_DAS_NODE_REMOVE, /* Initial node form set iter state */ + /* Leaf state set/replace sequence */ XFS_DAS_LEAF_SET_RMT, /* set a remote xattr from a leaf */ XFS_DAS_LEAF_ALLOC_RMT, /* We are allocating remote blocks */ diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index 260760ce2d05..01b047d86cd1 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -4136,6 +4136,9 @@ TRACE_DEFINE_ENUM(XFS_DAS_NODE_ADD); TRACE_DEFINE_ENUM(XFS_DAS_RMTBLK); TRACE_DEFINE_ENUM(XFS_DAS_RM_NAME); TRACE_DEFINE_ENUM(XFS_DAS_RM_SHRINK); +TRACE_DEFINE_ENUM(XFS_DAS_SF_REMOVE); +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REMOVE); +TRACE_DEFINE_ENUM(XFS_DAS_NODE_REMOVE); TRACE_DEFINE_ENUM(XFS_DAS_LEAF_SET_RMT); TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ALLOC_RMT); TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REPLACE);