Re: [PATCH v3 3/5] btrfs: remove identified alien btrfs device in open_fs_devices

[Josef Bacik <josef@toxicpanda.com>]

On 1/17/20 4:10 AM, Anand Jain wrote:
> 
> 
> On 1/16/20 11:56 PM, Josef Bacik wrote:
>> On 10/7/19 5:45 AM, Anand Jain wrote:
>>> In open_fs_devices() we identify alien device but we don't reset its
>>> the device::name. So progs device list does not show the device missing
>>> as shown in the script below.
>>>
>>> mkfs.btrfs -fq /dev/sdd && mount /dev/sdd /btrfs
>>> mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdb
>>> sleep 3 # avoid racing with udev's useless scans if needed
>>> btrfs dev add -f /dev/sdb /btrfs
>>> mount -o degraded /dev/sdc /btrfs1
>>>
>>> No missing device:
>>> btrfs fi show -m /btrfs1
>>> Label: none  uuid: 3eb7cd50-4594-458f-9d68-c243cc49954d
>>>     Total devices 2 FS bytes used 128.00KiB
>>>     devid    1 size 12.00GiB used 1.26GiB path /dev/sdc
>>>     devid    2 size 12.00GiB used 1.26GiB path /dev/sdb
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> Why not just remove the device if there's any error?  I'm not sure why these 
>> particular checks make a difference from any other error?  Thanks,
> 
>   That's interesting, but disadvantage is user has to re-run the
>   device scan if we remove the device for a non-alien device which can
>   fail temporarily in btrfs_open_one_device() function stack such as
> 
> 
>      *bdev = blkdev_get_by_path(device_path, flags, holder);
> 
>    If user land has opened the device with O_EXCL this shall
>    fail with -EBUSY. So here we shouldn't remove.
> 
> 
>       ret = set_blocksize(*bdev, BTRFS_BDEV_BLOCKSIZE);
> 
>    This can fail if the bdev does not accept the blocksize and its
>    rather a good idea to remove the device as we won't be able to
>    use this device any time. So as this is not a temporary issue,
>    here we could remove the device.
> 
> 
>          *bh = btrfs_read_dev_super(*bdev);
> 
>    This function is still an incomplete (because we don't yet handle
>    the corrupted super block #1, there is a patch in the ML but
>    in dispute, I think). Needs clarity on how a completed function
>    will look like. So here it depends on when this function completes.
> 
> 
>       bh = __bread(bdev, bytenr / BTRFS_BDEV_BLOCKSIZE, BTRFS_SUPER_INFO_SIZE);
> 
>    Read can fail momentarily for transport/disconnect/plug-out issue
>    and which can reappears and assume if there isn't systemd auto scan
>    so here we shouldn't remove.
> 

Alright that's fair, thanks,

Josef