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

[Graham Cobb <g.btrfs@cobb.uk.net>]

On 07/09/2021 07:03, Nikolay Borisov wrote:
> 
> 
> On 6.09.21 г. 19: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)".
> 
> Actually in the case of mounted file systems this cannot happen because
> the missing bit is printed *only* if the device's path cannot be opened,
> i.e it's indeed missing:
> 
> 
> /* Add check for missing devices even mounted */
>      9                 fd = open((char *)tmp_dev_info->path, O_RDONLY);
>      8                 if (fd < 0) {
>      7                         printf("\tdevid %4llu size 0 used 0 path
> %s *MISSING*\n",
>      6                                tmp_dev_info->devid,
> canonical_path);
>      5                         continue;
>      4                 }

Thanks Nikolay. That makes sense. So to see the MISSING message with a
device name, it must have been present when the fs was mounted, got
included in the fs when it was mounted, but have gone missing later such
that a subsequent "open" fails. Is that correct?

Presumably btrfs is still holding the original fd used at mount time
open so the device name (if it can be opened at all) cannot now refer to
something else.

Graham