| Message ID | 20200403221229.4995-12-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Fri, Apr 03, 2020 at 03:12:20PM -0700, Allison Collins wrote: > This patch adds a new helper function xfs_attr_node_shrink used to > shrink an attr name into an inode if it is small enough. This helps to > modularize the greater calling function xfs_attr_node_removename. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++------------------ > 1 file changed, 42 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index db5a99c..27a9bb5 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c ... > @@ -1197,31 +1235,10 @@ xfs_attr_node_removename( > /* > * If the result is small enough, push it all into the inode. > */ > - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > - /* > - * Have to get rid of the copy of this dabuf in the state. > - */ > - ASSERT(state->path.active == 1); > - ASSERT(state->path.blk[0].bp); > - state->path.blk[0].bp = NULL; > - > - error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); > - if (error) > - goto out; > + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > + error = xfs_attr_node_shrink(args, state); > > - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) { > - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > - /* bp is gone due to xfs_da_shrink_inode */ > - if (error) > - goto out; > - error = xfs_defer_finish(&args->trans); > - if (error) > - goto out; > - } else > - xfs_trans_brelse(args->trans, bp); > - } > error = 0; Looks like the error handling is busted here..? We used to goto out, now we always reset error to 0. Brian > - > out: > if (state) > xfs_da_state_free(state); > -- > 2.7.4 >
On 4/7/20 7:17 AM, Brian Foster wrote: > On Fri, Apr 03, 2020 at 03:12:20PM -0700, Allison Collins wrote: >> This patch adds a new helper function xfs_attr_node_shrink used to >> shrink an attr name into an inode if it is small enough. This helps to >> modularize the greater calling function xfs_attr_node_removename. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++------------------ >> 1 file changed, 42 insertions(+), 25 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index db5a99c..27a9bb5 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c > ... >> @@ -1197,31 +1235,10 @@ xfs_attr_node_removename( >> /* >> * If the result is small enough, push it all into the inode. >> */ >> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> - /* >> - * Have to get rid of the copy of this dabuf in the state. >> - */ >> - ASSERT(state->path.active == 1); >> - ASSERT(state->path.blk[0].bp); >> - state->path.blk[0].bp = NULL; >> - >> - error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); >> - if (error) >> - goto out; >> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> + error = xfs_attr_node_shrink(args, state); >> >> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) { >> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> - /* bp is gone due to xfs_da_shrink_inode */ >> - if (error) >> - goto out; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - goto out; >> - } else >> - xfs_trans_brelse(args->trans, bp); >> - } >> error = 0; > > Looks like the error handling is busted here..? We used to goto out, now > we always reset error to 0. > > Brian Ah, yes the error = 0; should have been removed along with the rest of it. Will remove. Thanks! Allison > >> - >> out: >> if (state) >> xfs_da_state_free(state); >> -- >> 2.7.4 >> >
On Saturday, April 4, 2020 3:42 AM Allison Collins wrote: > This patch adds a new helper function xfs_attr_node_shrink used to > shrink an attr name into an inode if it is small enough. This helps to > modularize the greater calling function xfs_attr_node_removename. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++------------------ > 1 file changed, 42 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index db5a99c..27a9bb5 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1103,6 +1103,45 @@ xfs_attr_node_addname( > } > > /* > + * Shrink an attribute from leaf to shortform > + */ > +STATIC int > +xfs_attr_node_shrink( > + struct xfs_da_args *args, > + struct xfs_da_state *state) > +{ > + struct xfs_inode *dp = args->dp; > + int error, forkoff; > + struct xfs_buf *bp; > + > + /* > + * Have to get rid of the copy of this dabuf in the state. > + */ > + ASSERT(state->path.active == 1); > + ASSERT(state->path.blk[0].bp); > + state->path.blk[0].bp = NULL; > + > + error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); > + if (error) > + return error; > + > + forkoff = xfs_attr_shortform_allfit(bp, dp); > + if (forkoff) { > + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > + /* bp is gone due to xfs_da_shrink_inode */ > + if (error) > + return error; > + > + error = xfs_defer_finish(&args->trans); > + if (error) > + return error; > + } else > + xfs_trans_brelse(args->trans, bp); > + > + return 0; > +} > + > +/* > * Remove a name from a B-tree attribute list. > * > * This will involve walking down the Btree, and may involve joining > @@ -1115,8 +1154,7 @@ xfs_attr_node_removename( > { > struct xfs_da_state *state; > struct xfs_da_state_blk *blk; > - struct xfs_buf *bp; > - int retval, error, forkoff; > + int retval, error; > struct xfs_inode *dp = args->dp; > > trace_xfs_attr_node_removename(args); > @@ -1197,31 +1235,10 @@ xfs_attr_node_removename( > /* > * If the result is small enough, push it all into the inode. > */ > - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > - /* > - * Have to get rid of the copy of this dabuf in the state. > - */ > - ASSERT(state->path.active == 1); > - ASSERT(state->path.blk[0].bp); > - state->path.blk[0].bp = NULL; > - > - error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); > - if (error) > - goto out; > + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > + error = xfs_attr_node_shrink(args, state); If a non-zero error value is returned by the above statement, the following statement i.e. "error = 0" will overwrite it. Apart from that everything else looks fine. Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > > - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) { > - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > - /* bp is gone due to xfs_da_shrink_inode */ > - if (error) > - goto out; > - error = xfs_defer_finish(&args->trans); > - if (error) > - goto out; > - } else > - xfs_trans_brelse(args->trans, bp); > - } > error = 0; > - > out: > if (state) > xfs_da_state_free(state); >
On 4/15/20 3:16 AM, Chandan Rajendra wrote: > On Saturday, April 4, 2020 3:42 AM Allison Collins wrote: >> This patch adds a new helper function xfs_attr_node_shrink used to >> shrink an attr name into an inode if it is small enough. This helps to >> modularize the greater calling function xfs_attr_node_removename. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++------------------ >> 1 file changed, 42 insertions(+), 25 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index db5a99c..27a9bb5 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1103,6 +1103,45 @@ xfs_attr_node_addname( >> } >> >> /* >> + * Shrink an attribute from leaf to shortform >> + */ >> +STATIC int >> +xfs_attr_node_shrink( >> + struct xfs_da_args *args, >> + struct xfs_da_state *state) >> +{ >> + struct xfs_inode *dp = args->dp; >> + int error, forkoff; >> + struct xfs_buf *bp; >> + >> + /* >> + * Have to get rid of the copy of this dabuf in the state. >> + */ >> + ASSERT(state->path.active == 1); >> + ASSERT(state->path.blk[0].bp); >> + state->path.blk[0].bp = NULL; >> + >> + error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); >> + if (error) >> + return error; >> + >> + forkoff = xfs_attr_shortform_allfit(bp, dp); >> + if (forkoff) { >> + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> + /* bp is gone due to xfs_da_shrink_inode */ >> + if (error) >> + return error; >> + >> + error = xfs_defer_finish(&args->trans); >> + if (error) >> + return error; >> + } else >> + xfs_trans_brelse(args->trans, bp); >> + >> + return 0; >> +} >> + >> +/* >> * Remove a name from a B-tree attribute list. >> * >> * This will involve walking down the Btree, and may involve joining >> @@ -1115,8 +1154,7 @@ xfs_attr_node_removename( >> { >> struct xfs_da_state *state; >> struct xfs_da_state_blk *blk; >> - struct xfs_buf *bp; >> - int retval, error, forkoff; >> + int retval, error; >> struct xfs_inode *dp = args->dp; >> >> trace_xfs_attr_node_removename(args); >> @@ -1197,31 +1235,10 @@ xfs_attr_node_removename( >> /* >> * If the result is small enough, push it all into the inode. >> */ >> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> - /* >> - * Have to get rid of the copy of this dabuf in the state. >> - */ >> - ASSERT(state->path.active == 1); >> - ASSERT(state->path.blk[0].bp); >> - state->path.blk[0].bp = NULL; >> - >> - error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); >> - if (error) >> - goto out; >> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> + error = xfs_attr_node_shrink(args, state); > > If a non-zero error value is returned by the above statement, the following > statement i.e. "error = 0" will overwrite it. > Apart from that everything else looks fine. > > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> Yep, I've removed the trailing error = 0; in the new v9. Thanks for the review! Allison > >> >> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) { >> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> - /* bp is gone due to xfs_da_shrink_inode */ >> - if (error) >> - goto out; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - goto out; >> - } else >> - xfs_trans_brelse(args->trans, bp); >> - } >> error = 0; >> - >> out: >> if (state) >> xfs_da_state_free(state); >> > >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index db5a99c..27a9bb5 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1103,6 +1103,45 @@ xfs_attr_node_addname( } /* + * Shrink an attribute from leaf to shortform + */ +STATIC int +xfs_attr_node_shrink( + struct xfs_da_args *args, + struct xfs_da_state *state) +{ + struct xfs_inode *dp = args->dp; + int error, forkoff; + struct xfs_buf *bp; + + /* + * Have to get rid of the copy of this dabuf in the state. + */ + ASSERT(state->path.active == 1); + ASSERT(state->path.blk[0].bp); + state->path.blk[0].bp = NULL; + + error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); + if (error) + return error; + + forkoff = xfs_attr_shortform_allfit(bp, dp); + if (forkoff) { + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); + /* bp is gone due to xfs_da_shrink_inode */ + if (error) + return error; + + error = xfs_defer_finish(&args->trans); + if (error) + return error; + } else + xfs_trans_brelse(args->trans, bp); + + return 0; +} + +/* * Remove a name from a B-tree attribute list. * * This will involve walking down the Btree, and may involve joining @@ -1115,8 +1154,7 @@ xfs_attr_node_removename( { struct xfs_da_state *state; struct xfs_da_state_blk *blk; - struct xfs_buf *bp; - int retval, error, forkoff; + int retval, error; struct xfs_inode *dp = args->dp; trace_xfs_attr_node_removename(args); @@ -1197,31 +1235,10 @@ xfs_attr_node_removename( /* * If the result is small enough, push it all into the inode. */ - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { - /* - * Have to get rid of the copy of this dabuf in the state. - */ - ASSERT(state->path.active == 1); - ASSERT(state->path.blk[0].bp); - state->path.blk[0].bp = NULL; - - error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp); - if (error) - goto out; + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) + error = xfs_attr_node_shrink(args, state); - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) { - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); - /* bp is gone due to xfs_da_shrink_inode */ - if (error) - goto out; - error = xfs_defer_finish(&args->trans); - if (error) - goto out; - } else - xfs_trans_brelse(args->trans, bp); - } error = 0; - out: if (state) xfs_da_state_free(state);
This patch adds a new helper function xfs_attr_node_shrink used to shrink an attr name into an inode if it is small enough. This helps to modularize the greater calling function xfs_attr_node_removename. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 25 deletions(-)