Re: [PATCH] Btrfs: fix freeing delayed ref head while still holding its mutex
On Wed, Jan 30, 2013 at 03:16:35PM -0700, Zach Brown wrote:
On Wed, Jan 30, 2013 at 04:06:18PM -0500, Josef Bacik wrote:
I hit this error when reproducing a bug that would end in a transaction
abort. We take the delayed ref head's mutex to keep anybody from processing
it while we're destroying it, but we fail to drop the mutex before we carry
on and free the damned thing. Fix this by doing the remove logic for the
head ourselves and unlock the mutex, that way we can avoid use after free's
or hung tasks waiting on that mutex to come back so they know the delayed
ref completed. Thanks,
+ ref-in_tree = 0;
+ rb_erase(ref-rb_node, delayed_refs-root);
+ delayed_refs-num_entries--;
+ mutex_unlock(head-mutex);
+ } else {
+ ref-in_tree = 0;
+ rb_erase(ref-rb_node, delayed_refs-root);
+ delayed_refs-num_entries--;
Do you really need to duplicate the removal under the mutex? Isn't all
that protected by the delayed_refs-lock?
Isn't it enough to just add the mutex_unlock()?
Yeah I could re-arrange I guess, either way it's not pretty but I guess not
duplicating stuff is better. Thanks,
Josef
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Btrfs: fix freeing delayed ref head while still holding its mutex V2
I hit this error when reproducing a bug that would end in a transaction
abort. We take the delayed ref head's mutex to keep anybody from processing
it while we're destroying it, but we fail to drop the mutex before we carry
on and free the damned thing. Fix this by doing the remove logic for the
head ourselves and unlock the mutex, that way we can avoid use after free's
or hung tasks waiting on that mutex to come back so they know the delayed
ref completed. Thanks,
Signed-off-by: Josef Bacik jba...@fusionio.com
---
V1-V2: don't duplicate the freeing stuff, just unlock if we have a head.
fs/btrfs/disk-io.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 12ef591..42f83aa 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3615,11 +3615,11 @@ int btrfs_destroy_delayed_refs(struct btrfs_transaction
*trans,
}
while ((node = rb_first(delayed_refs-root)) != NULL) {
- ref = rb_entry(node, struct btrfs_delayed_ref_node, rb_node);
+ struct btrfs_delayed_ref_head *head = NULL;
+ ref = rb_entry(node, struct btrfs_delayed_ref_node, rb_node);
atomic_set(ref-refs, 1);
if (btrfs_delayed_ref_is_head(ref)) {
- struct btrfs_delayed_ref_head *head;
head = btrfs_delayed_node_to_head(ref);
if (!mutex_trylock(head-mutex)) {
@@ -3641,10 +3641,12 @@ int btrfs_destroy_delayed_refs(struct btrfs_transaction
*trans,
delayed_refs-num_heads_ready--;
list_del_init(head-cluster);
}
+
ref-in_tree = 0;
rb_erase(ref-rb_node, delayed_refs-root);
delayed_refs-num_entries--;
-
+ if (head)
+ mutex_unlock(head-mutex);
spin_unlock(delayed_refs-lock);
btrfs_put_delayed_ref(ref);
--
1.7.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: fix freeing delayed ref head while still holding its mutex V2
V1-V2: don't duplicate the freeing stuff, just unlock if we have a head. Nice, that's what I was picturing if we needed the freeing stuff to be covered by the mutex. Thanks for cleaning it up. - z -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Btrfs: fix freeing delayed ref head while still holding its mutex
I hit this error when reproducing a bug that would end in a transaction
abort. We take the delayed ref head's mutex to keep anybody from processing
it while we're destroying it, but we fail to drop the mutex before we carry
on and free the damned thing. Fix this by doing the remove logic for the
head ourselves and unlock the mutex, that way we can avoid use after free's
or hung tasks waiting on that mutex to come back so they know the delayed
ref completed. Thanks,
Signed-off-by: Josef Bacik jba...@fusionio.com
---
fs/btrfs/disk-io.c | 11 ---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 12a9547..51bff86 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3640,10 +3640,15 @@ int btrfs_destroy_delayed_refs(struct btrfs_transaction
*trans,
if (list_empty(head-cluster))
delayed_refs-num_heads_ready--;
list_del_init(head-cluster);
+ ref-in_tree = 0;
+ rb_erase(ref-rb_node, delayed_refs-root);
+ delayed_refs-num_entries--;
+ mutex_unlock(head-mutex);
+ } else {
+ ref-in_tree = 0;
+ rb_erase(ref-rb_node, delayed_refs-root);
+ delayed_refs-num_entries--;
}
- ref-in_tree = 0;
- rb_erase(ref-rb_node, delayed_refs-root);
- delayed_refs-num_entries--;
spin_unlock(delayed_refs-lock);
btrfs_put_delayed_ref(ref);
--
1.7.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: fix freeing delayed ref head while still holding its mutex
On Wed, Jan 30, 2013 at 04:06:18PM -0500, Josef Bacik wrote:
I hit this error when reproducing a bug that would end in a transaction
abort. We take the delayed ref head's mutex to keep anybody from processing
it while we're destroying it, but we fail to drop the mutex before we carry
on and free the damned thing. Fix this by doing the remove logic for the
head ourselves and unlock the mutex, that way we can avoid use after free's
or hung tasks waiting on that mutex to come back so they know the delayed
ref completed. Thanks,
+ ref-in_tree = 0;
+ rb_erase(ref-rb_node, delayed_refs-root);
+ delayed_refs-num_entries--;
+ mutex_unlock(head-mutex);
+ } else {
+ ref-in_tree = 0;
+ rb_erase(ref-rb_node, delayed_refs-root);
+ delayed_refs-num_entries--;
Do you really need to duplicate the removal under the mutex? Isn't all
that protected by the delayed_refs-lock?
Isn't it enough to just add the mutex_unlock()?
- z
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
