Re: [PATCH v2] btrfs: ioctl BTRFS_IOC_FS_INFO and BTRFS_IOC_DEV_INFO miss-matched with slots

[David Sterba <dsterba@suse.cz>]

On Mon, Aug 18, 2014 at 04:38:18PM +0800, Anand Jain wrote:
> ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed
> device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed
> disk when probed. So in the userland we hit a count-slot missmatch
> bug..
>             get_fs_info()
>             ::
>                     BUG_ON(ndevs >= fi_args->num_devices);
> which hits this bug when we have mounted a seed device.
> 
> So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO
> will provide total_devices instead of num_devices.

The ioctl is very unclear what the 'num_device' actually means.

> This would fix the problem partly. Partly because ealier num_devices
> included the replacing device but now total_device does not include
> the replacing device. Getting a count which includes a transient device
> is rather too in efficient/wrong indeed, because there can be a race
> condition where in the time between ioctl BTRFS_IOC_FS_INFO to
> BTRFS_IOC_DEV_INFO the replace device operation might have been
> completed. So to fix this problem its better that user land btrfs-progs
> probes replacing device (at devid 0) separately.
> 
> v2:
> Agree with Wang's comment. Its better to show seed disks under the
> sprout fs, so that user can establish mapping of seed to sprout devices.
> 
> So here I am making BTRFS_IOC_FS_INFO to return the total_devices
> which would count the seed devices (but not the replacing device).

This is even more confusing. I think we need to add another member to
the ioctl struct to reflect the number of regular devices (num_devices)
and the true total number of devices including seeding and replaced
devices. The difference should be accompanied by a flag that would say
if there's a seeding or replace in progress.

There are some backward compatibility concerns. Setting num_devices to
total_devices changes semantics of the ioctl, so I think it should stay
as is for now, but the BUG_ON can be removed and replaced by code that
reallocates the buffer or allocates a few more items in advance.