Re: [PATCH 02/11] btrfs-progs: call warn() for missing device

[Anand Jain <anand.jain@oracle.com>]

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