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

[Nikolay Borisov <nborisov@suse.com>]

On 7.09.21 г. 11:59, Graham Cobb wrote:
> 
> 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?

No, a filesystem can be mounted with a missing device from the beginning
by using the -o degraded, so it's possible that the kernel doesn't even
have a possible path for the device. In this case the path would be
likely an empty string. So the path is indeed at best advisory

> 
> 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.

Btrfs does indeed keep a particular device open while it's mounted
however if it disappear, and later a device re-appears nothing prevends
udev to give it the same name. I.e btrfs cannot control what name a new
device would have.



> 
> Graham
>