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

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

[Nikolay Borisov]

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.

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;
 }
-- 
2.17.1

[PATCH 2/2] btrfs-progs: tests: Add test for fi show

[Nikolay Borisov]

Add a test to ensure that 'btrfs fi show' on a mounted filesyste, which
has a missing device will explicitly print which device is missing.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tests/cli-tests/016-fi-show-missing/test.sh | 35 +++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100755 tests/cli-tests/016-fi-show-missing/test.sh

diff --git a/tests/cli-tests/016-fi-show-missing/test.sh b/tests/cli-tests/016-fi-show-missing/test.sh
new file mode 100755
index 000000000000..9df7afd5af94
--- /dev/null
+++ b/tests/cli-tests/016-fi-show-missing/test.sh
@@ -0,0 +1,35 @@
+#!/bin/bash
+#
+# Test that if a device is missing for a mounted filesystem, btrfs fi show will
+# show which device exactly is missing.
+
+source "$TEST_TOP/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+setup_loopdevs 2
+prepare_loopdevs
+
+dev1=${loopdevs[1]}
+dev2=${loopdevs[2]}
+
+run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f -draid1 $dev1 $dev2
+# move the device, changing its path, simulating the device being missing
+mv $dev2 /dev/loop-non-existent
+
+run_check $SUDO_HELPER mount -o degraded $dev1 $TEST_MNT
+
+if ! run_check_stdout $SUDO_HELPER "$TOP"/btrfs fi show $TEST_MNT | \
+	grep -q "$dev2 \*\*\*MISSING\*\*\*"; then
+
+	_fail "Didn't find exact missing device"
+fi
+
+mv /dev/loop-non-existent $dev2
+
+run_check $SUDO_HELPER umount $TEST_MNT
+
+cleanup_loopdevs
+
-- 
2.17.1

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

[Qu Wenruo]

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?

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;
>   }
>

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

[Nikolay Borisov]

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*

> 
> 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;
>>   }
>>
> 

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

[Qu Wenruo]

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.

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;
>>>    }
>>>
>>

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

[Nikolay Borisov]

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;
>>>>    }
>>>>
>>>
> 

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

[Qu Wenruo]

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.

printf() can handle NULL pointer for %s without problem.

Then it should be OK.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
>
>>
>> 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;
>>>>>     }
>>>>>
>>>>
>>

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

[g.btrfs]

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

Graham

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

[Anand Jain]

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
> 

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

[Nikolay Borisov]

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                 }

> 
> Graham
> 

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

[Graham Cobb]

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

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

[Nikolay Borisov]

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
> 

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

[Goffredo Baroncelli]

On 9/6/21 6:47 PM, g.btrfs@cobb.uk.net wrote:
> 
> On 02/09/2021 13:17, Qu Wenruo wrote:
>>
[...]
> 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)".
> 


My 2¢:

   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 ***MISSING*** (last seen as /dev/loop1)

I suggest to swap MISSING and 'last seen as...' part, because I think that the most
important part of the message is MISSING.


> Graham
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

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

[Anand Jain]

On 02/09/2021 18: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.

Looks better. How does this fair in the case of unmounted btrfs? Because
btrfs fi show is supposed to work on an unmounted btrfs to help find
btrfs devices.

Thx, Anand


> 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;
>   }
> 

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

[Nikolay Borisov]

On 2.09.21 г. 14:44, Anand Jain wrote:
> On 02/09/2021 18: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.
> 
> Looks better. How does this fair in the case of unmounted btrfs? Because
> btrfs fi show is supposed to work on an unmounted btrfs to help find
> btrfs devices.

On unmounted fs the output is unchanged - i.e we simply print "missing
device" because there is no way to derive the name of the missing
device. Check print_one_uuid for reference.

<snip>

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

[Anand Jain]

