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@xxxxxxxxxx>
> ---
> 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.
> */
>
--
chandan
