| Message ID | 20220509004138.762556-6-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:25AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We current use XFS_DAS_UNINIT for several steps in the attr_set > state machine. We use it for setting shortform xattrs, converting > from shortform to leaf, leaf add, leaf-to-node and leaf add. All of > these things are essentially known before we start the state machine > iterating, so we really should separate them out: > > XFS_DAS_SF_ADD: > - tries to do a shortform add > - on success -> done > - on ENOSPC converts to leaf, -> XFS_DAS_LEAF_ADD > - on error, dies. > > XFS_DAS_LEAF_ADD: > - tries to do leaf add > - on success: > - inline attr -> done > - remote xattr || REPLACE -> XFS_DAS_FOUND_LBLK > - on ENOSPC converts to node, -> XFS_DAS_NODE_ADD > - on error, dies > > XFS_DAS_NODE_ADD: > - tries to do node add > - on success: > - inline attr -> done > - remote xattr || REPLACE -> XFS_DAS_FOUND_NBLK > - on error, dies > > This makes it easier to understand how the state machine starts > up and sets us up on the path to further state machine > simplifications. Yes!! > This also converts the DAS state tracepoints to use strings rather > than numbers, as converting between enums and numbers requires > manual counting rather than just reading the name. > > This also introduces a XFS_DAS_DONE state so that we can trace > successful operation completions easily. Yes!! > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Allison Henderson<allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 161 +++++++++++++++++++------------------- > fs/xfs/libxfs/xfs_attr.h | 80 ++++++++++++++++--- > fs/xfs/libxfs/xfs_defer.c | 2 + > fs/xfs/xfs_acl.c | 4 +- > fs/xfs/xfs_attr_item.c | 13 ++- > fs/xfs/xfs_ioctl.c | 4 +- > fs/xfs/xfs_trace.h | 22 +++++- > fs/xfs/xfs_xattr.c | 2 +- > 8 files changed, 185 insertions(+), 103 deletions(-) > <snip> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index c9c867e3406c..ad52b5dc59e4 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); > int __init xfs_attrd_init_cache(void); > void xfs_attrd_destroy_cache(void); > > +/* > + * Check to see if the attr should be upgraded from non-existent or shortform to > + * single-leaf-block attribute list. > + */ > +static inline bool > +xfs_attr_is_shortform( > + struct xfs_inode *ip) > +{ > + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || > + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && > + ip->i_afp->if_nextents == 0); > +} > + > +static inline enum xfs_delattr_state > +xfs_attr_init_add_state(struct xfs_da_args *args) > +{ > + if (!args->dp->i_afp) > + return XFS_DAS_DONE; If we're in add/replace attr call without an attr fork, why do we go straight to finished? --D
On Tue, May 10, 2022 at 04:12:34PM -0700, Darrick J. Wong wrote: > On Mon, May 09, 2022 at 10:41:25AM +1000, Dave Chinner wrote: > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > index c9c867e3406c..ad52b5dc59e4 100644 > > --- a/fs/xfs/libxfs/xfs_attr.h > > +++ b/fs/xfs/libxfs/xfs_attr.h > > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); > > int __init xfs_attrd_init_cache(void); > > void xfs_attrd_destroy_cache(void); > > > > +/* > > + * Check to see if the attr should be upgraded from non-existent or shortform to > > + * single-leaf-block attribute list. > > + */ > > +static inline bool > > +xfs_attr_is_shortform( > > + struct xfs_inode *ip) > > +{ > > + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || > > + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && > > + ip->i_afp->if_nextents == 0); > > +} > > + > > +static inline enum xfs_delattr_state > > +xfs_attr_init_add_state(struct xfs_da_args *args) > > +{ > > + if (!args->dp->i_afp) > > + return XFS_DAS_DONE; > > If we're in add/replace attr call without an attr fork, why do we go > straight to finished? I suspect I've fixed all the issues that triggered crashes here because args->dp->i_afp was null. THere were transient states in a replace operaiton when the remove takes away the last attr, removes the attr fork, then calls the ADD operation. The add operation assumes that the attr fork has already been set up, and so bad things happened here. This also occurred when setting up recovery operations - recovery of an add/replace could start from that same "there's no attr fork" condition, and so calling xfs_inode_has_attr() or xfs_attr_is_shortform() direct from the reocovery setup code would go splat because ip->i_afp was null. I'm going to leave this for the moment (cleanup note made) because I don't want to have to find out that I missed a corner case somewhere they hard way right now. It's basically there to stop log recovery crashing hard, which only occurs when the experimental larp code is running, so I think this is safe to leave for a later cleanup. Cheers, Dave.
On Wed, May 11, 2022 at 11:06:51AM +1000, Dave Chinner wrote: > On Tue, May 10, 2022 at 04:12:34PM -0700, Darrick J. Wong wrote: > > On Mon, May 09, 2022 at 10:41:25AM +1000, Dave Chinner wrote: > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > index c9c867e3406c..ad52b5dc59e4 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); > > > int __init xfs_attrd_init_cache(void); > > > void xfs_attrd_destroy_cache(void); > > > > > > +/* > > > + * Check to see if the attr should be upgraded from non-existent or shortform to > > > + * single-leaf-block attribute list. > > > + */ > > > +static inline bool > > > +xfs_attr_is_shortform( > > > + struct xfs_inode *ip) > > > +{ > > > + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || > > > + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && > > > + ip->i_afp->if_nextents == 0); > > > +} > > > + > > > +static inline enum xfs_delattr_state > > > +xfs_attr_init_add_state(struct xfs_da_args *args) > > > +{ > > > + if (!args->dp->i_afp) > > > + return XFS_DAS_DONE; > > > > If we're in add/replace attr call without an attr fork, why do we go > > straight to finished? > > I suspect I've fixed all the issues that triggered crashes here > because args->dp->i_afp was null. THere were transient states in a > replace operaiton when the remove takes away the last attr, removes > the attr fork, then calls the ADD operation. The add operation > assumes that the attr fork has already been set up, and so bad > things happened here. > > This also occurred when setting up recovery operations - recovery of > an add/replace could start from that same "there's no attr fork" > condition, and so calling xfs_inode_has_attr() or > xfs_attr_is_shortform() direct from the reocovery setup code would > go splat because ip->i_afp was null. > > I'm going to leave this for the moment (cleanup note made) because I > don't want to have to find out that I missed a corner case somewhere > they hard way right now. It's basically there to stop log recovery > crashing hard, which only occurs when the experimental larp code is > running, so I think this is safe to leave for a later cleanup. Hmm, in that case, can this become: if (!args->dp->i_afp) { ASSERT(0); return XFS_DAS_DONE; } And then you can add: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Tue, May 10, 2022 at 06:08:48PM -0700, Darrick J. Wong wrote: > On Wed, May 11, 2022 at 11:06:51AM +1000, Dave Chinner wrote: > > On Tue, May 10, 2022 at 04:12:34PM -0700, Darrick J. Wong wrote: > > > On Mon, May 09, 2022 at 10:41:25AM +1000, Dave Chinner wrote: > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > index c9c867e3406c..ad52b5dc59e4 100644 > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); > > > > int __init xfs_attrd_init_cache(void); > > > > void xfs_attrd_destroy_cache(void); > > > > > > > > +/* > > > > + * Check to see if the attr should be upgraded from non-existent or shortform to > > > > + * single-leaf-block attribute list. > > > > + */ > > > > +static inline bool > > > > +xfs_attr_is_shortform( > > > > + struct xfs_inode *ip) > > > > +{ > > > > + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || > > > > + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && > > > > + ip->i_afp->if_nextents == 0); > > > > +} > > > > + > > > > +static inline enum xfs_delattr_state > > > > +xfs_attr_init_add_state(struct xfs_da_args *args) > > > > +{ > > > > + if (!args->dp->i_afp) > > > > + return XFS_DAS_DONE; > > > > > > If we're in add/replace attr call without an attr fork, why do we go > > > straight to finished? > > > > I suspect I've fixed all the issues that triggered crashes here > > because args->dp->i_afp was null. THere were transient states in a > > replace operaiton when the remove takes away the last attr, removes > > the attr fork, then calls the ADD operation. The add operation > > assumes that the attr fork has already been set up, and so bad > > things happened here. > > > > This also occurred when setting up recovery operations - recovery of > > an add/replace could start from that same "there's no attr fork" > > condition, and so calling xfs_inode_has_attr() or > > xfs_attr_is_shortform() direct from the reocovery setup code would > > go splat because ip->i_afp was null. > > > > I'm going to leave this for the moment (cleanup note made) because I > > don't want to have to find out that I missed a corner case somewhere > > they hard way right now. It's basically there to stop log recovery > > crashing hard, which only occurs when the experimental larp code is > > running, so I think this is safe to leave for a later cleanup. > > Hmm, in that case, can this become: > > if (!args->dp->i_afp) { > ASSERT(0); > return XFS_DAS_DONE; > } OK. > And then you can add: > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks! -Dave.
On Wed, May 11, 2022 at 11:38:51AM +1000, Dave Chinner wrote: > On Tue, May 10, 2022 at 06:08:48PM -0700, Darrick J. Wong wrote: > > On Wed, May 11, 2022 at 11:06:51AM +1000, Dave Chinner wrote: > > > On Tue, May 10, 2022 at 04:12:34PM -0700, Darrick J. Wong wrote: > > > > On Mon, May 09, 2022 at 10:41:25AM +1000, Dave Chinner wrote: > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > > index c9c867e3406c..ad52b5dc59e4 100644 > > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); > > > > > int __init xfs_attrd_init_cache(void); > > > > > void xfs_attrd_destroy_cache(void); > > > > > > > > > > +/* > > > > > + * Check to see if the attr should be upgraded from non-existent or shortform to > > > > > + * single-leaf-block attribute list. > > > > > + */ > > > > > +static inline bool > > > > > +xfs_attr_is_shortform( > > > > > + struct xfs_inode *ip) > > > > > +{ > > > > > + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || > > > > > + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && > > > > > + ip->i_afp->if_nextents == 0); > > > > > +} > > > > > + > > > > > +static inline enum xfs_delattr_state > > > > > +xfs_attr_init_add_state(struct xfs_da_args *args) > > > > > +{ > > > > > + if (!args->dp->i_afp) > > > > > + return XFS_DAS_DONE; > > > > > > > > If we're in add/replace attr call without an attr fork, why do we go > > > > straight to finished? > > > > > > I suspect I've fixed all the issues that triggered crashes here > > > because args->dp->i_afp was null. THere were transient states in a > > > replace operaiton when the remove takes away the last attr, removes > > > the attr fork, then calls the ADD operation. The add operation > > > assumes that the attr fork has already been set up, and so bad > > > things happened here. > > > > > > This also occurred when setting up recovery operations - recovery of > > > an add/replace could start from that same "there's no attr fork" > > > condition, and so calling xfs_inode_has_attr() or > > > xfs_attr_is_shortform() direct from the reocovery setup code would > > > go splat because ip->i_afp was null. > > > > > > I'm going to leave this for the moment (cleanup note made) because I > > > don't want to have to find out that I missed a corner case somewhere > > > they hard way right now. It's basically there to stop log recovery > > > crashing hard, which only occurs when the experimental larp code is > > > running, so I think this is safe to leave for a later cleanup. > > > > Hmm, in that case, can this become: > > > > if (!args->dp->i_afp) { > > ASSERT(0); > > return XFS_DAS_DONE; > > } > > OK. Ok, now generic/051 has reminded me exactly what this was for. Shortform attr remove will remove the attr and the attr fork from this code: case XFS_DAS_SF_REMOVE: error = xfs_attr_sf_removename(args); attr->xattri_dela_state = xfs_attr_complete_op(attr, xfs_attr_init_add_state(args)); break; But if we are doing this as part of a REPLACE operation and we still need to add the new attr, it calls xfs_attr_init_add_state() to get the add state we should start with. That then hits the null args->dp->i_afp case because the fork got removed. This can't happen if we are doing a replace op, so we'd then check if it's a shortform attr fork and return XFS_DAS_SF_ADD for the replace to then execute. But it's not a replace op, so we can have a null attr fork. I'm going to restore the old code with a comment so that I don't forget this again. /* * If called from the completion of a attr remove to determine * the next state, the attribute fork may be null. This can occur on * a pure remove, but we grab the next state before we check if a * replace operation is being performed. Hence if the attr fork is * null, it's a pure remove operation and we are done. */ Cheers, Dave.
On Wed, May 11, 2022 at 06:35:13PM +1000, Dave Chinner wrote: > On Wed, May 11, 2022 at 11:38:51AM +1000, Dave Chinner wrote: > > On Tue, May 10, 2022 at 06:08:48PM -0700, Darrick J. Wong wrote: > > > On Wed, May 11, 2022 at 11:06:51AM +1000, Dave Chinner wrote: > > > > On Tue, May 10, 2022 at 04:12:34PM -0700, Darrick J. Wong wrote: > > > > > On Mon, May 09, 2022 at 10:41:25AM +1000, Dave Chinner wrote: > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > > > index c9c867e3406c..ad52b5dc59e4 100644 > > > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > > > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); > > > > > > int __init xfs_attrd_init_cache(void); > > > > > > void xfs_attrd_destroy_cache(void); > > > > > > > > > > > > +/* > > > > > > + * Check to see if the attr should be upgraded from non-existent or shortform to > > > > > > + * single-leaf-block attribute list. > > > > > > + */ > > > > > > +static inline bool > > > > > > +xfs_attr_is_shortform( > > > > > > + struct xfs_inode *ip) > > > > > > +{ > > > > > > + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || > > > > > > + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && > > > > > > + ip->i_afp->if_nextents == 0); > > > > > > +} > > > > > > + > > > > > > +static inline enum xfs_delattr_state > > > > > > +xfs_attr_init_add_state(struct xfs_da_args *args) > > > > > > +{ > > > > > > + if (!args->dp->i_afp) > > > > > > + return XFS_DAS_DONE; > > > > > > > > > > If we're in add/replace attr call without an attr fork, why do we go > > > > > straight to finished? > > > > > > > > I suspect I've fixed all the issues that triggered crashes here > > > > because args->dp->i_afp was null. THere were transient states in a > > > > replace operaiton when the remove takes away the last attr, removes > > > > the attr fork, then calls the ADD operation. The add operation > > > > assumes that the attr fork has already been set up, and so bad > > > > things happened here. > > > > > > > > This also occurred when setting up recovery operations - recovery of > > > > an add/replace could start from that same "there's no attr fork" > > > > condition, and so calling xfs_inode_has_attr() or > > > > xfs_attr_is_shortform() direct from the reocovery setup code would > > > > go splat because ip->i_afp was null. > > > > > > > > I'm going to leave this for the moment (cleanup note made) because I > > > > don't want to have to find out that I missed a corner case somewhere > > > > they hard way right now. It's basically there to stop log recovery > > > > crashing hard, which only occurs when the experimental larp code is > > > > running, so I think this is safe to leave for a later cleanup. > > > > > > Hmm, in that case, can this become: > > > > > > if (!args->dp->i_afp) { > > > ASSERT(0); > > > return XFS_DAS_DONE; > > > } > > > > OK. > > Ok, now generic/051 has reminded me exactly what this was for. > > Shortform attr remove will remove the attr and the attr fork from > this code: > > case XFS_DAS_SF_REMOVE: > error = xfs_attr_sf_removename(args); > attr->xattri_dela_state = xfs_attr_complete_op(attr, > xfs_attr_init_add_state(args)); > break; > > But if we are doing this as part of a REPLACE operation and we > still need to add the new attr, it calls xfs_attr_init_add_state() > to get the add state we should start with. That then hits the > null args->dp->i_afp case because the fork got removed. > > This can't happen if we are doing a replace op, so we'd then check > if it's a shortform attr fork and return XFS_DAS_SF_ADD for the > replace to then execute. But it's not a replace op, so we can > have a null attr fork. > > I'm going to restore the old code with a comment so that I don't > forget this again. > > /* > * If called from the completion of a attr remove to determine > * the next state, the attribute fork may be null. This can occur on > * a pure remove, but we grab the next state before we check if a > * replace operation is being performed. Hence if the attr fork is > * null, it's a pure remove operation and we are done. > */ Ahh, I see -- sf_removename will /never/ kill i_afp if we're doing a DA_OP_REPLACE or ADDNAME, and leaf_removename also won't do that if we're doing DA_OP_REPLACE. IOWs, only a removexattr operation can result in i_afp being freed. And the XATTR_CREATE operation always guarantee that i_afp is non-null before we start, so xfs_attr_defer_add should never be called with args->dp->i_afp == NULL, hence it'll never hit that state. Would you mind adding a sentence to the comment? "A pure create ensures the existence of i_afp and never encounters this state." FBO of maintainers who aren't quite as uptodate on how xattrs work? ;) (Admittedly all this will probably go away if we stop freeing i_afp, but I wasn't going to push on that until the LARP stuff settles...) --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, May 11, 2022 at 08:39:52AM -0700, Darrick J. Wong wrote: > On Wed, May 11, 2022 at 06:35:13PM +1000, Dave Chinner wrote: > > On Wed, May 11, 2022 at 11:38:51AM +1000, Dave Chinner wrote: > > > On Tue, May 10, 2022 at 06:08:48PM -0700, Darrick J. Wong wrote: > > > > On Wed, May 11, 2022 at 11:06:51AM +1000, Dave Chinner wrote: > > > > > I'm going to leave this for the moment (cleanup note made) because I > > > > > don't want to have to find out that I missed a corner case somewhere > > > > > they hard way right now. It's basically there to stop log recovery > > > > > crashing hard, which only occurs when the experimental larp code is > > > > > running, so I think this is safe to leave for a later cleanup. > > > > > > > > Hmm, in that case, can this become: > > > > > > > > if (!args->dp->i_afp) { > > > > ASSERT(0); > > > > return XFS_DAS_DONE; > > > > } > > > > > > OK. > > > > Ok, now generic/051 has reminded me exactly what this was for. > > > > Shortform attr remove will remove the attr and the attr fork from > > this code: > > > > case XFS_DAS_SF_REMOVE: > > error = xfs_attr_sf_removename(args); > > attr->xattri_dela_state = xfs_attr_complete_op(attr, > > xfs_attr_init_add_state(args)); > > break; > > > > But if we are doing this as part of a REPLACE operation and we > > still need to add the new attr, it calls xfs_attr_init_add_state() > > to get the add state we should start with. That then hits the > > null args->dp->i_afp case because the fork got removed. > > > > This can't happen if we are doing a replace op, so we'd then check > > if it's a shortform attr fork and return XFS_DAS_SF_ADD for the > > replace to then execute. But it's not a replace op, so we can > > have a null attr fork. > > > > I'm going to restore the old code with a comment so that I don't > > forget this again. > > > > /* > > * If called from the completion of a attr remove to determine > > * the next state, the attribute fork may be null. This can occur on > > * a pure remove, but we grab the next state before we check if a > > * replace operation is being performed. Hence if the attr fork is > > * null, it's a pure remove operation and we are done. > > */ > > Ahh, I see -- sf_removename will /never/ kill i_afp if we're doing a > DA_OP_REPLACE or ADDNAME, and leaf_removename also won't do that if > we're doing DA_OP_REPLACE. IOWs, only a removexattr operation can > result in i_afp being freed. > > And the XATTR_CREATE operation always guarantee that i_afp is non-null > before we start, so xfs_attr_defer_add should never be called with > args->dp->i_afp == NULL, hence it'll never hit that state. > > Would you mind adding a sentence to the comment? > > "A pure create ensures the existence of i_afp and never encounters this > state." Sure. -Dave.
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 817e15740f9c..edc31075fde4 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -59,7 +59,7 @@ STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp); */ STATIC int xfs_attr_node_get(xfs_da_args_t *args); STATIC void xfs_attr_restore_rmt_blk(struct xfs_da_args *args); -STATIC int xfs_attr_node_addname(struct xfs_attr_item *attr); +static int xfs_attr_node_try_addname(struct xfs_attr_item *attr); STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_item *attr); STATIC int xfs_attr_node_addname_clear_incomplete(struct xfs_attr_item *attr); STATIC int xfs_attr_node_hasname(xfs_da_args_t *args, @@ -224,6 +224,11 @@ xfs_init_attr_trans( } } +/* + * Add an attr to a shortform fork. If there is no space, + * xfs_attr_shortform_addname() will convert to leaf format and return -ENOSPC. + * to use. + */ STATIC int xfs_attr_try_sf_addname( struct xfs_inode *dp, @@ -255,20 +260,7 @@ xfs_attr_try_sf_addname( return error; } -/* - * Check to see if the attr should be upgraded from non-existent or shortform to - * single-leaf-block attribute list. - */ -static inline bool -xfs_attr_is_shortform( - struct xfs_inode *ip) -{ - return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || - (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && - ip->i_afp->if_nextents == 0); -} - -STATIC int +static int xfs_attr_sf_addname( struct xfs_attr_item *attr) { @@ -276,14 +268,12 @@ xfs_attr_sf_addname( struct xfs_inode *dp = args->dp; int error = 0; - /* - * Try to add the attr to the attribute list in the inode. - */ error = xfs_attr_try_sf_addname(dp, args); - - /* Should only be 0, -EEXIST or -ENOSPC */ - if (error != -ENOSPC) - return error; + if (error != -ENOSPC) { + ASSERT(!error || error == -EEXIST); + attr->xattri_dela_state = XFS_DAS_DONE; + goto out; + } /* * It won't fit in the shortform, transform to a leaf block. GROT: @@ -299,64 +289,42 @@ xfs_attr_sf_addname( * with the write verifier. */ xfs_trans_bhold(args->trans, attr->xattri_leaf_bp); - - /* - * We're still in XFS_DAS_UNINIT state here. We've converted - * the attr fork to leaf format and will restart with the leaf - * add. - */ - trace_xfs_attr_sf_addname_return(XFS_DAS_UNINIT, args->dp); - return -EAGAIN; + 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; } -STATIC int +static int xfs_attr_leaf_addname( struct xfs_attr_item *attr) { struct xfs_da_args *args = attr->xattri_da_args; - struct xfs_inode *dp = args->dp; - enum xfs_delattr_state next_state = XFS_DAS_UNINIT; int error; - if (xfs_attr_is_leaf(dp)) { - - /* - * Use the leaf buffer we may already hold locked as a result of - * a sf-to-leaf conversion. The held buffer is no longer valid - * after this call, regardless of the result. - */ - error = xfs_attr_leaf_try_add(args, attr->xattri_leaf_bp); - attr->xattri_leaf_bp = NULL; + ASSERT(xfs_attr_is_leaf(args->dp)); - if (error == -ENOSPC) { - error = xfs_attr3_leaf_to_node(args); - if (error) - return error; - - /* - * Finish any deferred work items and roll the - * transaction once more. The goal here is to call - * node_addname with the inode and transaction in the - * same state (inode locked and joined, transaction - * clean) no matter how we got to this step. - * - * At this point, we are still in XFS_DAS_UNINIT, but - * when we come back, we'll be a node, so we'll fall - * down into the node handling code below - */ - error = -EAGAIN; - goto out; - } - next_state = XFS_DAS_FOUND_LBLK; - } else { - ASSERT(!attr->xattri_leaf_bp); + /* + * Use the leaf buffer we may already hold locked as a result of + * a sf-to-leaf conversion. The held buffer is no longer valid + * after this call, regardless of the result. + */ + error = xfs_attr_leaf_try_add(args, attr->xattri_leaf_bp); + attr->xattri_leaf_bp = NULL; - error = xfs_attr_node_addname_find_attr(attr); + if (error == -ENOSPC) { + error = xfs_attr3_leaf_to_node(args); if (error) return error; - next_state = XFS_DAS_FOUND_NBLK; - error = xfs_attr_node_addname(attr); + /* + * We're not in leaf format anymore, so roll the transaction and + * retry the add to the newly allocated node block. + */ + attr->xattri_dela_state = XFS_DAS_NODE_ADD; + error = -EAGAIN; + goto out; } if (error) return error; @@ -368,15 +336,46 @@ xfs_attr_leaf_addname( */ if (args->rmtblkno || (args->op_flags & XFS_DA_OP_RENAME)) { - attr->xattri_dela_state = next_state; + attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; error = -EAGAIN; + } else { + attr->xattri_dela_state = XFS_DAS_DONE; } - out: trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); return error; } +static int +xfs_attr_node_addname( + struct xfs_attr_item *attr) +{ + struct xfs_da_args *args = attr->xattri_da_args; + int error; + + ASSERT(!attr->xattri_leaf_bp); + + error = xfs_attr_node_addname_find_attr(attr); + if (error) + return error; + + error = xfs_attr_node_try_addname(attr); + if (error) + return error; + + if (args->rmtblkno || + (args->op_flags & XFS_DA_OP_RENAME)) { + attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; + error = -EAGAIN; + } else { + attr->xattri_dela_state = XFS_DAS_DONE; + } + + trace_xfs_attr_node_addname_return(attr->xattri_dela_state, args->dp); + return error; +} + + /* * Set the attribute specified in @args. * This routine is meant to function as a delayed operation, and may return @@ -397,16 +396,14 @@ xfs_attr_set_iter( /* State machine switch */ switch (attr->xattri_dela_state) { case XFS_DAS_UNINIT: - /* - * If the fork is shortform, attempt to add the attr. If there - * is no space, this converts to leaf format and returns - * -EAGAIN with the leaf buffer held across the roll. The caller - * will deal with a transaction roll error, but otherwise - * release the hold once we return with a clean transaction. - */ - if (xfs_attr_is_shortform(dp)) - return xfs_attr_sf_addname(attr); + ASSERT(0); + return -EFSCORRUPTED; + case XFS_DAS_SF_ADD: + return xfs_attr_sf_addname(attr); + case XFS_DAS_LEAF_ADD: return xfs_attr_leaf_addname(attr); + case XFS_DAS_NODE_ADD: + return xfs_attr_node_addname(attr); case XFS_DAS_FOUND_LBLK: /* @@ -700,7 +697,7 @@ xfs_attr_defer_add( if (error) return error; - new->xattri_dela_state = XFS_DAS_UNINIT; + new->xattri_dela_state = xfs_attr_init_add_state(args); xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp); @@ -719,7 +716,7 @@ xfs_attr_defer_replace( if (error) return error; - new->xattri_dela_state = XFS_DAS_UNINIT; + new->xattri_dela_state = xfs_attr_init_replace_state(args); xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); trace_xfs_attr_defer_replace(new->xattri_dela_state, args->dp); @@ -1262,8 +1259,8 @@ xfs_attr_node_addname_find_attr( * to handle this, and recall the function until a successful error code is *returned. */ -STATIC int -xfs_attr_node_addname( +static int +xfs_attr_node_try_addname( struct xfs_attr_item *attr) { struct xfs_da_args *args = attr->xattri_da_args; diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index c9c867e3406c..ad52b5dc59e4 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -443,21 +443,44 @@ struct xfs_attr_list_context { * to where it was and resume executing where it left off. */ enum xfs_delattr_state { - XFS_DAS_UNINIT = 0, /* No state has been set yet */ - XFS_DAS_RMTBLK, /* Removing remote blks */ - XFS_DAS_RM_NAME, /* Remove attr name */ - XFS_DAS_RM_SHRINK, /* We are shrinking the tree */ - XFS_DAS_FOUND_LBLK, /* We found leaf blk for attr */ - XFS_DAS_FOUND_NBLK, /* We found node blk for attr */ - 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 */ - XFS_DAS_ALLOC_NODE, /* We are allocating node 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_UNINIT = 0, /* No state has been set yet */ + XFS_DAS_SF_ADD, /* Initial shortform set iter state */ + XFS_DAS_LEAF_ADD, /* Initial leaf form set iter state */ + XFS_DAS_NODE_ADD, /* Initial node form set iter state */ + XFS_DAS_RMTBLK, /* Removing remote blks */ + XFS_DAS_RM_NAME, /* Remove attr name */ + XFS_DAS_RM_SHRINK, /* We are shrinking the tree */ + XFS_DAS_FOUND_LBLK, /* We found leaf blk for attr */ + XFS_DAS_FOUND_NBLK, /* We found node blk for attr */ + 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 */ + XFS_DAS_ALLOC_NODE, /* We are allocating node 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 */ }; +#define XFS_DAS_STRINGS \ + { XFS_DAS_UNINIT, "XFS_DAS_UNINIT" }, \ + { XFS_DAS_SF_ADD, "XFS_DAS_SF_ADD" }, \ + { XFS_DAS_LEAF_ADD, "XFS_DAS_LEAF_ADD" }, \ + { XFS_DAS_NODE_ADD, "XFS_DAS_NODE_ADD" }, \ + { XFS_DAS_RMTBLK, "XFS_DAS_RMTBLK" }, \ + { XFS_DAS_RM_NAME, "XFS_DAS_RM_NAME" }, \ + { XFS_DAS_RM_SHRINK, "XFS_DAS_RM_SHRINK" }, \ + { XFS_DAS_FOUND_LBLK, "XFS_DAS_FOUND_LBLK" }, \ + { XFS_DAS_FOUND_NBLK, "XFS_DAS_FOUND_NBLK" }, \ + { XFS_DAS_FLIP_LFLAG, "XFS_DAS_FLIP_LFLAG" }, \ + { XFS_DAS_RM_LBLK, "XFS_DAS_RM_LBLK" }, \ + { XFS_DAS_RD_LEAF, "XFS_DAS_RD_LEAF" }, \ + { XFS_DAS_ALLOC_NODE, "XFS_DAS_ALLOC_NODE" }, \ + { XFS_DAS_FLIP_NFLAG, "XFS_DAS_FLIP_NFLAG" }, \ + { XFS_DAS_RM_NBLK, "XFS_DAS_RM_NBLK" }, \ + { XFS_DAS_CLR_FLAG, "XFS_DAS_CLR_FLAG" }, \ + { XFS_DAS_DONE, "XFS_DAS_DONE" } + /* * Defines for xfs_attr_item.xattri_flags */ @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void); int __init xfs_attrd_init_cache(void); void xfs_attrd_destroy_cache(void); +/* + * Check to see if the attr should be upgraded from non-existent or shortform to + * single-leaf-block attribute list. + */ +static inline bool +xfs_attr_is_shortform( + struct xfs_inode *ip) +{ + return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL || + (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS && + ip->i_afp->if_nextents == 0); +} + +static inline enum xfs_delattr_state +xfs_attr_init_add_state(struct xfs_da_args *args) +{ + if (!args->dp->i_afp) + return XFS_DAS_DONE; + if (xfs_attr_is_shortform(args->dp)) + return XFS_DAS_SF_ADD; + if (xfs_attr_is_leaf(args->dp)) + return XFS_DAS_LEAF_ADD; + return XFS_DAS_NODE_ADD; +} + +static inline enum xfs_delattr_state +xfs_attr_init_replace_state(struct xfs_da_args *args) +{ + return xfs_attr_init_add_state(args); +} + #endif /* __XFS_ATTR_H__ */ diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c index b2ecc272f9e4..ceb222b4f261 100644 --- a/fs/xfs/libxfs/xfs_defer.c +++ b/fs/xfs/libxfs/xfs_defer.c @@ -23,6 +23,8 @@ #include "xfs_bmap.h" #include "xfs_alloc.h" #include "xfs_buf.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" static struct kmem_cache *xfs_defer_pending_cache; diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c index 5c52ee869272..3df9c1782ead 100644 --- a/fs/xfs/xfs_acl.c +++ b/fs/xfs/xfs_acl.c @@ -10,12 +10,12 @@ #include "xfs_trans_resv.h" #include "xfs_mount.h" #include "xfs_inode.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_trace.h" #include "xfs_error.h" #include "xfs_acl.h" -#include "xfs_da_format.h" -#include "xfs_da_btree.h" #include "xfs_trans.h" #include <linux/posix_acl_xattr.h> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c index fe1e37696634..5bfb33746e37 100644 --- a/fs/xfs/xfs_attr_item.c +++ b/fs/xfs/xfs_attr_item.c @@ -570,10 +570,21 @@ xfs_attri_item_recover( args->hashval = xfs_da_hashname(args->name, args->namelen); args->attr_filter = attrp->alfi_attr_flags; - if (attrp->alfi_op_flags == XFS_ATTR_OP_FLAGS_SET) { + switch (attrp->alfi_op_flags & XFS_ATTR_OP_FLAGS_TYPE_MASK) { + case XFS_ATTR_OP_FLAGS_SET: + case XFS_ATTR_OP_FLAGS_REPLACE: args->value = attrip->attri_value; args->valuelen = attrp->alfi_value_len; args->total = xfs_attr_calc_size(args, &local); + attr->xattri_dela_state = xfs_attr_init_add_state(args); + break; + case XFS_ATTR_OP_FLAGS_REMOVE: + attr->xattri_dela_state = XFS_DAS_UNINIT; + break; + default: + ASSERT(0); + error = -EFSCORRUPTED; + goto out; } xfs_init_attr_trans(args, &tres, &total); diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index e9eadc7337ce..0e5cb7936206 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -15,6 +15,8 @@ #include "xfs_iwalk.h" #include "xfs_itable.h" #include "xfs_error.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_bmap.h" #include "xfs_bmap_util.h" @@ -35,8 +37,6 @@ #include "xfs_health.h" #include "xfs_reflink.h" #include "xfs_ioctl.h" -#include "xfs_da_format.h" -#include "xfs_da_btree.h" #include <linux/mount.h> #include <linux/namei.h> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index 01ce0401aa32..8f722be25c29 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -4129,6 +4129,23 @@ DEFINE_ICLOG_EVENT(xlog_iclog_want_sync); DEFINE_ICLOG_EVENT(xlog_iclog_wait_on); DEFINE_ICLOG_EVENT(xlog_iclog_write); +TRACE_DEFINE_ENUM(XFS_DAS_UNINIT); +TRACE_DEFINE_ENUM(XFS_DAS_SF_ADD); +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ADD); +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_FOUND_LBLK); +TRACE_DEFINE_ENUM(XFS_DAS_FOUND_NBLK); +TRACE_DEFINE_ENUM(XFS_DAS_FLIP_LFLAG); +TRACE_DEFINE_ENUM(XFS_DAS_RM_LBLK); +TRACE_DEFINE_ENUM(XFS_DAS_RD_LEAF); +TRACE_DEFINE_ENUM(XFS_DAS_ALLOC_NODE); +TRACE_DEFINE_ENUM(XFS_DAS_FLIP_NFLAG); +TRACE_DEFINE_ENUM(XFS_DAS_RM_NBLK); +TRACE_DEFINE_ENUM(XFS_DAS_CLR_FLAG); + DECLARE_EVENT_CLASS(xfs_das_state_class, TP_PROTO(int das, struct xfs_inode *ip), TP_ARGS(das, ip), @@ -4140,8 +4157,9 @@ DECLARE_EVENT_CLASS(xfs_das_state_class, __entry->das = das; __entry->ino = ip->i_ino; ), - TP_printk("state change %d ino 0x%llx", - __entry->das, __entry->ino) + TP_printk("state change %s ino 0x%llx", + __print_symbolic(__entry->das, XFS_DAS_STRINGS), + __entry->ino) ) #define DEFINE_DAS_STATE_EVENT(name) \ diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index 0d050f8829ef..7a044afd4c46 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -12,9 +12,9 @@ #include "xfs_trans_resv.h" #include "xfs_mount.h" #include "xfs_inode.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_acl.h" -#include "xfs_da_btree.h" #include <linux/posix_acl_xattr.h>