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

[Nikolay Borisov <nborisov@suse.com>]

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?

> 
> Thanks,
> Qu
> 
>>
>>>
>>> Thanks,
>>> Qu
>>>>
>>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>>> ---
>>>>    cmds/filesystem.c | 7 +++----
>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/cmds/filesystem.c b/cmds/filesystem.c
>>>> index db8433ba3542..ff13de6ac990 100644
>>>> --- a/cmds/filesystem.c
>>>> +++ b/cmds/filesystem.c
>>>> @@ -295,7 +295,6 @@ static int print_one_fs(struct
>>>> btrfs_ioctl_fs_info_args *fs_info,
>>>>    {
>>>>        int i;
>>>>        int fd;
>>>> -    int missing = 0;
>>>>        char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>>>>        struct btrfs_ioctl_dev_info_args *tmp_dev_info;
>>>>        int ret;
>>>> @@ -325,8 +324,10 @@ static int print_one_fs(struct
>>>> btrfs_ioctl_fs_info_args *fs_info,
>>>>            /* Add check for missing devices even mounted */
>>>>            fd = open((char *)tmp_dev_info->path, O_RDONLY);
>>>>            if (fd < 0) {
>>>> -            missing = 1;
>>>> +            printf("\tdevid %4llu size 0 used 0 path %s
>>>> ***MISSING***\n",
>>>> +                    tmp_dev_info->devid,tmp_dev_info->path);
>>>>                continue;
>>>> +
>>>>            }
>>>>            close(fd);
>>>>            canonical_path = path_canonicalize((char
>>>> *)tmp_dev_info->path);
>>>> @@ -339,8 +340,6 @@ static int print_one_fs(struct
>>>> btrfs_ioctl_fs_info_args *fs_info,
>>>>            free(canonical_path);
>>>>        }
>>>>
>>>> -    if (missing)
>>>> -        printf("\t*** Some devices missing\n");
>>>>        printf("\n");
>>>>        return 0;
>>>>    }
>>>>
>>>
>