Re: [PATCH 3/6] btrfs: cleanup extent_op handling

[Lu Fengqi <lufq.fnst@cn.fujitsu.com>]

On Wed, Nov 21, 2018 at 01:59:09PM -0500, Josef Bacik wrote:
>From: Josef Bacik <jbacik@fb.com>
>
>The cleanup_extent_op function actually would run the extent_op if it
>needed running, which made the name sort of a misnomer.  Change it to
>run_and_cleanup_extent_op, and move the actual cleanup work to
>cleanup_extent_op so it can be used by check_ref_cleanup() in order to
>unify the extent op handling.
>
>Signed-off-by: Josef Bacik <jbacik@fb.com>

One nitpick below.

Reviewed-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

>---
> fs/btrfs/extent-tree.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
>diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>index e3ed3507018d..8a776dc9cb38 100644
>--- a/fs/btrfs/extent-tree.c
>+++ b/fs/btrfs/extent-tree.c
>@@ -2424,19 +2424,33 @@ static void unselect_delayed_ref_head(struct btrfs_delayed_ref_root *delayed_ref
> 	btrfs_delayed_ref_unlock(head);
> }
> 
>-static int cleanup_extent_op(struct btrfs_trans_handle *trans,
>-			     struct btrfs_delayed_ref_head *head)
>+static struct btrfs_delayed_extent_op *
>+cleanup_extent_op(struct btrfs_trans_handle *trans,

The trans parameter seems useless.

-- 
Thanks,
Lu

>+		  struct btrfs_delayed_ref_head *head)
> {
> 	struct btrfs_delayed_extent_op *extent_op = head->extent_op;
>-	int ret;
> 
> 	if (!extent_op)
>-		return 0;
>-	head->extent_op = NULL;
>+		return NULL;
>+
> 	if (head->must_insert_reserved) {
>+		head->extent_op = NULL;
> 		btrfs_free_delayed_extent_op(extent_op);
>-		return 0;
>+		return NULL;
> 	}
>+	return extent_op;
>+}
>+
>+static int run_and_cleanup_extent_op(struct btrfs_trans_handle *trans,
>+				     struct btrfs_delayed_ref_head *head)
>+{
>+	struct btrfs_delayed_extent_op *extent_op =
>+		cleanup_extent_op(trans, head);
>+	int ret;
>+
>+	if (!extent_op)
>+		return 0;
>+	head->extent_op = NULL;
> 	spin_unlock(&head->lock);
> 	ret = run_delayed_extent_op(trans, head, extent_op);
> 	btrfs_free_delayed_extent_op(extent_op);
>@@ -2488,7 +2502,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
> 
> 	delayed_refs = &trans->transaction->delayed_refs;
> 
>-	ret = cleanup_extent_op(trans, head);
>+	ret = run_and_cleanup_extent_op(trans, head);
> 	if (ret < 0) {
> 		unselect_delayed_ref_head(delayed_refs, head);
> 		btrfs_debug(fs_info, "run_delayed_extent_op returned %d", ret);
>@@ -6977,12 +6991,8 @@ static noinline int check_ref_cleanup(struct btrfs_trans_handle *trans,
> 	if (!RB_EMPTY_ROOT(&head->ref_tree.rb_root))
> 		goto out;
> 
>-	if (head->extent_op) {
>-		if (!head->must_insert_reserved)
>-			goto out;
>-		btrfs_free_delayed_extent_op(head->extent_op);
>-		head->extent_op = NULL;
>-	}
>+	if (cleanup_extent_op(trans, head) != NULL)
>+		goto out;
> 
> 	/*
> 	 * waiting for the lock here would deadlock.  If someone else has it
>-- 
>2.14.3
>
>
>