Re: [PATCH 1/6] btrfs: metadata_uuid: fix failed assertion due to unsuccessful device scan (reformatted)

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

  metadata_uuid code is too confusing, a lot of if and if-nots
  it should be have been better.

  more below.

On 13/12/19 10:46 AM, Su Yue wrote:
> On 2019/12/12 10:15 PM, Nikolay Borisov wrote:
>>
>>
>> On 12.12.19 г. 13:01 ч., damenly.su@gmail.com wrote:
>>
>> <snip>
>>
>>> Acutally, there are two devices in the fs. Device 2 with
>>> FSID_CHANGING_V2 allocated a fs_devices. But, device 1 found the
>>> fs_devices but failed to be added into since fs_devices->opened (
>>
>> It's not clear why device 1 wasn't able to be added to the fs_devices
>> allocated by dev 2. Please elaborate?
>>
>>
> Sure, of course.
> 
> For example.
> 
> $cat test.sh
> ====================================================================
> img1="/tmp/test1.img"
> img2="/tmp/test2.img"
> 
> [ -f "$img1" ] || fallocate -l 300M "$img1"
> [ -f "$img2" ] || fallocate -l 300M "$img2"
> 
> mkfs.btrfs -f $img1 $img2 2>&1 >/dev/null|| exit 1
> losetup -D
> 
> dmesg -C
> rmmod btrfs
> modprobe btrfs
> 
> loop1=$(losetup --find --show "$img1")
> loop2=$(losetup --find --show "$img2")

  Can you explicitly show what devices should be scanned to make the
  device mount (below) successful. Fist you can cleanup the
  device list using

    btrfs device --forget

> mount $loop1 /mnt || exit 1
> umount /mnt
> ====================================================================
> 
> $dmesg
> ====================================================================
> [  395.205221] BTRFS: device fsid 5090db22-5e48-4767-8fb7-d037c619c1ee
> devid 1 transid 5 /dev/loop0 scanned by systemd-udevd (13620)
> [  395.210773] !!!!!!!!fs_device opened
> [  395.213875] BTRFS info (device loop0): disk space caching is enabled
> [  395.214994] BTRFS info (device loop0): has skinny extents
> [  395.215891] BTRFS info (device loop0): flagging fs with big metadata
> feature
> [  395.222639] BTRFS error (device loop0): devid 2 uuid
> adcc8454-695f-4e1d-bde8-94041b7bf761 is missing
> [  395.224147] BTRFS error (device loop0): failed to read the system
> array: -2
> [  395.246163] !!!!!!!!fs_device opened
> [  395.338219] BTRFS error (device loop0): open_ctree failed
> =====================================================================
> 
> The line "!!!!!!!!fs_device opened" is handy added by me in debug purpose.
> 
> =====================================================================
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -794,6 +794,7 @@ static noinline struct btrfs_device
> *device_list_add(const char *path,
> 
>          if (!device) {
>                  if (fs_devices->opened) {
> +                       pr_info("!!!!!!!!fs_device opened\n");
>                          mutex_unlock(&fs_devices->device_list_mutex);
>                          return ERR_PTR(-EBUSY);
>                  }
> =====================================================================
> 
> To make it more clear. The following is in metadata_uuid situation.
> Device 1 is without FSID_CHANGING_V2 but has IMCOMPAT_METADATA_UUID.
> (newer transid).
> 
> Device 2 is with FSID_CHANGING_V2 and IMCOMPAT_METADATA_UUID.(Older
> transid).

How were you able to set both BTRFS_SUPER_FLAG_CHANGING_FSID_V2
and BTRFS_FEATURE_INCOMPAT_METADATA_UUID on only devid 2 ?


> The workflow in misc-tests/034 is
> 
> loop1=$(losetup --find --show "$device2")
> loop2=$(losetup --find --show "$device1")
> 
> mount $loop1 /mnt ---> fails here
> 
> Assume the fs_devices was allocated by systemd-udevd through
> btrfs_control_ioctl() path after finish of scanning of device2.
> 
> Then:
> 

In the two threads which are in race (below), the mount thread can't be 
successful unless -o degraded is used, if it does it means the devid 1 
is already scanned and for that btrfs_device to be in the
btrfs_fs_devices list the fsid has to match (does not matter metadata_uuid).

> Thread *mounting device2*            Thread *scanning device1*
> 
> 
> btrfs_mount_root                     btrfs_control_ioctl
> 
>    mutex_lock(&uuid_mutex);
> 
>      btrfs_read_disk_super
>      btrfs_scan_one_device
>      --> there is only device2
>      in the fs_devices
> 
>      btrfs_open_devices
>        fs_devices->opened = 1
>        fs_devices->latest_bdev = device2
> 
>      mutex_unlock(&uuid_mutex);
> 
>                                        mutex_lock(&uuid_mutex);
>                                        btrfs_scan_one_device
>                                          btrfs_read_disk_super
> 
>                                          device_list_add
>                                            found fs_devices
>                                              device = btrfs_find_device
> 
>                                              rewrite fs_deivces->fsid if
>                                              scanned device1 is newer
>                                               --> Change fs_devices->fsi
>                                                    d to device1->fsid
> 
>                                            if (!device)
>                                               if(fs_devices->opened)
>                           return -EBUSY
>                                               --> the device1 adding
>                                                   aborts since
>                                                   fs_devices was opened
>                                        mutex_unlock(&uuid_mutex);
>    btrfs_fill_super
>      open_ctree
>         btrfs_read_dev_super(
>         fs_devices->latest_bdev)
>         --> the latest_bdev is device2
> 
>         assert fs_devices->fsid equals
>         device2's fsid.
>         --> fs_device->fsid was rewritten by
>             the scanning thread
> 
> The result is fs_device->fsid is from device1 but super->fsid is from
> the lastest device2.
> 

  Oops that's not good. However still not able to image various devices
  and its fsid to achieve that condition. Is it possible to write a test
  case? It would help.

Thanks, Anand

>>> the thread is doing mount device 1). But device 1's fsid was copied
>>> to fs_devices->fsid then the assertion failed.
>>
>>
>> dev 1 fsid should be copied iff its transid is newer.
>>
> 
> Even it was failed to be added into the fs_devices?
> 
>>>
>>> The solution is that only if a new device was added into a existing
>>> fs_device, then the fs_devices->fsid is allowed to be rewritten.
>>
>> fs_devices->fsid must be re-written by any device which is _newer_ w.r.t
>> to the transid.
>>
> 
> Then the assertion failed in above scenario. Just do not update the
> fs_devices->fsid, let later btrfs_read_sys_array() report the device
> missing then reject to mount.
> 
> Thanks
> 
>>>
>>> Fixes: 7a62d0f07377 ("btrfs: Handle one more split-brain scenario 
>>> during fsid change")
>>> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
>>> ---
>>>   fs/btrfs/volumes.c | 36 +++++++++++++++++++++---------------
>>>   1 file changed, 21 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index d8e5560db285..9efa4123c335 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -732,6 +732,9 @@ static noinline struct btrfs_device 
>>> *device_list_add(const char *path,
>>>           BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
>>>       bool fsid_change_in_progress = (btrfs_super_flags(disk_super) &
>>>                       BTRFS_SUPER_FLAG_CHANGING_FSID_V2);
>>> +    bool fs_devices_found = false;
>>> +
>>> +    *new_device_added = false;
>>>
>>>       if (fsid_change_in_progress) {
>>>           if (!has_metadata_uuid) {
>>> @@ -772,24 +775,11 @@ static noinline struct btrfs_device 
>>> *device_list_add(const char *path,
>>>
>>>           device = NULL;
>>>       } else {
>>> +        fs_devices_found = true;
>>> +
>>>           mutex_lock(&fs_devices->device_list_mutex);
>>>           device = btrfs_find_device(fs_devices, devid,
>>>                   disk_super->dev_item.uuid, NULL, false);
>>> -
>>> -        /*
>>> -         * If this disk has been pulled into an fs devices created by
>>> -         * a device which had the CHANGING_FSID_V2 flag then replace 
>>> the
>>> -         * metadata_uuid/fsid values of the fs_devices.
>>> -         */
>>> -        if (has_metadata_uuid && fs_devices->fsid_change &&
>>> -            found_transid > fs_devices->latest_generation) {
>>> -            memcpy(fs_devices->fsid, disk_super->fsid,
>>> -                    BTRFS_FSID_SIZE);
>>> -            memcpy(fs_devices->metadata_uuid,
>>> -                    disk_super->metadata_uuid, BTRFS_FSID_SIZE);
>>> -
>>> -            fs_devices->fsid_change = false;
>>> -        }
>>>       }
>>>
>>>       if (!device) {
>>> @@ -912,6 +902,22 @@ static noinline struct btrfs_device 
>>> *device_list_add(const char *path,
>>>           }
>>>       }
>>>
>>> +    /*
>>> +     * If the new added disk has been pulled into an fs devices 
>>> created by
>>> +     * a device which had the CHANGING_FSID_V2 flag then replace the
>>> +     * metadata_uuid/fsid values of the fs_devices.
>>> +     */
>>> +    if (*new_device_added && fs_devices_found &&
>>> +        has_metadata_uuid && fs_devices->fsid_change &&
>>> +        found_transid > fs_devices->latest_generation) {
>>> +        memcpy(fs_devices->fsid, disk_super->fsid,
>>> +               BTRFS_FSID_SIZE);
>>> +        memcpy(fs_devices->metadata_uuid,
>>> +               disk_super->metadata_uuid, BTRFS_FSID_SIZE);
>>> +
>>> +        fs_devices->fsid_change = false;
>>> +    }
>>> +
>>>       /*
>>>        * Unmount does not free the btrfs_device struct but would zero
>>>        * generation along with most of the other members. So just update
>>>
>