On 02/09/2021 22:28, Nikolay Borisov wrote:
> 
> 
> On 2.09.21 г. 14:44, Anand Jain wrote:
>> On 02/09/2021 18: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 change has to percolate to xfstests as well. I think btrfs/197,
btrfs/198 and btrfs/003 depends on the existing format. IMO those
test cases still have to be backward btrfs-progs compatible.

Thanks, Anand


>>>
>>> This is a lot more usable output as it presents the user with the id
>>> of the missing device and its path.
>>
>> Looks better. How does this fair in the case of unmounted btrfs? Because
>> btrfs fi show is supposed to work on an unmounted btrfs to help find
>> btrfs devices.
> 
> On unmounted fs the output is unchanged - i.e we simply print "missing
> device" because there is no way to derive the name of the missing
> device. Check print_one_uuid for reference.
> 
> <snip>
> 

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

[Nikolay Borisov]

On 3.09.21 г. 8:12, Anand Jain wrote:
> On 02/09/2021 22:28, Nikolay Borisov wrote:
>>
>>
>> On 2.09.21 г. 14:44, Anand Jain wrote:
>>> On 02/09/2021 18: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 change has to percolate to xfstests as well. I think btrfs/197,
> btrfs/198 and btrfs/003 depends on the existing format. IMO those
> test cases still have to be backward btrfs-progs compatible.

Actually it's only btrfs/197, I will fix it.

> 
> Thanks, Anand
> 
> 
>>>>
>>>> This is a lot more usable output as it presents the user with the id
>>>> of the missing device and its path.
>>>
>>> Looks better. How does this fair in the case of unmounted btrfs? Because
>>> btrfs fi show is supposed to work on an unmounted btrfs to help find
>>> btrfs devices.
>>
>> On unmounted fs the output is unchanged - i.e we simply print "missing
>> device" because there is no way to derive the name of the missing
>> device. Check print_one_uuid for reference.
>>
>> <snip>
>>
> 

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

[David Sterba]

On Thu, Sep 02, 2021 at 01:06:42PM +0300, 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***

There was a discussion regarding the output, so what's the last version?
The path is not always available, so how does the output look like in
that case? I'd vote against the *** markers and rather establish some
parseable format, like

 	devid    2 size 0 used 0 MISSING (last: /dev/loop1)

The path is optional so it would bebetter to put it at the end.

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

[Nikolay Borisov]

On 7.09.21 г. 16:50, David Sterba wrote:
> There was a discussion regarding the output, so what's the last version?
> The path is not always available, so how does the output look like in
> that case? I'd vote against the *** markers and rather establish some
> parseable format, like
> 
>  	devid    2 size 0 used 0 MISSING (last: /dev/loop1)
> 
> The path is optional so it would bebetter to put it at the end.

I'm fine with this format, will send updated patches.

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

[Nikolay Borisov]

On 7.09.21 г. 16:50, David Sterba wrote:
> devid    2 size 0 used 0 MISSING (last: /dev/loop1)


unfortunately that 'last:' is a bit cryptic, OTOH 'last seen as' sounds
a bit verbose. So I'd rather leave it as :

devid    2 size 0 used 0 path /dev/foo MISSING


In case we don't have a path for the device then the output would be :
devid    2 size 0 used 0 path  MISSING

I think that's simple enough.

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

[Nikolay Borisov]

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.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

Resending this series as it seems to have fallen through the cracks.


 cmds/filesystem.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 7cd08fcd5f62..fe32838a25bf 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -296,7 +296,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;
@@ -326,8 +325,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);
@@ -340,8 +341,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;
 }
--
2.17.1

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

[Nikolay Borisov]

On 29.03.22 г. 11:30 ч., 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.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>


Ping

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

[David Sterba]

On Tue, Jun 07, 2022 at 11:35:45AM +0300, Nikolay Borisov wrote:
> 
> 
> On 29.03.22 г. 11:30 ч., 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.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> 
> Ping

Sorry for late reponse, added to devel, thanks.