| Message ID | 20210902100643.1075385-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs-progs: fi show: Print missing device for a mounted file system | expand |
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; > } >
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; >> } >> >
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; >>> } >>> >>
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; >>>> } >>>> >>> >
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; > } >
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; >>>>> } >>>>> >>>> >>
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>
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> >
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> >> >
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
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 >
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 >
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
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 >
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.
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.
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 >
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.
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; }
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(-)