| Message ID | e4e0299f46b9b4715039cd74f90944d9357446af.1688724045.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: fix bugs and CHANGING_FSID_V2 flag | expand |
On Fri, Jul 07, 2023 at 11:52:32PM +0800, Anand Jain wrote: > When we add a struct btrfs_device for the missing device, announce a > warning indicating that a device is missing. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > kernel-shared/volumes.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c > index 92282524867d..d20cb3177a34 100644 > --- a/kernel-shared/volumes.c > +++ b/kernel-shared/volumes.c > @@ -2252,6 +2252,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, > > device = btrfs_find_device(fs_info, devid, dev_uuid, fs_uuid); > if (!device) { > + warning("device id %llu is missing", devid); read_one_dev() is a low level helper that should not print any messages, the calling context is not known. If you really want to print such message then please move it to the appropriate caller. > device = kzalloc(sizeof(*device), GFP_NOFS); > if (!device) > return -ENOMEM; > -- > 2.39.3
On 14/07/2023 02:48, David Sterba wrote: > On Fri, Jul 07, 2023 at 11:52:32PM +0800, Anand Jain wrote: >> When we add a struct btrfs_device for the missing device, announce a >> warning indicating that a device is missing. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> kernel-shared/volumes.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c >> index 92282524867d..d20cb3177a34 100644 >> --- a/kernel-shared/volumes.c >> +++ b/kernel-shared/volumes.c >> @@ -2252,6 +2252,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, >> >> device = btrfs_find_device(fs_info, devid, dev_uuid, fs_uuid); >> if (!device) { >> + warning("device id %llu is missing", devid); > > read_one_dev() is a low level helper that should not print any messages, > the calling context is not known. If you really want to print such > message then please move it to the appropriate caller. The motivation for adding the warning comes from the following test: Despite the missing device, btrfstune -m was still able to change the fsid. $ mkfs.btrfs -f -dsingle -msingle /dev/sdb1 /dev/sdb2 $ wipefs -a /dev/sdb2 $ btrfstune -m /dev/sdb1 This series fixes the bug; that is, it makes btrfstune fail in this test case and this particular patch reports the missing device. Which is similar to the behavior in the kernel code. read_one_dev() function is only used by btrfs_read_chunk_tree(), which is not aware of the missing device through the returned error code. Perhaps we can make an exception here? Thanks. > >> device = kzalloc(sizeof(*device), GFP_NOFS); >> if (!device) >> return -ENOMEM; >> -- >> 2.39.3
On 18/07/2023 02:22, Anand Jain wrote: > > > On 14/07/2023 02:48, David Sterba wrote: >> On Fri, Jul 07, 2023 at 11:52:32PM +0800, Anand Jain wrote: >>> When we add a struct btrfs_device for the missing device, announce a >>> warning indicating that a device is missing. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> kernel-shared/volumes.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c >>> index 92282524867d..d20cb3177a34 100644 >>> --- a/kernel-shared/volumes.c >>> +++ b/kernel-shared/volumes.c >>> @@ -2252,6 +2252,7 @@ static int read_one_dev(struct btrfs_fs_info >>> *fs_info, >>> device = btrfs_find_device(fs_info, devid, dev_uuid, fs_uuid); >>> if (!device) { >>> + warning("device id %llu is missing", devid); >> >> read_one_dev() is a low level helper that should not print any messages, >> the calling context is not known. If you really want to print such >> message then please move it to the appropriate caller. Furthermore, there is only one caller from the top function: open_ctree context. __open_ctree_fd() btrfs_setup_chunk_tree_and_device_map() btrfs_read_chunk_tree() read_one_dev() Do you remember any failing test cases or potential concerns if we print the identified missing device information here? Thanks. > > The motivation for adding the warning comes from the following test: > Despite the missing device, btrfstune -m was still able to change > the fsid. > > $ mkfs.btrfs -f -dsingle -msingle /dev/sdb1 /dev/sdb2 > $ wipefs -a /dev/sdb2 > $ btrfstune -m /dev/sdb1 > > This series fixes the bug; that is, it makes btrfstune fail in this > test case and this particular patch reports the missing device. > > Which is similar to the behavior in the kernel code. > > read_one_dev() function is only used by btrfs_read_chunk_tree(), > which is not aware of the missing device through the returned > error code. > > Perhaps we can make an exception here? > > Thanks. >> >>> device = kzalloc(sizeof(*device), GFP_NOFS); >>> if (!device) >>> return -ENOMEM; >>> -- >>> 2.39.3
diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c index 92282524867d..d20cb3177a34 100644 --- a/kernel-shared/volumes.c +++ b/kernel-shared/volumes.c @@ -2252,6 +2252,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, device = btrfs_find_device(fs_info, devid, dev_uuid, fs_uuid); if (!device) { + warning("device id %llu is missing", devid); device = kzalloc(sizeof(*device), GFP_NOFS); if (!device) return -ENOMEM;
When we add a struct btrfs_device for the missing device, announce a warning indicating that a device is missing. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- kernel-shared/volumes.c | 1 + 1 file changed, 1 insertion(+)