[PATCH] btrfs: update superblock's device bytes_used when dropping chunk

[PATCH] btrfs: update superblock's device bytes_used when dropping chunk

[Mark Harmstone]

Each superblock contains a copy of the device item for that device. In a
transaction which drops a chunk but doesn't create any new ones, we were
correctly updating the device item in the chunk tree but not copying
over the new bytes_used value to the superblock.

This can be seen by doing the following:

 # cd
 # dd if=/dev/zero of=test bs=4096 count=2621440
 # mkfs.btrfs test
 # mount test /root/temp

 # cd /root/temp
 # for i in {00..10}; do dd if=/dev/zero of=$i bs=4096 count=32768; done
 # sync
 # rm *
 # sync
 # btrfs balance start -dusage=0 .
 # sync

 # cd
 # umount /root/temp
 # btrfs check test

(For btrfs-check to detect this, you will also need my patch at
https://github.com/kdave/btrfs-progs/pull/991.)

Change btrfs_remove_dev_extents() so that it adds the devices to the
post_commit_list if they're not there already. This causes
btrfs_commit_device_sizes() to be called, which updates the bytes_used
value in the superblock.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
---
 fs/btrfs/volumes.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e59aa0b5c4f3..ee886dc08d15 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3272,6 +3272,12 @@ int btrfs_remove_dev_extents(struct btrfs_trans_handle *trans,
 					device->bytes_used - dev_extent_len);
 			atomic64_add(dev_extent_len, &fs_info->free_chunk_space);
 			btrfs_clear_space_info_full(fs_info);
+
+			if (list_empty(&device->post_commit_list)) {
+				list_add_tail(&device->post_commit_list,
+					      &trans->transaction->dev_update_list);
+			}
+
 			mutex_unlock(&fs_info->chunk_mutex);
 		}
 	}
-- 
2.49.0

Re: [PATCH] btrfs: update superblock's device bytes_used when dropping chunk

[Qu Wenruo]

在 2025/5/29 19:07, Mark Harmstone 写道:
> Each superblock contains a copy of the device item for that device. In a
> transaction which drops a chunk but doesn't create any new ones, we were
> correctly updating the device item in the chunk tree but not copying
> over the new bytes_used value to the superblock.

Oh! This explains why the image in 
fsck/020/keyed_data_ref_with_shared_leaf.img has the mismatched result.

It's not an older kernel (almost 4 years after another related kernel 
fix), but really some bug in the kernel.

> 
> This can be seen by doing the following:
> 
>   # cd
>   # dd if=/dev/zero of=test bs=4096 count=2621440
>   # mkfs.btrfs test
>   # mount test /root/temp
> 
>   # cd /root/temp
>   # for i in {00..10}; do dd if=/dev/zero of=$i bs=4096 count=32768; done
>   # sync
>   # rm *
>   # sync
>   # btrfs balance start -dusage=0 .
>   # sync
> 
>   # cd
>   # umount /root/temp
>   # btrfs check test
> 
> (For btrfs-check to detect this, you will also need my patch at
> https://github.com/kdave/btrfs-progs/pull/991.)
> 
> Change btrfs_remove_dev_extents() so that it adds the devices to the
> post_commit_list if they're not there already. This causes
> btrfs_commit_device_sizes() to be called, which updates the bytes_used
> value in the superblock.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>

Looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu> ---
>   fs/btrfs/volumes.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e59aa0b5c4f3..ee886dc08d15 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3272,6 +3272,12 @@ int btrfs_remove_dev_extents(struct btrfs_trans_handle *trans,
>   					device->bytes_used - dev_extent_len);
>   			atomic64_add(dev_extent_len, &fs_info->free_chunk_space);
>   			btrfs_clear_space_info_full(fs_info);
> +
> +			if (list_empty(&device->post_commit_list)) {
> +				list_add_tail(&device->post_commit_list,
> +					      &trans->transaction->dev_update_list);
> +			}
> +
>   			mutex_unlock(&fs_info->chunk_mutex);
>   		}
>   	}

Re: [PATCH] btrfs: update superblock's device bytes_used when dropping chunk

[David Sterba]

