Re: [PATCH 3/6] btrfs: cleanup extent_op handling
On Thu, Nov 22, 2018 at 12:09:34PM +0200, Nikolay Borisov wrote: > > > On 21.11.18 г. 20:59 ч., Josef Bacik wrote: > > From: Josef Bacik > > > > 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. > > The whole name extent_op is actually a misnomer since AFAIR this is some > sort of modification of the references of metadata nodes. I don't see > why it can't be made as yet another type of reference which is run for a > given node. > It would change the key for a metadata extent reference for non-skinny metadata, and it sets the FULL_BACKREF flag. Since it really only changes flags now we could probably roll that into it's own thing, but that's out of scope for this stuff. Thanks, Josef
Re: [PATCH 3/6] btrfs: cleanup extent_op handling
On Wed, Nov 21, 2018 at 01:59:09PM -0500, Josef Bacik wrote:
> From: Josef Bacik
>
> 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
> ---
> 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,
> + struct btrfs_delayed_ref_head *head)
Please keep the type and function name on one line, if the arguments
would overflow, then move it on the next line and un-indent as
necessary. I fix such things when merging, no need to resend.
> {
> 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);
I prefer to do non-trivial initializations in the statement block, it's
easy to overlook that in among the declarations. Simple variable init,
pointer dereferenes or using simple helpers is fine but cleanup_extent_op
does not seem to fit to that. It's in other patches too so I can update
that unless there are other things that would need a resend.
Re: [PATCH 3/6] btrfs: cleanup extent_op handling
On 21.11.18 г. 20:59 ч., Josef Bacik wrote:
> From: Josef Bacik
>
> 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.
The whole name extent_op is actually a misnomer since AFAIR this is some
sort of modification of the references of metadata nodes. I don't see
why it can't be made as yet another type of reference which is run for a
given node.
>
> Signed-off-by: Josef Bacik
> ---
> 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,
> + 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(>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 = >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(>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
>
Re: [PATCH 3/6] btrfs: cleanup extent_op handling
On Wed, Nov 21, 2018 at 01:59:09PM -0500, Josef Bacik wrote:
>From: Josef Bacik
>
>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
One nitpick below.
Reviewed-by: Lu Fengqi
>---
> 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(>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 = >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(>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
>
>
>
[PATCH 3/6] btrfs: cleanup extent_op handling
From: Josef Bacik
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
---
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,
+ 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(>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 = >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(>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
