Re: [PATCH v2 1/3] btrfs: introduce the device layout aware per-profile available space

[Chris Mason <clm@meta.com>]

Qu Wenruo <wqu@suse.com> wrote:
> [BUG]
> There is a long known bug that if metadata is using RAID1 on two disks
> with unbalanced sizes, there is a very high chance to hit ENOSPC related
> transaction abort.
> 
> [CAUSE]
> The root cause is in the available space estimation code:
> 
> - Factor based calculation
>   Just use all unallocated space, divide by the profile factor
>   One obvious user is can_overcommit().
> 
> This can not handle the following example:
> 
>   devid 1 unallocated:	1GiB
>   devid 2 unallocated:	50GiB
>   metadata type:	RAID1
 
Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit 6f42a3863d937025e9670539658409ed668a7d48
Author: Qu Wenruo <wqu@suse.com>

btrfs: introduce the device layout aware per-profile available space

This commit introduces per-profile available space estimation to fix a
long-standing ENOSPC bug with RAID1 on unbalanced device sizes. It adds
infrastructure for chunk-allocator-like calculations to provide accurate
(conservative) estimations.

> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 8288d79372a56..0082c166af91f 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h

[ ... ]

> @@ -458,6 +465,15 @@ struct btrfs_fs_devices {
>  	/* Device to be used for reading in case of RAID1. */
>  	u64 read_devid;
>  #endif
> +
> +	/*
> +	 * Each value indicates the available space for that profile.
> +	 * U64_MAX means the estimation is unavailable.
> +	 *
> +	 * Protected by per_profile_lock;
> +	 */
> +	u64 per_profile_avail[BTRFS_NR_RAID_TYPES];
> +	spinlock_t per_profile_lock;
>  };

Where is this spinlock initialized? Looking at alloc_fs_devices() in
fs/btrfs/volumes.c:381, it uses kzalloc() which zeroes the memory, but
spinlocks require explicit initialization.

The follow-up commit 185737deaf9dc adds calls to
btrfs_update_per_profile_avail() which uses spin_lock() at line 5532, and
btrfs_get_per_profile_avail() also uses this spinlock. Using an uninitialized
spinlock is undefined behavior and will trigger warnings/BUGs with
CONFIG_DEBUG_SPINLOCK.

Other locks in the same struct (device_list_mutex) are properly initialized
with mutex_init() at line 389. Is there a missing spin_lock_init() call for
per_profile_lock?