Re: [PATCH 2/2] btrfs-progs: fix an error path which can lead to empty device list

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Apr 18, 2022 at 09:10:08PM +0800, Qu Wenruo wrote:
> [BUG]
> With the incoming delayed chunk item insertion feature, there is a super
> weird failure at mkfs/022:
> 
>   ====== RUN CHECK ./mkfs.btrfs -f --rootdir tmp.KnKpP5 -d dup -b 350M tests/test.img
>   ...
>   Checksum:           crc32c
>   Number of devices:  0
>   Devices:
>      ID        SIZE  PATH
> 
> Note the "Number of devices: 0" line, this means our
> fs_info->fs_devices->devices list is empty.
> 
> And since our rw device list is empty, we won't finish the mkfs with
> proper superblock magic, and cause later btrfs check to fail.
> 
> [CAUSE]
> Although the failure is only triggered by the incoming delayed chunk
> item insertion feature, the bug itself is here for a while.
> 
> In btrfs_alloc_chunk(), we move rw devices to our @private_devs list
> first, then in create_chunk(), we move it back to our rw devices list.
> 
> This dance is pretty dangerous, epsecially if btrfs_alloc_dev_extent()
> failed inside create_chunk(), and current profile have multiple stripes
> (including DUP), we will exit create_chunk() directly, without moving the
> remaining devices in @private_devs list back to @dev_list.
> 
> Furthermore, btrfs_alloc_chunk() is expected to return -ENOSPC, as we
> call btrfs_alloc_chunk() to pre-allocate chunks, and ignore the -ENOSPC
> error if it's just a pre-allocation failure.
> 
> This existing error path can lead to the empty rw list seen above.
> 
> [FIX]
> After create_chunk(), unconditionally move all devices in @private_devs
> back to rw device list.
> 
> And add extra check to make sure our rw device list is never empty after
> a chunk allocation attempt.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef