* [patch 1/2] Btrfs: double unlock bug in error handling @ 2012-04-18 6:59 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2012-04-18 6:59 UTC (permalink / raw) To: Chris Mason, Jeff Mahoney; +Cc: linux-btrfs, kernel-janitors The caller expects this function to return with the lock held and releases it immediately on error. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 2b35f8d..a0bb9dc 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -2301,6 +2301,7 @@ 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); return ret; } @@ -2331,6 +2332,7 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, if (ret) { printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n", ret); + spin_lock(&delayed_refs->lock); return ret; } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch 1/2] Btrfs: double unlock bug in error handling @ 2012-04-18 6:59 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2012-04-18 6:59 UTC (permalink / raw) To: Chris Mason, Jeff Mahoney; +Cc: linux-btrfs, kernel-janitors The caller expects this function to return with the lock held and releases it immediately on error. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 2b35f8d..a0bb9dc 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -2301,6 +2301,7 @@ 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); return ret; } @@ -2331,6 +2332,7 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, if (ret) { printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n", ret); + spin_lock(&delayed_refs->lock); return ret; } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling 2012-04-18 6:59 ` Dan Carpenter @ 2012-04-18 10:14 ` David Sterba -1 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2012-04-18 10:14 UTC (permalink / raw) To: Dan Carpenter; +Cc: Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On Wed, Apr 18, 2012 at 09:59:03AM +0300, Dan Carpenter wrote: > The caller expects this function to return with the lock held and > releases it immediately on error. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: David Sterba <dsterba@suse.cz> Good catch, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling @ 2012-04-18 10:14 ` David Sterba 0 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2012-04-18 10:14 UTC (permalink / raw) To: Dan Carpenter; +Cc: Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On Wed, Apr 18, 2012 at 09:59:03AM +0300, Dan Carpenter wrote: > The caller expects this function to return with the lock held and > releases it immediately on error. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: David Sterba <dsterba@suse.cz> Good catch, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling 2012-04-18 6:59 ` Dan Carpenter @ 2012-04-18 11:29 ` Jan Schmidt -1 siblings, 0 replies; 10+ messages in thread From: Jan Schmidt @ 2012-04-18 11:29 UTC (permalink / raw) To: Dan Carpenter; +Cc: Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On 18.04.2012 08:59, Dan Carpenter wrote: > The caller expects this function to return with the lock held and > releases it immediately on error. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 2b35f8d..a0bb9dc 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -2301,6 +2301,7 @@ 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); > return ret; > } > > @@ -2331,6 +2332,7 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, > > if (ret) { > printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n", ret); > + spin_lock(&delayed_refs->lock); > return ret; > } I think the correct way to fix this is: diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index a844204..9af261a 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -2450,7 +2450,6 @@ again: ret = run_clustered_refs(trans, root, &cluster); if (ret < 0) { - spin_unlock(&delayed_refs->lock); btrfs_abort_transaction(trans, root, ret); return ret; } -Jan ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling @ 2012-04-18 11:29 ` Jan Schmidt 0 siblings, 0 replies; 10+ messages in thread From: Jan Schmidt @ 2012-04-18 11:29 UTC (permalink / raw) To: Dan Carpenter; +Cc: Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On 18.04.2012 08:59, Dan Carpenter wrote: > The caller expects this function to return with the lock held and > releases it immediately on error. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 2b35f8d..a0bb9dc 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -2301,6 +2301,7 @@ 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); > return ret; > } > > @@ -2331,6 +2332,7 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans, > > if (ret) { > printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n", ret); > + spin_lock(&delayed_refs->lock); > return ret; > } I think the correct way to fix this is: diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index a844204..9af261a 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -2450,7 +2450,6 @@ again: ret = run_clustered_refs(trans, root, &cluster); if (ret < 0) { - spin_unlock(&delayed_refs->lock); btrfs_abort_transaction(trans, root, ret); return ret; } -Jan ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling 2012-04-18 11:29 ` Jan Schmidt @ 2012-04-18 11:58 ` David Sterba -1 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2012-04-18 11:58 UTC (permalink / raw) To: Jan Schmidt Cc: Dan Carpenter, Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On Wed, Apr 18, 2012 at 01:29:46PM +0200, Jan Schmidt wrote: > I think the correct way to fix this is: > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index a844204..9af261a 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -2450,7 +2450,6 @@ again: > > ret = run_clustered_refs(trans, root, &cluster); > if (ret < 0) { > - spin_unlock(&delayed_refs->lock); > btrfs_abort_transaction(trans, root, ret); > return ret; > } That's another way to fix it, but I'd rather see the lock/unlock balanced within a function, and in this case the extra lock does not hurt as it's a rare codepath. david ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling @ 2012-04-18 11:58 ` David Sterba 0 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2012-04-18 11:58 UTC (permalink / raw) To: Jan Schmidt Cc: Dan Carpenter, Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On Wed, Apr 18, 2012 at 01:29:46PM +0200, Jan Schmidt wrote: > I think the correct way to fix this is: > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index a844204..9af261a 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -2450,7 +2450,6 @@ again: > > ret = run_clustered_refs(trans, root, &cluster); > if (ret < 0) { > - spin_unlock(&delayed_refs->lock); > btrfs_abort_transaction(trans, root, ret); > return ret; > } That's another way to fix it, but I'd rather see the lock/unlock balanced within a function, and in this case the extra lock does not hurt as it's a rare codepath. david ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling 2012-04-18 11:29 ` Jan Schmidt @ 2012-04-18 12:06 ` Dan Carpenter -1 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2012-04-18 12:06 UTC (permalink / raw) To: Jan Schmidt; +Cc: Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On Wed, Apr 18, 2012 at 01:29:46PM +0200, Jan Schmidt wrote: > On 18.04.2012 08:59, Dan Carpenter wrote: > I think the correct way to fix this is: > Yeah. I considered both ways, but I chose the one which makes the tools happy. Which is the wrong choice, because taking a pointless lock is wrong even on the slow path and we should improve the tools. I'll resend. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/2] Btrfs: double unlock bug in error handling @ 2012-04-18 12:06 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2012-04-18 12:06 UTC (permalink / raw) To: Jan Schmidt; +Cc: Chris Mason, Jeff Mahoney, linux-btrfs, kernel-janitors On Wed, Apr 18, 2012 at 01:29:46PM +0200, Jan Schmidt wrote: > On 18.04.2012 08:59, Dan Carpenter wrote: > I think the correct way to fix this is: > Yeah. I considered both ways, but I chose the one which makes the tools happy. Which is the wrong choice, because taking a pointless lock is wrong even on the slow path and we should improve the tools. I'll resend. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-04-18 12:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-18 6:59 [patch 1/2] Btrfs: double unlock bug in error handling Dan Carpenter 2012-04-18 6:59 ` Dan Carpenter 2012-04-18 10:14 ` David Sterba 2012-04-18 10:14 ` David Sterba 2012-04-18 11:29 ` Jan Schmidt 2012-04-18 11:29 ` Jan Schmidt 2012-04-18 11:58 ` David Sterba 2012-04-18 11:58 ` David Sterba 2012-04-18 12:06 ` Dan Carpenter 2012-04-18 12:06 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.