Re: [PATCH 1/2] Btrfs: fix a deadlock in aborting transaction due to ENOSPC
On mon, 19 Nov 2012 15:58:55 +0800, Liu Bo wrote: > On Fri, Nov 16, 2012 at 03:19:49PM +0100, David Sterba wrote: >> On Mon, Nov 05, 2012 at 08:42:08PM +0800, Liu Bo wrote: >>> When committing a transaction, we may bail out of running delayed refs >>> due to ENOSPC, and then abort the current transaction to flip into readonly. >>> >>> But we'll hit a deadlock on ref head's lock since we forget to release >>> its lock and other cleanup stuff. >> >> Miao sent a patch for this on Tue, 08 May 2012 >> >> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg16361.html >> >> and it does more things than just the unlock, I can't tell which one of >> the patches is the right one, only letting you know. >> > > Thanks for pointing that, David. Yes, that patch will work. > > Miao, can you send it here again? > [PATCH] Btrfs: fix deadlock when the process of delayed refs fails OK, I will send it out again. Thanks Miao > > thanks, > liubo > > >> david >> -- >> 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 > -- > 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 > -- 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-progs: Fix a segmentation fault in btrfstune when is invalid.
When open_ctree(device, ...) failed, the return value should be checked. Signed-off-by: Cheng Yang --- btrfstune.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/btrfstune.c b/btrfstune.c index 47830c5..07648d5 100644 --- a/btrfstune.c +++ b/btrfstune.c @@ -107,6 +107,8 @@ int main(int argc, char *argv[]) } root = open_ctree(device, 0, 1); + if (!root) + return 1; if (seeding_flag) { ret = update_seeding_flag(root, seeding_value); -- 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
[PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails
Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call fails and jumps to the cleanup_transaction label with these mutexes still held causing deadlock when they are reacquired. Fix this problem by unlocking those mutexes at the suitable place. Reported-by: Daniel J Blueman Signed-off-by: Miao Xie --- fs/btrfs/delayed-ref.c | 8 fs/btrfs/delayed-ref.h | 6 ++ fs/btrfs/extent-tree.c | 25 ++--- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c index ae94117..364cb90 100644 --- a/fs/btrfs/delayed-ref.c +++ b/fs/btrfs/delayed-ref.c @@ -422,6 +422,14 @@ again: return 1; } +void btrfs_release_ref_cluster(struct list_head *cluster) +{ + struct list_head *pos, *q; + + list_for_each_safe(pos, q, cluster) + list_del_init(pos); +} + /* * helper function to update an extent delayed ref in the * rbtree. existing and update must both have the same diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h index ab53005..fcc5a29 100644 --- a/fs/btrfs/delayed-ref.h +++ b/fs/btrfs/delayed-ref.h @@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head * btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr); int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans, struct btrfs_delayed_ref_head *head); +static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head *head) +{ + mutex_unlock(&head->mutex); +} + int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans, struct list_head *cluster, u64 search_start); +void btrfs_release_ref_cluster(struct list_head *cluster); int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info, struct btrfs_delayed_ref_root *delayed_refs, diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 3d3e2c1..43a58cb 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct btrfs_trans_handle *trans, node->num_bytes); } } - mutex_unlock(&head->mutex); return ret; } @@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, * process of being added. Don't run this ref yet. */ list_del_init(&locked_ref->cluster); - mutex_unlock(&locked_ref->mutex); + btrfs_delayed_ref_unlock(locked_ref); locked_ref = NULL; delayed_refs->num_heads_ready++; spin_unlock(&delayed_refs->lock); @@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, if (ret) { printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned %d\n", ret); spin_lock(&delayed_refs->lock); + btrfs_delayed_ref_unlock(locked_ref); return ret; } goto next; } - - list_del_init(&locked_ref->cluster); - locked_ref = NULL; } ref->in_tree = 0; @@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, ret = run_one_delayed_ref(trans, root, ref, extent_op, must_insert_reserved); - - btrfs_put_delayed_ref(ref); kfree(extent_op); count++; + /* +* If this node is a head, we will pick the next head to deal +* with. If there is something wrong when we process the +* delayed ref, we will end our operation. So in these two +* cases, we have to unlock the head and drop it from the +* cluster list before we release it though the code is ugly. +*/ + if (btrfs_delayed_ref_is_head(ref) || ret) { + list_del_init(&locked_ref->cluster); + btrfs_delayed_ref_unlock(locked_ref); + locked_ref = NULL; + } + + btrfs_put_delayed_ref(ref); + if (ret) { printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n", ret); spin_lock(&delayed_refs->lock); @@ -2510,6 +2520,7 @@ again: ret = run_clustered_refs(trans, root, &cluster); if (ret < 0) { + btrfs_release_ref_cluster(&cluster);
Re: [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails
On Mon, Nov 19, 2012 at 05:20:06PM +0800, Miao Xie wrote: > Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call > fails and jumps to the cleanup_transaction label with these mutexes still held > causing deadlock when they are reacquired. > > Fix this problem by unlocking those mutexes at the suitable place. > > Reported-by: Daniel J Blueman > Signed-off-by: Miao Xie > --- > fs/btrfs/delayed-ref.c | 8 > fs/btrfs/delayed-ref.h | 6 ++ > fs/btrfs/extent-tree.c | 25 ++--- > 3 files changed, 32 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c > index ae94117..364cb90 100644 > --- a/fs/btrfs/delayed-ref.c > +++ b/fs/btrfs/delayed-ref.c > @@ -422,6 +422,14 @@ again: > return 1; > } > > +void btrfs_release_ref_cluster(struct list_head *cluster) > +{ > + struct list_head *pos, *q; > + > + list_for_each_safe(pos, q, cluster) > + list_del_init(pos); > +} > + > /* > * helper function to update an extent delayed ref in the > * rbtree. existing and update must both have the same > diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h > index ab53005..fcc5a29 100644 > --- a/fs/btrfs/delayed-ref.h > +++ b/fs/btrfs/delayed-ref.h > @@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head * > btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr); > int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans, > struct btrfs_delayed_ref_head *head); > +static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head > *head) > +{ > + mutex_unlock(&head->mutex); > +} > + > int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans, > struct list_head *cluster, u64 search_start); > +void btrfs_release_ref_cluster(struct list_head *cluster); > > int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info, > struct btrfs_delayed_ref_root *delayed_refs, > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 3d3e2c1..43a58cb 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct > btrfs_trans_handle *trans, > node->num_bytes); > } > } > - mutex_unlock(&head->mutex); I think code can be clean here if we keep this mutex_unlock(), see below, please. > return ret; > } > > @@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct > btrfs_trans_handle *trans, >* process of being added. Don't run this ref yet. >*/ > list_del_init(&locked_ref->cluster); > - mutex_unlock(&locked_ref->mutex); > + btrfs_delayed_ref_unlock(locked_ref); > locked_ref = NULL; > delayed_refs->num_heads_ready++; > spin_unlock(&delayed_refs->lock); > @@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct > btrfs_trans_handle *trans, > if (ret) { > printk(KERN_DEBUG "btrfs: > run_delayed_extent_op returned %d\n", ret); > spin_lock(&delayed_refs->lock); > + btrfs_delayed_ref_unlock(locked_ref); > return ret; > } > > goto next; > } > - > - list_del_init(&locked_ref->cluster); > - locked_ref = NULL; > } > > ref->in_tree = 0; > @@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct > btrfs_trans_handle *trans, > > ret = run_one_delayed_ref(trans, root, ref, extent_op, > must_insert_reserved); > - > - btrfs_put_delayed_ref(ref); > kfree(extent_op); > count++; > > + /* > + * If this node is a head, we will pick the next head to deal > + * with. If there is something wrong when we process the > + * delayed ref, we will end our operation. So in these two > + * cases, we have to unlock the head and drop it from the > + * cluster list before we release it though the code is ugly. > + */ > + if (btrfs_delayed_ref_is_head(ref) || ret) { > + list_del_init(&locked_ref->cluster); > + btrfs_delayed_ref_unlock(locked_ref); > + locked_ref = NULL; > + } > + In case that we don't remove mutex_unlock above, if ret is non-zero, either A)locked_ref is not NULL, or B)locked_ref is NULL, and it has done list
Re: btrfs crash - Null dereference - 3.7.0-rc5-00068-gc5e35d6
On Mon, Nov 19, 2012 at 12:55:40AM -0200, Gustavo Padovan wrote: > Hi, > > my system suddenly crashed and gave me this dump: > > http://imgur.com/oO6S0 > > I checked and there is not btrfs commit in linus' tree since I compiled this > kernel. > Hi Gustavo, It's weird that NULL pointer oops happens here. Since you own the kernel, you may also have debuginfo left, can you please run 'gdb fs/btrfs/btrfs.ko' and 'list *block_rsv_release_bytes+0x21' to check which one is NULL pointer? thanks, liubo > Gustavo > -- > 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 -- 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: btrfs crash - Null dereference - 3.7.0-rc5-00068-gc5e35d6
Hi Liu, * Liu Bo [2012-11-19 18:32:23 +0800]: > On Mon, Nov 19, 2012 at 12:55:40AM -0200, Gustavo Padovan wrote: > > Hi, > > > > my system suddenly crashed and gave me this dump: > > > > http://imgur.com/oO6S0 > > > > I checked and there is not btrfs commit in linus' tree since I compiled this > > kernel. > > > > Hi Gustavo, > > It's weird that NULL pointer oops happens here. > > Since you own the kernel, you may also have debuginfo left, > > can you please run > 'gdb fs/btrfs/btrfs.ko' and 'list *block_rsv_release_bytes+0x21' to > check which one is NULL pointer? (gdb) list *block_rsv_release_bytes+0x21 0x811a83c1 is in block_rsv_release_bytes (fs/btrfs/extent-tree.c:4065). 4060 4061static void block_rsv_release_bytes(struct btrfs_fs_info *fs_info, 4062struct btrfs_block_rsv *block_rsv, 4063struct btrfs_block_rsv *dest, u64 num_bytes) 4064{ 4065struct btrfs_space_info *space_info = block_rsv->space_info; 4066 4067spin_lock(&block_rsv->lock); 4068if (num_bytes == (u64)-1) 4069num_bytes = block_rsv->size; (gdb) Seems block_rsv is NULL here and looking to btrfs_block_rsv_release() it can only be NULL at this point if global_rsv->full is true otherwise the crash would happen there. I didn't go any further than this. Gustavo -- 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: btrfs crash - Null dereference - 3.7.0-rc5-00068-gc5e35d6
On Mon, Nov 19, 2012 at 11:07:52AM -0200, Gustavo Padovan wrote: > > can you please run > > 'gdb fs/btrfs/btrfs.ko' and 'list *block_rsv_release_bytes+0x21' to > > check which one is NULL pointer? > > > (gdb) list *block_rsv_release_bytes+0x21 > 0x811a83c1 is in block_rsv_release_bytes > (fs/btrfs/extent-tree.c:4065). > 4060 > 4061 static void block_rsv_release_bytes(struct btrfs_fs_info *fs_info, > 4062 struct btrfs_block_rsv *block_rsv, > 4063 struct btrfs_block_rsv *dest, u64 > num_bytes) > 4064 { > 4065 struct btrfs_space_info *space_info = block_rsv->space_info; > 4066 > 4067 spin_lock(&block_rsv->lock); > 4068 if (num_bytes == (u64)-1) > 4069 num_bytes = block_rsv->size; > (gdb) > > Seems block_rsv is NULL here and looking to btrfs_block_rsv_release() it can > only be NULL at this point if global_rsv->full is true otherwise the crash > would happen there. I didn't go any further than this. > > Gustavo Can you check if the following commit is in your tree? commit 321f0e70225abc792d74902a2bc4a60164265fd4 Author: Miao Xie Btrfs: fix wrong orphan count of the fs/file tree diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 878116d..a6824bd 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -2228,7 +2228,7 @@ int btrfs_orphan_add(struct btrfs_trans_handle *trans, struct inode *inode) insert = 1; #endif insert = 1; - atomic_dec(&root->orphan_inodes); + atomic_inc(&root->orphan_inodes); } if (!test_and_set_bit(BTRFS_INODE_ORPHAN_META_RESERVED, thanks, liubo -- 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: btrfs crash - Null dereference - 3.7.0-rc5-00068-gc5e35d6
* Liu Bo [2012-11-19 23:27:53 +0800]: > On Mon, Nov 19, 2012 at 11:07:52AM -0200, Gustavo Padovan wrote: > > > can you please run > > > 'gdb fs/btrfs/btrfs.ko' and 'list *block_rsv_release_bytes+0x21' to > > > check which one is NULL pointer? > > > > > > (gdb) list *block_rsv_release_bytes+0x21 > > 0x811a83c1 is in block_rsv_release_bytes > > (fs/btrfs/extent-tree.c:4065). > > 4060 > > 4061static void block_rsv_release_bytes(struct btrfs_fs_info > > *fs_info, > > 4062struct btrfs_block_rsv > > *block_rsv, > > 4063struct btrfs_block_rsv > > *dest, u64 > > num_bytes) > > 4064{ > > 4065struct btrfs_space_info *space_info = > > block_rsv->space_info; > > 4066 > > 4067spin_lock(&block_rsv->lock); > > 4068if (num_bytes == (u64)-1) > > 4069num_bytes = block_rsv->size; > > (gdb) > > > > Seems block_rsv is NULL here and looking to btrfs_block_rsv_release() it can > > only be NULL at this point if global_rsv->full is true otherwise the crash > > would happen there. I didn't go any further than this. > > > > Gustavo > > Can you check if the following commit is in your tree? > > commit 321f0e70225abc792d74902a2bc4a60164265fd4 > Author: Miao Xie > > Btrfs: fix wrong orphan count of the fs/file tree > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 878116d..a6824bd 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -2228,7 +2228,7 @@ int btrfs_orphan_add(struct btrfs_trans_handle *trans, > struct inode *inode) > insert = 1; > #endif > insert = 1; > - atomic_dec(&root->orphan_inodes); > + atomic_inc(&root->orphan_inodes); > } > > if (!test_and_set_bit(BTRFS_INODE_ORPHAN_META_RESERVED, Yes, it is. I'm using linus tree from last week, head at c5e35d6. Gustavo -- 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 V2] Btrfs: fix deadlock when the process of delayed refs fails
Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call fails and jumps to the cleanup_transaction label with these mutexes still held causing deadlock when they are reacquired. Fix this problem by unlocking those mutexes at the suitable place. Reported-by: Daniel J Blueman Signed-off-by: Miao Xie --- Changelog from v1 -> v2: - split the code of the run_one_delayed_ref()'s error path from the normal path --- fs/btrfs/delayed-ref.c | 8 fs/btrfs/delayed-ref.h | 6 ++ fs/btrfs/extent-tree.c | 36 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c index ae94117..364cb90 100644 --- a/fs/btrfs/delayed-ref.c +++ b/fs/btrfs/delayed-ref.c @@ -422,6 +422,14 @@ again: return 1; } +void btrfs_release_ref_cluster(struct list_head *cluster) +{ + struct list_head *pos, *q; + + list_for_each_safe(pos, q, cluster) + list_del_init(pos); +} + /* * helper function to update an extent delayed ref in the * rbtree. existing and update must both have the same diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h index ab53005..fcc5a29 100644 --- a/fs/btrfs/delayed-ref.h +++ b/fs/btrfs/delayed-ref.h @@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head * btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr); int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans, struct btrfs_delayed_ref_head *head); +static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head *head) +{ + mutex_unlock(&head->mutex); +} + int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans, struct list_head *cluster, u64 search_start); +void btrfs_release_ref_cluster(struct list_head *cluster); int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info, struct btrfs_delayed_ref_root *delayed_refs, diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 3d3e2c1..93faf5e 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct btrfs_trans_handle *trans, node->num_bytes); } } - mutex_unlock(&head->mutex); return ret; } @@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, * process of being added. Don't run this ref yet. */ list_del_init(&locked_ref->cluster); - mutex_unlock(&locked_ref->mutex); + btrfs_delayed_ref_unlock(locked_ref); locked_ref = NULL; delayed_refs->num_heads_ready++; spin_unlock(&delayed_refs->lock); @@ -2314,16 +2313,17 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, kfree(extent_op); if (ret) { - printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned %d\n", ret); + printk(KERN_DEBUG + "btrfs: run_delayed_extent_op " + "returned %d\n", + ret); spin_lock(&delayed_refs->lock); + btrfs_delayed_ref_unlock(locked_ref); return ret; } goto next; } - - list_del_init(&locked_ref->cluster); - locked_ref = NULL; } ref->in_tree = 0; @@ -2350,17 +2350,28 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, ret = run_one_delayed_ref(trans, root, ref, extent_op, must_insert_reserved); - - btrfs_put_delayed_ref(ref); kfree(extent_op); - count++; - if (ret) { - printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n", ret); + btrfs_delayed_ref_unlock(locked_ref); + btrfs_put_delayed_ref(ref); + printk(KERN_DEBUG + "btrfs: run_one_delayed_ref returned %d\n", ret); spin_lock(&delayed_refs->lock); return ret; } - + /* +* If this node is a head, that means all the refs in this head +* have been dealt with, and we will pick the next head to deal +
Re: [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails
On mon, 19 Nov 2012 18:18:48 +0800, Liu Bo wrote: >> @@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct >> btrfs_trans_handle *trans, >> if (ret) { >> printk(KERN_DEBUG "btrfs: >> run_delayed_extent_op returned %d\n", ret); >> spin_lock(&delayed_refs->lock); >> +btrfs_delayed_ref_unlock(locked_ref); >> return ret; >> } >> >> goto next; >> } >> - >> -list_del_init(&locked_ref->cluster); >> -locked_ref = NULL; >> } >> >> ref->in_tree = 0; >> @@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct >> btrfs_trans_handle *trans, >> >> ret = run_one_delayed_ref(trans, root, ref, extent_op, >>must_insert_reserved); >> - >> -btrfs_put_delayed_ref(ref); >> kfree(extent_op); >> count++; >> >> +/* >> + * If this node is a head, we will pick the next head to deal >> + * with. If there is something wrong when we process the >> + * delayed ref, we will end our operation. So in these two >> + * cases, we have to unlock the head and drop it from the >> + * cluster list before we release it though the code is ugly. >> + */ >> +if (btrfs_delayed_ref_is_head(ref) || ret) { >> +list_del_init(&locked_ref->cluster); >> +btrfs_delayed_ref_unlock(locked_ref); >> +locked_ref = NULL; >> +} >> + > > In case that we don't remove mutex_unlock above, > > if ret is non-zero, either > A)locked_ref is not NULL, or > B)locked_ref is NULL, and it has done list_del_init above and > also done mutex_unlock in run_one_delayed_ref(). > > So in the case A), it is ok to do list_del_init() and mutex_unlock(), > while in the case B), we need to do nothing. > > Then the code can be clean as we wish, > if (ret) { > if (locked_ref) { > list_del_init(); > mutex_unlock(); > } > ... > } I think it is not good style that locking/unlocking a lock in the different functions, because it is error prone and the readability of the code is very bad, so I remove mutex_unlock() in run_one_delayed_ref(). Maybe I should not mix the code of the error path into the normal one, I will send out a new patch to make the code cleaner and more readable. Thanks Miao -- 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 V2] Btrfs: fix deadlock when the process of delayed refs fails
On Tue, Nov 20, 2012 at 10:52:36AM +0800, Miao Xie wrote: > Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call > fails and jumps to the cleanup_transaction label with these mutexes still held > causing deadlock when they are reacquired. > > Fix this problem by unlocking those mutexes at the suitable place. > > Reported-by: Daniel J Blueman > Signed-off-by: Miao Xie > --- > Changelog from v1 -> v2: > - split the code of the run_one_delayed_ref()'s error path from the normal > path > --- > fs/btrfs/delayed-ref.c | 8 > fs/btrfs/delayed-ref.h | 6 ++ > fs/btrfs/extent-tree.c | 36 > 3 files changed, 38 insertions(+), 12 deletions(-) > > diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c > index ae94117..364cb90 100644 > --- a/fs/btrfs/delayed-ref.c > +++ b/fs/btrfs/delayed-ref.c > @@ -422,6 +422,14 @@ again: > return 1; > } > > +void btrfs_release_ref_cluster(struct list_head *cluster) > +{ > + struct list_head *pos, *q; > + > + list_for_each_safe(pos, q, cluster) > + list_del_init(pos); > +} > + > /* > * helper function to update an extent delayed ref in the > * rbtree. existing and update must both have the same > diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h > index ab53005..fcc5a29 100644 > --- a/fs/btrfs/delayed-ref.h > +++ b/fs/btrfs/delayed-ref.h > @@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head * > btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr); > int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans, > struct btrfs_delayed_ref_head *head); > +static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head > *head) > +{ > + mutex_unlock(&head->mutex); > +} > + > int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans, > struct list_head *cluster, u64 search_start); > +void btrfs_release_ref_cluster(struct list_head *cluster); > > int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info, > struct btrfs_delayed_ref_root *delayed_refs, > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 3d3e2c1..93faf5e 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct > btrfs_trans_handle *trans, > node->num_bytes); > } > } > - mutex_unlock(&head->mutex); > return ret; > } > > @@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct > btrfs_trans_handle *trans, >* process of being added. Don't run this ref yet. >*/ > list_del_init(&locked_ref->cluster); > - mutex_unlock(&locked_ref->mutex); > + btrfs_delayed_ref_unlock(locked_ref); > locked_ref = NULL; > delayed_refs->num_heads_ready++; > spin_unlock(&delayed_refs->lock); > @@ -2314,16 +2313,17 @@ static noinline int run_clustered_refs(struct > btrfs_trans_handle *trans, > kfree(extent_op); > > if (ret) { > - printk(KERN_DEBUG "btrfs: > run_delayed_extent_op returned %d\n", ret); > + printk(KERN_DEBUG > +"btrfs: run_delayed_extent_op " > +"returned %d\n", > +ret); > spin_lock(&delayed_refs->lock); > + btrfs_delayed_ref_unlock(locked_ref); > return ret; > } > > goto next; > } > - > - list_del_init(&locked_ref->cluster); > - locked_ref = NULL; > } > > ref->in_tree = 0; > @@ -2350,17 +2350,28 @@ static noinline int run_clustered_refs(struct > btrfs_trans_handle *trans, > > ret = run_one_delayed_ref(trans, root, ref, extent_op, > must_insert_reserved); > - > - btrfs_put_delayed_ref(ref); > kfree(extent_op); > - count++; > - > if (ret) { > - printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned > %d\n", ret); > + btrfs_delayed_ref_unlock(locked_ref); > + btrfs_put_delayed_ref(ref); > + printk(KERN_DEBUG > +"btrfs: run_one_delayed_ref returned %d\n", ret); > spin_lock(&delayed_refs->lock); > return ret; >
[PATCH] btrfs-progs: print the right mount checking info in restore.c
The mount status is checked against the arg (argv[optind]), not the . Correct the info printed on mounted. Signed-off-by: Wang Sheng-Hui --- restore.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/restore.c b/restore.c index b9e5381..a7580f8 100644 --- a/restore.c +++ b/restore.c @@ -827,7 +827,7 @@ int main(int argc, char **argv) strerror(ret)); return ret; } else if (ret) { - fprintf(stderr, "%s is currently mounted. Aborting.\n", argv[optind + 1]); + fprintf(stderr, "%s is currently mounted. Aborting.\n", argv[optind]); return -EBUSY; } -- 1.7.5.4 -- 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