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

[David Sterba <dsterba@suse.cz>]

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>