| Message ID | 20200223020611.1802-16-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delayed Ready Attrs | expand |
On Sun, Feb 23, 2020 at 4:07 AM Allison Collins <allison.henderson@oracle.com> 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 | 66 +++++++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 26 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 4b788f2..30a16fe 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1366,6 +1366,43 @@ 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; > + > + args->dac.flags |= XFS_DAC_FINISH_TRANS; Why did xfs_defer_finish(&args->trans); turn into the above? Are you testing reviewers alertness? ;-) Please keep logic preserving patches separate from logic change patches. Thanks, Amir. > + } 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 > @@ -1383,8 +1420,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); > @@ -1493,30 +1529,8 @@ 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 ((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; > + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > + error = xfs_attr_node_shrink(args, state); > > out: > if (state) > -- > 2.7.4 >
On 2/23/20 6:22 AM, Amir Goldstein wrote: > On Sun, Feb 23, 2020 at 4:07 AM Allison Collins > <allison.henderson@oracle.com> 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 | 66 +++++++++++++++++++++++++++++------------------- >> 1 file changed, 40 insertions(+), 26 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 4b788f2..30a16fe 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1366,6 +1366,43 @@ 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; >> + >> + args->dac.flags |= XFS_DAC_FINISH_TRANS; > > Why did xfs_defer_finish(&args->trans); turn into the above? > > Are you testing reviewers alertness? ;-) > Please keep logic preserving patches separate from logic change patches. You're right, I missed this when I was separating it from the previous patch. Will correct. Thanks for the catch! Allison > > Thanks, > Amir. > >> + } 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 >> @@ -1383,8 +1420,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); >> @@ -1493,30 +1529,8 @@ 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 ((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; >> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> + error = xfs_attr_node_shrink(args, state); >> >> out: >> if (state) >> -- >> 2.7.4 >>
On Sat, Feb 22, 2020 at 07:06:07PM -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> Can you move this helper function up to early in the patch set? That way the code gets simpler and less tangled before adding all the new state machine gubbins? I suspect that you should do this for all the functions the state machine breaks up into gotos, too. THat way adding the state machine is really just changing how the functions that do the work are called, rather than jumping into the middle of long functions.... I know, it turns it into a longer series, but it also means that all the refactoring work (which needs to be done anyway) can be separated and merged while we are still reviewing and working on the state machine based operations, thereby reducing the size of the patchset you have to manage and keep up to date over time.... Cheers, Dave.
On 2/25/20 2:05 AM, Dave Chinner wrote: > On Sat, Feb 22, 2020 at 07:06:07PM -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> > > Can you move this helper function up to early in the patch set? > That way the code gets simpler and less tangled before adding > all the new state machine gubbins? > > I suspect that you should do this for all the functions the state > machine breaks up into gotos, too. THat way adding the state machine > is really just changing how the functions that do the work are > called, rather than jumping into the middle of long functions.... > > I know, it turns it into a longer series, but it also means that all > the refactoring work (which needs to be done anyway) can be > separated and merged while we are still reviewing and working on the > state machine based operations, thereby reducing the size of the > patchset you have to manage and keep up to date over time.... > > Cheers, > > Dave. > Sure, looking ahead it sounds like that works better for others too, so I'll move them back down. Thanks! Allison
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 4b788f2..30a16fe 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1366,6 +1366,43 @@ 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; + + args->dac.flags |= XFS_DAC_FINISH_TRANS; + } 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 @@ -1383,8 +1420,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); @@ -1493,30 +1529,8 @@ 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 ((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; + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) + error = xfs_attr_node_shrink(args, state); out: if (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 | 66 +++++++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 26 deletions(-)