All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()
@ 2019-02-11 18:32 ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2019-02-11 18:32 UTC (permalink / raw)
  To: Chris Mason, Anand Jain
  Cc: Josef Bacik, David Sterba, linux-btrfs, kernel-janitors

We should drop the lock on this error path.  This is from static
analysis and I don't know if it's possible to hit this error path in
real life.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/btrfs/dev-replace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 13863354ff9d..ee193c5222b2 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -862,6 +862,7 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 			btrfs_destroy_dev_replace_tgtdev(tgt_device);
 		break;
 	default:
+		up_write(&dev_replace->rwsem);
 		result = -EINVAL;
 	}
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()
@ 2019-02-11 18:32 ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2019-02-11 18:32 UTC (permalink / raw)
  To: Chris Mason, Anand Jain
  Cc: Josef Bacik, David Sterba, linux-btrfs, kernel-janitors

We should drop the lock on this error path.  This is from static
analysis and I don't know if it's possible to hit this error path in
real life.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/btrfs/dev-replace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 13863354ff9d..ee193c5222b2 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -862,6 +862,7 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 			btrfs_destroy_dev_replace_tgtdev(tgt_device);
 		break;
 	default:
+		up_write(&dev_replace->rwsem);
 		result = -EINVAL;
 	}
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()
  2019-02-11 18:32 ` Dan Carpenter
@ 2019-02-19 19:04   ` David Sterba
  -1 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-02-19 19:04 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Chris Mason, Anand Jain, Josef Bacik, David Sterba, linux-btrfs,
	kernel-janitors

On Mon, Feb 11, 2019 at 09:32:10PM +0300, Dan Carpenter wrote:
> We should drop the lock on this error path.  This is from static
> analysis and I don't know if it's possible to hit this error path in
> real life.

Yes the lock needs to be released, it's there to protect access to the
dev_replace members and is not supposed to be left locked. The value of
state that's being switched would need to be artifically changed to an
invalid value so the default: branch is taken.

It's been introduced by d189dd70e25561817325 in 5.0-rc1 so it counts as
a regression but I don't think it's urgent enough to be sent to a late
rc. It'll go through the stable tree channel. Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()
@ 2019-02-19 19:04   ` David Sterba
  0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-02-19 19:04 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Chris Mason, Anand Jain, Josef Bacik, David Sterba, linux-btrfs,
	kernel-janitors

On Mon, Feb 11, 2019 at 09:32:10PM +0300, Dan Carpenter wrote:
> We should drop the lock on this error path.  This is from static
> analysis and I don't know if it's possible to hit this error path in
> real life.

Yes the lock needs to be released, it's there to protect access to the
dev_replace members and is not supposed to be left locked. The value of
state that's being switched would need to be artifically changed to an
invalid value so the default: branch is taken.

It's been introduced by d189dd70e25561817325 in 5.0-rc1 so it counts as
a regression but I don't think it's urgent enough to be sent to a late
rc. It'll go through the stable tree channel. Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()
  2019-02-19 19:04   ` David Sterba
@ 2019-02-20  1:20     ` Anand Jain
  -1 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-20  1:20 UTC (permalink / raw)
  To: dsterba, Dan Carpenter
  Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, kernel-janitors



On 2/20/19 3:04 AM, David Sterba wrote:
> On Mon, Feb 11, 2019 at 09:32:10PM +0300, Dan Carpenter wrote:
>> We should drop the lock on this error path.  This is from static
>> analysis and I don't know if it's possible to hit this error path in
>> real life.
> 
> Yes the lock needs to be released, it's there to protect access to the
> dev_replace members and is not supposed to be left locked. The value of
> state that's being switched would need to be artifically changed to an
> invalid value so the default: branch is taken.
> 
> It's been introduced by d189dd70e25561817325 in 5.0-rc1 so it counts as
> a regression but I don't think it's urgent enough to be sent to a late
> rc. It'll go through the stable tree channel. Thanks.
> 

oops I missed this email. Thanks Dan and David.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()
@ 2019-02-20  1:20     ` Anand Jain
  0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-20  1:20 UTC (permalink / raw)
  To: dsterba, Dan Carpenter
  Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, kernel-janitors



On 2/20/19 3:04 AM, David Sterba wrote:
> On Mon, Feb 11, 2019 at 09:32:10PM +0300, Dan Carpenter wrote:
>> We should drop the lock on this error path.  This is from static
>> analysis and I don't know if it's possible to hit this error path in
>> real life.
> 
> Yes the lock needs to be released, it's there to protect access to the
> dev_replace members and is not supposed to be left locked. The value of
> state that's being switched would need to be artifically changed to an
> invalid value so the default: branch is taken.
> 
> It's been introduced by d189dd70e25561817325 in 5.0-rc1 so it counts as
> a regression but I don't think it's urgent enough to be sent to a late
> rc. It'll go through the stable tree channel. Thanks.
> 

oops I missed this email. Thanks Dan and David.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-02-20  1:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11 18:32 [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel() Dan Carpenter
2019-02-11 18:32 ` Dan Carpenter
2019-02-19 19:04 ` David Sterba
2019-02-19 19:04   ` David Sterba
2019-02-20  1:20   ` Anand Jain
2019-02-20  1:20     ` Anand Jain

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.