[PATCH 0/2] minor cleanups for device close and free functions

[PATCH 0/2] minor cleanups for device close and free functions

[Anand Jain]

This patch series includes fixes for minor issues identified in the
btrfs_close_one_device() and btrfs_free_device() functions.

Anand Jain (2):
  btrfs: warn for any missed cleanup at btrfs_close_one_device
  btrfs: remove redundant release of alloc_state

 fs/btrfs/volumes.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.39.2

[PATCH 1/2] btrfs: warn for any missed cleanup at btrfs_close_one_device

[Anand Jain]

During my recent search for the root cause of a reported bug, I realized
that it's a good idea to issue a warning for missed cleanup instead of
using debug-only assertions. Since most installations run with debug off,
missed cleanups and premature calls to close could go unnoticed. However,
these issues are serious enough to warrant reporting and fixing.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index eead4a1f53b7..0e3677650a78 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1150,10 +1150,10 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	device->last_flush_error = 0;
 
 	/* Verify the device is back in a pristine state  */
-	ASSERT(!test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
-	ASSERT(!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state));
-	ASSERT(list_empty(&device->dev_alloc_list));
-	ASSERT(list_empty(&device->post_commit_list));
+	WARN_ON(test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
+	WARN_ON(test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state));
+	WARN_ON(!list_empty(&device->dev_alloc_list));
+	WARN_ON(!list_empty(&device->post_commit_list));
 }
 
 static void close_fs_devices(struct btrfs_fs_devices *fs_devices)
-- 
2.39.2

[PATCH 2/2] btrfs: remove redundant release of alloc_state

[Anand Jain]

Commit 321f69f86a0f ("btrfs: reset device back to allocation state when
removing") included adding extent_io_tree_release(&device->alloc_state)
to btrfs_close_one_device(), which had already been called in
btrfs_free_device().

The alloc_state tree (IO_TREE_DEVICE_ALLOC_STATE), is created in
btrfs_alloc_device() and released in btrfs_close_one_device(). Therefore,
the additional call to extent_io_tree_release(&device->alloc_state) in
btrfs_free_device() is unnecessary and can be removed.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0e3677650a78..c201d72f798e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -395,7 +395,6 @@ void btrfs_free_device(struct btrfs_device *device)
 {
 	WARN_ON(!list_empty(&device->post_commit_list));
 	rcu_string_free(device->name);
-	extent_io_tree_release(&device->alloc_state);
 	btrfs_destroy_dev_zone_info(device);
 	kfree(device);
 }
-- 
2.39.2

Re: [PATCH 1/2] btrfs: warn for any missed cleanup at btrfs_close_one_device

[David Sterba]

On Tue, Apr 04, 2023 at 10:55:11PM +0800, Anand Jain wrote:
> During my recent search for the root cause of a reported bug, I realized
> that it's a good idea to issue a warning for missed cleanup instead of
> using debug-only assertions. Since most installations run with debug off,
> missed cleanups and premature calls to close could go unnoticed. However,
> these issues are serious enough to warrant reporting and fixing.

There are other WARN_ONs checking for device state so it should be ok to
add some more.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index eead4a1f53b7..0e3677650a78 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1150,10 +1150,10 @@ static void btrfs_close_one_device(struct btrfs_device *device)
>  	device->last_flush_error = 0;
>  
>  	/* Verify the device is back in a pristine state  */
> -	ASSERT(!test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
> -	ASSERT(!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state));
> -	ASSERT(list_empty(&device->dev_alloc_list));
> -	ASSERT(list_empty(&device->post_commit_list));
> +	WARN_ON(test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
> +	WARN_ON(test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state));
> +	WARN_ON(!list_empty(&device->dev_alloc_list));
> +	WARN_ON(!list_empty(&device->post_commit_list));

I'm thinking if we can reset some of the values to a safe state too, not
keeping it until the next time the device is opened, because not
everything is reset at device open time.

Re: [PATCH 0/2] minor cleanups for device close and free functions

[David Sterba]

On Tue, Apr 04, 2023 at 10:55:10PM +0800, Anand Jain wrote:
> This patch series includes fixes for minor issues identified in the
> btrfs_close_one_device() and btrfs_free_device() functions.
> 
> Anand Jain (2):
>   btrfs: warn for any missed cleanup at btrfs_close_one_device
>   btrfs: remove redundant release of alloc_state

Added to misc-next, thanks.

Re: [PATCH 1/2] btrfs: warn for any missed cleanup at btrfs_close_one_device

[Anand Jain]

On 4/6/23 01:37, David Sterba wrote:
> On Tue, Apr 04, 2023 at 10:55:11PM +0800, Anand Jain wrote:
>> During my recent search for the root cause of a reported bug, I realized
>> that it's a good idea to issue a warning for missed cleanup instead of
>> using debug-only assertions. Since most installations run with debug off,
>> missed cleanups and premature calls to close could go unnoticed. However,
>> these issues are serious enough to warrant reporting and fixing.
> 
> There are other WARN_ONs checking for device state so it should be ok to
> add some more.
> 
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index eead4a1f53b7..0e3677650a78 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1150,10 +1150,10 @@ static void btrfs_close_one_device(struct btrfs_device *device)
>>   	device->last_flush_error = 0;
>>   
>>   	/* Verify the device is back in a pristine state  */
>> -	ASSERT(!test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
>> -	ASSERT(!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state));
>> -	ASSERT(list_empty(&device->dev_alloc_list));
>> -	ASSERT(list_empty(&device->post_commit_list));
>> +	WARN_ON(test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state));
>> +	WARN_ON(test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state));
>> +	WARN_ON(!list_empty(&device->dev_alloc_list));
>> +	WARN_ON(!list_empty(&device->post_commit_list));
> 
> I'm thinking if we can reset some of the values to a safe state too, not
> keeping it until the next time the device is opened, because not
> everything is reset at device open time.

Good idea. We need some cleanup around these code lines. I will send 
patches.

Thanks, Anand