| Message ID | 20201218072917.16805-4-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delayed Attributes | expand |
On Fri, 18 Dec 2020 00:29:05 -0700, Allison Henderson wrote: > This patch hoists transaction handling in xfs_attr_node_remove to ... "transaction handling in xfs_attr_node_removename" > xfs_attr_node_remove_step. This will help keep transaction handling in > higher level functions instead of buried in subfunctions when we > introduce delay attributes > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 43 ++++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index e93d76a..1969b88 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1251,7 +1251,7 @@ xfs_attr_node_remove_step( > struct xfs_da_args *args, > struct xfs_da_state *state) > { > - int retval, error; > + int error; > struct xfs_inode *dp = args->dp; The declaration of "dp" variable can be removed since there are no references to it left after the removal of the following hunk. > > > @@ -1265,25 +1265,6 @@ xfs_attr_node_remove_step( > if (error) > return error; > } > - retval = xfs_attr_node_remove_cleanup(args, state); > - > - /* > - * Check to see if the tree needs to be collapsed. > - */ > - if (retval && (state->path.active > 1)) { > - error = xfs_da3_join(state); > - if (error) > - return error; > - error = xfs_defer_finish(&args->trans); > - if (error) > - return error; > - /* > - * Commit the Btree join operation and start a new trans. > - */ > - error = xfs_trans_roll_inode(&args->trans, dp); > - if (error) > - return error; > - } > > return error; > } > @@ -1299,7 +1280,7 @@ xfs_attr_node_removename( > struct xfs_da_args *args) > { > struct xfs_da_state *state = NULL; > - int error; > + int retval, error; > struct xfs_inode *dp = args->dp; > > trace_xfs_attr_node_removename(args); > @@ -1312,6 +1293,26 @@ xfs_attr_node_removename( > if (error) > goto out; > > + retval = xfs_attr_node_remove_cleanup(args, state); > + > + /* > + * Check to see if the tree needs to be collapsed. > + */ > + if (retval && (state->path.active > 1)) { > + error = xfs_da3_join(state); > + if (error) > + return error; When a non-zero value is returned by xfs_da3_join(), the code would fail to free the memory pointed to by "state". Same review comment applies to the two return statements below. > + error = xfs_defer_finish(&args->trans); > + if (error) > + return error; > + /* > + * Commit the Btree join operation and start a new trans. > + */ > + error = xfs_trans_roll_inode(&args->trans, dp); > + if (error) > + return error; > + } > + > /* > * If the result is small enough, push it all into the inode. > */ >
On 12/20/20 11:45 PM, Chandan Babu R wrote: > On Fri, 18 Dec 2020 00:29:05 -0700, Allison Henderson wrote: >> This patch hoists transaction handling in xfs_attr_node_remove to > > ... "transaction handling in xfs_attr_node_removename" > >> xfs_attr_node_remove_step. This will help keep transaction handling in >> higher level functions instead of buried in subfunctions when we >> introduce delay attributes >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 43 ++++++++++++++++++++++--------------------- >> 1 file changed, 22 insertions(+), 21 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index e93d76a..1969b88 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1251,7 +1251,7 @@ xfs_attr_node_remove_step( >> struct xfs_da_args *args, >> struct xfs_da_state *state) >> { >> - int retval, error; >> + int error; >> struct xfs_inode *dp = args->dp; > > The declaration of "dp" variable can be removed since there are no references > to it left after the removal of the following hunk. Ok, will remove > >> >> >> @@ -1265,25 +1265,6 @@ xfs_attr_node_remove_step( >> if (error) >> return error; >> } >> - retval = xfs_attr_node_remove_cleanup(args, state); >> - >> - /* >> - * Check to see if the tree needs to be collapsed. >> - */ >> - if (retval && (state->path.active > 1)) { >> - error = xfs_da3_join(state); >> - if (error) >> - return error; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - return error; >> - /* >> - * Commit the Btree join operation and start a new trans. >> - */ >> - error = xfs_trans_roll_inode(&args->trans, dp); >> - if (error) >> - return error; >> - } >> >> return error; >> } >> @@ -1299,7 +1280,7 @@ xfs_attr_node_removename( >> struct xfs_da_args *args) >> { >> struct xfs_da_state *state = NULL; >> - int error; >> + int retval, error; >> struct xfs_inode *dp = args->dp; >> >> trace_xfs_attr_node_removename(args); >> @@ -1312,6 +1293,26 @@ xfs_attr_node_removename( >> if (error) >> goto out; >> >> + retval = xfs_attr_node_remove_cleanup(args, state); >> + >> + /* >> + * Check to see if the tree needs to be collapsed. >> + */ >> + if (retval && (state->path.active > 1)) { >> + error = xfs_da3_join(state); >> + if (error) >> + return error; > > When a non-zero value is returned by xfs_da3_join(), the code would fail to > free the memory pointed to by "state". Same review comment applies to the two > return statements below. Ok, these need to be "goto out". Will fix, thx! Allison > >> + error = xfs_defer_finish(&args->trans); >> + if (error) >> + return error; >> + /* >> + * Commit the Btree join operation and start a new trans. >> + */ >> + error = xfs_trans_roll_inode(&args->trans, dp); >> + if (error) >> + return error; >> + } >> + >> /* >> * If the result is small enough, push it all into the inode. >> */ >> > >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index e93d76a..1969b88 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1251,7 +1251,7 @@ xfs_attr_node_remove_step( struct xfs_da_args *args, struct xfs_da_state *state) { - int retval, error; + int error; struct xfs_inode *dp = args->dp; @@ -1265,25 +1265,6 @@ xfs_attr_node_remove_step( if (error) return error; } - retval = xfs_attr_node_remove_cleanup(args, state); - - /* - * Check to see if the tree needs to be collapsed. - */ - if (retval && (state->path.active > 1)) { - error = xfs_da3_join(state); - if (error) - return error; - error = xfs_defer_finish(&args->trans); - if (error) - return error; - /* - * Commit the Btree join operation and start a new trans. - */ - error = xfs_trans_roll_inode(&args->trans, dp); - if (error) - return error; - } return error; } @@ -1299,7 +1280,7 @@ xfs_attr_node_removename( struct xfs_da_args *args) { struct xfs_da_state *state = NULL; - int error; + int retval, error; struct xfs_inode *dp = args->dp; trace_xfs_attr_node_removename(args); @@ -1312,6 +1293,26 @@ xfs_attr_node_removename( if (error) goto out; + retval = xfs_attr_node_remove_cleanup(args, state); + + /* + * Check to see if the tree needs to be collapsed. + */ + if (retval && (state->path.active > 1)) { + error = xfs_da3_join(state); + if (error) + return error; + error = xfs_defer_finish(&args->trans); + if (error) + return error; + /* + * Commit the Btree join operation and start a new trans. + */ + error = xfs_trans_roll_inode(&args->trans, dp); + if (error) + return error; + } + /* * If the result is small enough, push it all into the inode. */
This patch hoists transaction handling in xfs_attr_node_remove to xfs_attr_node_remove_step. This will help keep transaction handling in higher level functions instead of buried in subfunctions when we introduce delay attributes Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)