On Thu, May 29, 2025 at 10:37:44AM +0100, Mark Harmstone wrote:
> Each superblock contains a copy of the device item for that device. In a
> transaction which drops a chunk but doesn't create any new ones, we were
> correctly updating the device item in the chunk tree but not copying
> over the new bytes_used value to the superblock.
> 
> This can be seen by doing the following:
> 
>  # cd
>  # dd if=/dev/zero of=test bs=4096 count=2621440
>  # mkfs.btrfs test
>  # mount test /root/temp
> 
>  # cd /root/temp
>  # for i in {00..10}; do dd if=/dev/zero of=$i bs=4096 count=32768; done
>  # sync
>  # rm *
>  # sync
>  # btrfs balance start -dusage=0 .
>  # sync
> 
>  # cd
>  # umount /root/temp
>  # btrfs check test
> 
> (For btrfs-check to detect this, you will also need my patch at
> https://github.com/kdave/btrfs-progs/pull/991.)
> 
> Change btrfs_remove_dev_extents() so that it adds the devices to the

There's no btrfs_remove_dev_extents(), the code is in
btrfs_remove_chunk()

> post_commit_list if they're not there already. This causes
> btrfs_commit_device_sizes() to be called, which updates the bytes_used
> value in the superblock.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>  fs/btrfs/volumes.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e59aa0b5c4f3..ee886dc08d15 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3272,6 +3272,12 @@ int btrfs_remove_dev_extents(struct btrfs_trans_handle *trans,
>  					device->bytes_used - dev_extent_len);
>  			atomic64_add(dev_extent_len, &fs_info->free_chunk_space);
>  			btrfs_clear_space_info_full(fs_info);
> +
> +			if (list_empty(&device->post_commit_list)) {
> +				list_add_tail(&device->post_commit_list,
> +					      &trans->transaction->dev_update_list);
> +			}

There are 2 cases, one is being fixed here. I wonder if we can add some
sort of assertion or transaction commit time verification that we don't
miss that in the future. The pattern seems to be after
btrfs_device_set_bytes_used() and btrfs_clear_space_info_full(). We'd
have to have something set a bit in btrfs_device_set_bytes_used() and
check it in commit unless something unset it when adding the device to
the dev_update_list.

Otherwise ok,

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH] btrfs: update superblock's device bytes_used when dropping chunk

[Mark Harmstone]

On 2/6/25 21:16, David Sterba wrote:
> > 
> On Thu, May 29, 2025 at 10:37:44AM +0100, Mark Harmstone wrote:
>> Each superblock contains a copy of the device item for that device. In a
>> transaction which drops a chunk but doesn't create any new ones, we were
>> correctly updating the device item in the chunk tree but not copying
>> over the new bytes_used value to the superblock.
>>
>> This can be seen by doing the following:
>>
>>   # cd
>>   # dd if=/dev/zero of=test bs=4096 count=2621440
>>   # mkfs.btrfs test
>>   # mount test /root/temp
>>
>>   # cd /root/temp
>>   # for i in {00..10}; do dd if=/dev/zero of=$i bs=4096 count=32768; done
>>   # sync
>>   # rm *
>>   # sync
>>   # btrfs balance start -dusage=0 .
>>   # sync
>>
>>   # cd
>>   # umount /root/temp
>>   # btrfs check test
>>
>> (For btrfs-check to detect this, you will also need my patch at
>> https://github.com/kdave/btrfs-progs/pull/991 .)
>>
>> Change btrfs_remove_dev_extents() so that it adds the devices to the
> 
> There's no btrfs_remove_dev_extents(), the code is in
> btrfs_remove_chunk()

Sorry, that should have been remove_dev_extents(). I have it renamed in 
my tree so that it can be un-staticked.

> 
>> post_commit_list if they're not there already. This causes
>> btrfs_commit_device_sizes() to be called, which updates the bytes_used
>> value in the superblock.
>>
>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
>> ---
>>   fs/btrfs/volumes.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index e59aa0b5c4f3..ee886dc08d15 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3272,6 +3272,12 @@ int btrfs_remove_dev_extents(struct btrfs_trans_handle *trans,
>>   					device->bytes_used - dev_extent_len);
>>   			atomic64_add(dev_extent_len, &fs_info->free_chunk_space);
>>   			btrfs_clear_space_info_full(fs_info);
>> +
>> +			if (list_empty(&device->post_commit_list)) {
>> +				list_add_tail(&device->post_commit_list,
>> +					      &trans->transaction->dev_update_list);
>> +			}
> 
> There are 2 cases, one is being fixed here. I wonder if we can add some
> sort of assertion or transaction commit time verification that we don't
> miss that in the future. The pattern seems to be after
> btrfs_device_set_bytes_used() and btrfs_clear_space_info_full(). We'd
> have to have something set a bit in btrfs_device_set_bytes_used() and
> check it in commit unless something unset it when adding the device to
> the dev_update_list.
> 
> Otherwise ok,
> 
> Reviewed-by: David Sterba <dsterba@suse.com>