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

[Anand Jain <Anand.Jain@oracle.com>]

  There will be compatibility issue with this patch running older
  kernel, sorry I slipped some combination. As I see this is already in,
  I am sending a patch to back out this changes if it helps. Thanks.



On 09/04/14 20:02, Anand Jain wrote:
>
>
>
> On 09/04/2014 05:58 PM, David Sterba wrote:
>> 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.
>
>   Right. Thats also true in kernel. very messy. very confusing.
>   tool btrfs-devlist would help understand whats going on.
>
> ----
>   $ egrep num_device *.c | egrep "total_device"
> ioctl.c:    fi_args->num_devices = fs_devices->total_devices;
> super.c:        ret = !(fs_devices->num_devices ==
> fs_devices->total_devices);
> volumes.c:    total_devices = btrfs_super_num_devices(disk_super);
> ----
>
>   By the way about BTRFS_IOC_DEVICES_READY ioctl above its long time
>   broken with seed/replace, just waiting to get these patches integrated
>   first so to fix it later.
>
>
>>> 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.
>
>   that will be a better way. thanks.
>
>> 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,
>
>   As I have tested there is not backward compatibility issue.
>   But from semantics perspective .. agreed.
>
>> but the BUG_ON can be removed and replaced by code that
>> reallocates the buffer or allocates a few more items in advance.
>
>    We don't know how may seed devices are there for a sprout FS.
>    So thats not possible.
>
>   Will review  resubmit.
>
> Thanks for commenting.
>
> Anand
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html