Re: [PATCH 1/2] btrfs-progs: fi show: Print missing device for a mounted file system

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

On 07/09/2021 00:47, g.btrfs@cobb.uk.net wrote:
> 
> On 02/09/2021 13:17, Qu Wenruo wrote:
>>
>>
>> On 2021/9/2 下午6:59, Nikolay Borisov wrote:
>>>
>>>
>>> On 2.09.21 г. 13:46, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2021/9/2 下午6:41, Nikolay Borisov wrote:
>>>>>
>>>>>
>>>>> On 2.09.21 г. 13:27, Qu Wenruo wrote:
>>>>>>
>>>>>>
>>>>>> On 2021/9/2 下午6:06, Nikolay Borisov wrote:
>>>>>>> Currently when a device is missing for a mounted filesystem the
>>>>>>> output
>>>>>>> that is produced is unhelpful:
>>>>>>>
>>>>>>> Label: none  uuid: 139ef309-021f-4b98-a3a8-ce230a83b1e2
>>>>>>>        Total devices 2 FS bytes used 128.00KiB
>>>>>>>        devid    1 size 5.00GiB used 1.26GiB path /dev/loop0
>>>>>>>        *** Some devices missing
>>>>>>>
>>>>>>> While the context which prints this is perfectly capable of showing
>>>>>>> which device exactly is missing, like so:
>>>>>>>
>>>>>>> Label: none  uuid: 4a85a40b-9b79-4bde-8e52-c65a550a176b
>>>>>>>        Total devices 2 FS bytes used 128.00KiB
>>>>>>>        devid    1 size 5.00GiB used 1.26GiB path /dev/loop0
>>>>>>>        devid    2 size 0 used 0 path /dev/loop1 ***MISSING***
>>>>>>>
>>>>>>> This is a lot more usable output as it presents the user with the id
>>>>>>> of the missing device and its path.
>>>>>>
>>>>>> The idea is pretty awesome.
>>>>>>
>>>>>> Just one question, if one device is missing, how could we know its
>>>>>> path?
>>>>>> Thus does the device path output make any sense?
>>>>>
>>>>> The path is not canonicalized but otherwise the paths comes from
>>>>> btrfs_ioctl_dev_info_args which is filled by a call to get_fs_info
>>>>> where
>>>>> we call get_device_info for every device in the fs_info.
>>>>>
>>>>> So the path is really dev->name from kernel space or if we don't have a
>>>>> dev->name it will be 0. In either case it's useful that we get the
>>>>> devid
>>>>> so that the user can do :
>>>>>
>>>>> btrfs device remove 2 (if we take the above example), alternatively the
>>>>> path would be a NULL-terminated string which aka empty. I guess that's
>>>>> still better than simply saying *some devices are missing*
>>>>
>>>> Definitely the devid output is way better than the existing output.
>>>>
>>>> I just wonder can we skip the path completely since it's missing (and
>>>> under most case its NULL anyway).
>>>>
>>>> Despite that, I'm completely fine with the patch.
>>>
>>> As you can see form the test I have added this was tested rather
>>> synthetically by simply moving a loop device, in this case the device's
>>> record was still in the fs_devices and had the name, though the name
>>> itself couldn't be acted on. So omitting the path entirely is definitely
>>> something we could do, but I'd rather try and be a bit cleverer, simply
>>> checking if the name is null or not and if not just print it?
>>
>> Oh, I forgot the case where the stall path may still be there.
>>
>> In that case your existing one should be enough to handle it.
> 
> I realise this comment might be too late so feel free to ignore it if
> so. Could this path name potentially conflict with a (new) device using
> the same name? For example, could someone have created a new /dev/loop1?
> Or could a USB disk /dev/sdf1 (say) have been removed and a different
> disk inserted and acquired the /dev/sdf1 name? Or would that be
> prevented in the case where "the device's record was still in the
> fs_devices"?
> 
> If so, I think this could be very confusing to the user trying to work
> out what has happened. Maybe the output needs to change to something like:
> 
> devid    2 size 0 used 0 last seen as /dev/loop1 ***MISSING***
> 
> "last seen as" could just be "previously". Or, to make it even clearer
> that this is just a hint to help the user understand which device is
> missing, maybe something like "(last mounted as /dev/loop1)".

  Yeah. I agree. We found devid (only) was important in this bug report.
  Either 'last seen ..' or completely removing the device path will work.

Thanks, Anand

> 
> Graham
>