[PATCH 0/2] fix verify_one_dev_extent and btrfs_find_device

[PATCH 0/2] fix verify_one_dev_extent and btrfs_find_device

[Anand Jain]

btrfs_find_device()'s last arg %seed is unused, which the commit
343694eee8d8 (btrfs: switch seed device to list api) ignored purposely
or missed.

But there isn't any regression due to that. And this series makes
it official that btrfs_find_device() doesn't need the last arg.

To achieve that patch 1/2 critically reviews the need for the check
disk_total_bytes == 0 in the function verify_one_dev_extent() and finds
that, the condition is never met and so deletes the same. Which also
drops one of the parents of btrfs_find_device() with the last arg false.

So only device_list_add() is using btrfs_find_device() with the last arg as
false, which the patch 2/2 finds is not required as well. So
this patch drops the last arg in btrfs_find_device() altogether.

Anand Jain (2):
  btrfs: drop never met condition of disk_total_bytes == 0
  btrfs: fix btrfs_find_device unused arg seed

 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 37 ++++++++++---------------------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

btrfs_device::disk_total_bytes is set even for a seed device (the
comment is wrong).

The function fill_device_from_item() does the job of reading it from the
item and updating btrfs_device::disk_total_bytes. So both the missing
device and the seed devices do have their disk_total_bytes updated.

So this patch removes the check dev->disk_total_bytes == 0 in the
function verify_one_dev_extent()

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7f43ed88fffc..9be40eece8ed 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	/* It's possible this device is a dummy for seed device */
-	if (dev->disk_total_bytes == 0) {
-		struct btrfs_fs_devices *devs;
-
-		devs = list_first_entry(&fs_info->fs_devices->seed_list,
-					struct btrfs_fs_devices, seed_list);
-		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
-		if (!dev) {
-			btrfs_err(fs_info, "failed to find seed devid %llu",
-				  devid);
-			ret = -EUCLEAN;
-			goto out;
-		}
-	}
-
 	if (physical_offset + physical_len > dev->disk_total_bytes) {
 		btrfs_err(fs_info,
 "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
-- 
2.25.1

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Nikolay Borisov]

On 24.09.20 г. 13:11 ч., Anand Jain wrote:
> btrfs_device::disk_total_bytes is set even for a seed device (the
> comment is wrong).
> 
> The function fill_device_from_item() does the job of reading it from the
> item and updating btrfs_device::disk_total_bytes. So both the missing
> device and the seed devices do have their disk_total_bytes updated.
> 
> So this patch removes the check dev->disk_total_bytes == 0 in the
> function verify_one_dev_extent()
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7f43ed88fffc..9be40eece8ed 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>  		goto out;
>  	}
>  
> -	/* It's possible this device is a dummy for seed device */
> -	if (dev->disk_total_bytes == 0) {
> -		struct btrfs_fs_devices *devs;
> -
> -		devs = list_first_entry(&fs_info->fs_devices->seed_list,
> -					struct btrfs_fs_devices, seed_list);
> -		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
> -		if (!dev) {
> -			btrfs_err(fs_info, "failed to find seed devid %llu",
> -				  devid);
> -			ret = -EUCLEAN;
> -			goto out;
> -		}
> -	}

The commit which introduced this check states that the device with a
disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed.
It seems the check is legit and your changelog doesn't account for that
if it's safe you should provide description why is that.

> -
>  	if (physical_offset + physical_len > dev->disk_total_bytes) {
>  		btrfs_err(fs_info,
>  "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
> 

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Qu Wenruo]

On 2020/9/24 下午7:48, Nikolay Borisov wrote:
>
>
> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>> btrfs_device::disk_total_bytes is set even for a seed device (the
>> comment is wrong).
>>
>> The function fill_device_from_item() does the job of reading it from the
>> item and updating btrfs_device::disk_total_bytes. So both the missing
>> device and the seed devices do have their disk_total_bytes updated.
>>
>> So this patch removes the check dev->disk_total_bytes == 0 in the
>> function verify_one_dev_extent()
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>  fs/btrfs/volumes.c | 15 ---------------
>>  1 file changed, 15 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 7f43ed88fffc..9be40eece8ed 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>>  		goto out;
>>  	}
>>
>> -	/* It's possible this device is a dummy for seed device */
>> -	if (dev->disk_total_bytes == 0) {
>> -		struct btrfs_fs_devices *devs;
>> -
>> -		devs = list_first_entry(&fs_info->fs_devices->seed_list,
>> -					struct btrfs_fs_devices, seed_list);
>> -		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
>> -		if (!dev) {
>> -			btrfs_err(fs_info, "failed to find seed devid %llu",
>> -				  devid);
>> -			ret = -EUCLEAN;
>> -			goto out;
>> -		}
>> -	}
>
> The commit which introduced this check states that the device with a
> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed.
> It seems the check is legit and your changelog doesn't account for that
> if it's safe you should provide description why is that.

And it would be even better to mention the fragmentation problem in the
man page for btrfs-convert.

The fragmentation problem is a little too complex to explain in the
error message nor usage.

Thanks,
Qu
>
>> -
>>  	if (physical_offset + physical_len > dev->disk_total_bytes) {
>>  		btrfs_err(fs_info,
>>  "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
>>

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Qu Wenruo]

On 2020/9/24 下午7:58, Qu Wenruo wrote:
>
>
> On 2020/9/24 下午7:48, Nikolay Borisov wrote:
>>
>>
>> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>>> btrfs_device::disk_total_bytes is set even for a seed device (the
>>> comment is wrong).
>>>
>>> The function fill_device_from_item() does the job of reading it from the
>>> item and updating btrfs_device::disk_total_bytes. So both the missing
>>> device and the seed devices do have their disk_total_bytes updated.
>>>
>>> So this patch removes the check dev->disk_total_bytes == 0 in the
>>> function verify_one_dev_extent()
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>  fs/btrfs/volumes.c | 15 ---------------
>>>  1 file changed, 15 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 7f43ed88fffc..9be40eece8ed 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>>>  		goto out;
>>>  	}
>>>
>>> -	/* It's possible this device is a dummy for seed device */
>>> -	if (dev->disk_total_bytes == 0) {
>>> -		struct btrfs_fs_devices *devs;
>>> -
>>> -		devs = list_first_entry(&fs_info->fs_devices->seed_list,
>>> -					struct btrfs_fs_devices, seed_list);
>>> -		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
>>> -		if (!dev) {
>>> -			btrfs_err(fs_info, "failed to find seed devid %llu",
>>> -				  devid);
>>> -			ret = -EUCLEAN;
>>> -			goto out;
>>> -		}
>>> -	}
>>
>> The commit which introduced this check states that the device with a
>> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed.
>> It seems the check is legit and your changelog doesn't account for that
>> if it's safe you should provide description why is that.
>
> And it would be even better to mention the fragmentation problem in the
> man page for btrfs-convert.
>
> The fragmentation problem is a little too complex to explain in the
> error message nor usage.

Please ignore this, I replied to the wrong thread...

>
> Thanks,
> Qu
>>
>>> -
>>>  	if (physical_offset + physical_len > dev->disk_total_bytes) {
>>>  		btrfs_err(fs_info,
>>>  "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
>>>

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

On 24/9/20 7:48 pm, Nikolay Borisov wrote:
> 
> 
> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>> btrfs_device::disk_total_bytes is set even for a seed device (the
>> comment is wrong).
>>
>> The function fill_device_from_item() does the job of reading it from the
>> item and updating btrfs_device::disk_total_bytes. So both the missing
>> device and the seed devices do have their disk_total_bytes updated.
>>
>> So this patch removes the check dev->disk_total_bytes == 0 in the
>> function verify_one_dev_extent()
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 15 ---------------
>>   1 file changed, 15 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 7f43ed88fffc..9be40eece8ed 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>>   		goto out;
>>   	}
>>   
>> -	/* It's possible this device is a dummy for seed device */
>> -	if (dev->disk_total_bytes == 0) {
>> -		struct btrfs_fs_devices *devs;
>> -
>> -		devs = list_first_entry(&fs_info->fs_devices->seed_list,
>> -					struct btrfs_fs_devices, seed_list);
>> -		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
>> -		if (!dev) {
>> -			btrfs_err(fs_info, "failed to find seed devid %llu",
>> -				  devid);
>> -			ret = -EUCLEAN;
>> -			goto out;
>> -		}
>> -	}
> 
> The commit which introduced this check states that the device with a
> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed.
> It seems the check is legit and your changelog doesn't account for that
> if it's safe you should provide description why is that.

yes the commit 1b3922a8bc74 (btrfs: Use real device structure to verify 
dev extent) introduced it. In Qu's analysis, it is unclear why the 
total_disk_bytes was 0.

Theoretically, all devices (including missing and seed) marked with the 
BTRFS_DEV_STATE_IN_FS_METADATA flag gets the total_disk_bytes updated at 
fill_device_from_item().

open_ctree()
  btrfs_read_chunk_tree()
   read_one_dev()
    open_seed_device()
    fill_device_from_item()

Even if verify_one_dev_extent() reports total_disk_bytes == 0, then IMO 
its a bug to be fixed somewhere else and not in verify_one_dev_extent() 
as it's just a messenger. It is never expected that a total_disk_bytes 
shall be zero.

So it is fine to drop the total_disk_bytes == 0 check.

Thanks, Anand



> 
>> -
>>   	if (physical_offset + physical_len > dev->disk_total_bytes) {
>>   		btrfs_err(fs_info,
>>   "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
>>

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Nikolay Borisov]

On 25.09.20 г. 7:17 ч., Anand Jain wrote:
> 
> 
> On 24/9/20 7:48 pm, Nikolay Borisov wrote:
>>
>>
>> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>>> btrfs_device::disk_total_bytes is set even for a seed device (the
>>> comment is wrong).
>>>
>>> The function fill_device_from_item() does the job of reading it from the
>>> item and updating btrfs_device::disk_total_bytes. So both the missing
>>> device and the seed devices do have their disk_total_bytes updated.
>>>
>>> So this patch removes the check dev->disk_total_bytes == 0 in the
>>> function verify_one_dev_extent()
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/volumes.c | 15 ---------------
>>>   1 file changed, 15 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 7f43ed88fffc..9be40eece8ed 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct
>>> btrfs_fs_info *fs_info,
>>>           goto out;
>>>       }
>>>   -    /* It's possible this device is a dummy for seed device */
>>> -    if (dev->disk_total_bytes == 0) {
>>> -        struct btrfs_fs_devices *devs;
>>> -
>>> -        devs = list_first_entry(&fs_info->fs_devices->seed_list,
>>> -                    struct btrfs_fs_devices, seed_list);
>>> -        dev = btrfs_find_device(devs, devid, NULL, NULL, false);
>>> -        if (!dev) {
>>> -            btrfs_err(fs_info, "failed to find seed devid %llu",
>>> -                  devid);
>>> -            ret = -EUCLEAN;
>>> -            goto out;
>>> -        }
>>> -    }
>>
>> The commit which introduced this check states that the device with a
>> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed.
>> It seems the check is legit and your changelog doesn't account for that
>> if it's safe you should provide description why is that.
> 
> yes the commit 1b3922a8bc74 (btrfs: Use real device structure to verify
> dev extent) introduced it. In Qu's analysis, it is unclear why the
> total_disk_bytes was 0.
> 
> Theoretically, all devices (including missing and seed) marked with the
> BTRFS_DEV_STATE_IN_FS_METADATA flag gets the total_disk_bytes updated at
> fill_device_from_item().
> 
> open_ctree()
>  btrfs_read_chunk_tree()
>   read_one_dev()
>    open_seed_device() 

This function returns the cloned fs_devices whose devices are not
initialized. Later, in read_one_dev the 'device' is acquired from
fs_info->fs_devices, not from the returned fs_devices :

device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
                           fs_uuid, true);

And finally fill_device_from_item(leaf, dev_item, device); is called for
the device which was found from fs_info->fs_devices and not from the
returned 'fs_devices' from :

fs_devices = open_seed_devices(fs_info, fs_uuid);

What this means is that struct btrfs_device of devices in
fs_info->seed_list is never fully initialized.

>    fill_device_from_item()
> 
> Even if verify_one_dev_extent() reports total_disk_bytes == 0, then IMO
> its a bug to be fixed somewhere else and not in verify_one_dev_extent()
> as it's just a messenger. It is never expected that a total_disk_bytes
> shall be zero.

I agree, however this would involve fixing clone_fs_devices to properly
initialize struct btrfs_device. I'm in favor of removing special casing.

Looking closer into verify_one_dev_extent I see that the device is
referenced from fs_info->fs_devices :

dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);

And indeed, it seems that all devices of fs_info->fs_devices are
initialized as per your above explanation. So yeah, your patch is
codewise correct but the changelog is wrong:

disk_total_bytes is never set for seed devices (seed devices are after
all housed in fs_info->seed_list which as I explained above doesn't
fully initialize btrfs_devices)

A better changelog would document following invariants:

1. Seed devices don't have their disktotal bytes initialized

2. In spite of (1), verify_dev_one_extent is never called for such
devices so it's fine because devices anchored at fs_info->fs_devices are
always properly initialized.


<snip>

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

On 25/9/20 2:19 pm, Nikolay Borisov wrote:
> 
> 
> On 25.09.20 г. 7:17 ч., Anand Jain wrote:
>>
>>
>> On 24/9/20 7:48 pm, Nikolay Borisov wrote:
>>>
>>>
>>> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>>>> btrfs_device::disk_total_bytes is set even for a seed device (the
>>>> comment is wrong).
>>>>
>>>> The function fill_device_from_item() does the job of reading it from the
>>>> item and updating btrfs_device::disk_total_bytes. So both the missing
>>>> device and the seed devices do have their disk_total_bytes updated.
>>>>
>>>> So this patch removes the check dev->disk_total_bytes == 0 in the
>>>> function verify_one_dev_extent()
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>>    fs/btrfs/volumes.c | 15 ---------------
>>>>    1 file changed, 15 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 7f43ed88fffc..9be40eece8ed 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -7578,21 +7578,6 @@ static int verify_one_dev_extent(struct
>>>> btrfs_fs_info *fs_info,
>>>>            goto out;
>>>>        }
>>>>    -    /* It's possible this device is a dummy for seed device */
>>>> -    if (dev->disk_total_bytes == 0) {
>>>> -        struct btrfs_fs_devices *devs;
>>>> -
>>>> -        devs = list_first_entry(&fs_info->fs_devices->seed_list,
>>>> -                    struct btrfs_fs_devices, seed_list);
>>>> -        dev = btrfs_find_device(devs, devid, NULL, NULL, false);
>>>> -        if (!dev) {
>>>> -            btrfs_err(fs_info, "failed to find seed devid %llu",
>>>> -                  devid);
>>>> -            ret = -EUCLEAN;
>>>> -            goto out;
>>>> -        }
>>>> -    }
>>>
>>> The commit which introduced this check states that the device with a
>>> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed.
>>> It seems the check is legit and your changelog doesn't account for that
>>> if it's safe you should provide description why is that.
>>
>> yes the commit 1b3922a8bc74 (btrfs: Use real device structure to verify
>> dev extent) introduced it. In Qu's analysis, it is unclear why the
>> total_disk_bytes was 0.
>>
>> Theoretically, all devices (including missing and seed) marked with the
>> BTRFS_DEV_STATE_IN_FS_METADATA flag gets the total_disk_bytes updated at
>> fill_device_from_item().
>>
>> open_ctree()
>>   btrfs_read_chunk_tree()
>>    read_one_dev()
>>     open_seed_device()
> 
> This function returns the cloned fs_devices whose devices are not
> initialized. Later, in read_one_dev the 'device' is acquired from
> fs_info->fs_devices, not from the returned fs_devices :
> 
> device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
>                             fs_uuid, true);
> >
> And finally fill_device_from_item(leaf, dev_item, device); is called for
> the device which was found from fs_info->fs_devices and not from the
> returned 'fs_devices' from :
> 
> fs_devices = open_seed_devices(fs_info, fs_uuid);
> 
> What this means is that struct btrfs_device of devices in
> fs_info->seed_list is never fully initialized.


> 
>>     fill_device_from_item()

>>
>> Even if verify_one_dev_extent() reports total_disk_bytes == 0, then IMO
>> its a bug to be fixed somewhere else and not in verify_one_dev_extent()
>> as it's just a messenger. It is never expected that a total_disk_bytes
>> shall be zero.
> 
> I agree, however this would involve fixing clone_fs_devices to properly
> initialize struct btrfs_device. I'm in favor of removing special casing.
> 

  Cleanups are welcome. But function fill_device_from_item() is already 
properly initializing the struct btrfs_device. clone_fs_devices() is 
called at more than one place, so IMO it is fair.


> Looking closer into verify_one_dev_extent I see that the device is
> referenced from fs_info->fs_devices :
> 
> dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
> 
> And indeed, it seems that all devices of fs_info->fs_devices are
> initialized as per your above explanation. So yeah, your patch is


> codewise correct but the changelog is wrong:
 >
> disk_total_bytes is never set for seed devices (seed devices are after
> all housed in fs_info->seed_list which as I explained above doesn't
> fully initialize btrfs_devices)

  With all due respect, did you miss to check what 
fill_device_from_item() is doing?


> A better changelog would document following invariants:
> 
> 1. Seed devices don't have their disktotal bytes initialized
> 

  This is wrong.

> 2. In spite of (1), verify_dev_one_extent is never called for such
> devices so it's fine because devices anchored at fs_info->fs_devices are
> always properly initialized.
>

Even this is wrong. Generally seed's devid is 1, and
btrfs_verify_dev_extents() starts verifying from the dev object id = 1.
So typically, the seed will be the first device that gets verified. As 
btrfs_read_chunk_tree() is called before btrfs_verify_dev_extents() so 
the btrfs_device is properly initialized before the verify check.

2817 int __cold open_ctree

3073         ret = btrfs_read_chunk_tree(fs_info);  <-- seed init
::
3106         ret = btrfs_verify_dev_extents(fs_info);


Thanks, Anand

> 
> <snip>
> 

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Nikolay Borisov]

On 25.09.20 г. 10:33 ч., Anand Jain wrote:
> 
> 
<snip>

> 
> Even this is wrong. Generally seed's devid is 1, and
> btrfs_verify_dev_extents() starts verifying from the dev object id = 1.
> So typically, the seed will be the first device that gets verified. As
> btrfs_read_chunk_tree() is called before btrfs_verify_dev_extents() so
> the btrfs_device is properly initialized before the verify check.
> 
> 2817 int __cold open_ctree
> 
> 3073         ret = btrfs_read_chunk_tree(fs_info);  <-- seed init
> ::
> 3106         ret = btrfs_verify_dev_extents(fs_info);


Fair, I missed that btrfs_find_device can also return a device from
fs_info->seed_list because it searches it as well. So this indeed means
all devices are initialized by fill_device_from_item so :

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

Still I think this mechanism ought to be explicitly described in the
changelog i.e mentioning that btrfs_find_device also returns seed devices.


> 
> 
> Thanks, Anand
> 
>>
>> <snip>
>>
> 

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

On 25/9/20 3:56 pm, Nikolay Borisov wrote:
> 
> 
> On 25.09.20 г. 10:33 ч., Anand Jain wrote:
>>
>>
> <snip>
> 
>>
>> Even this is wrong. Generally seed's devid is 1, and
>> btrfs_verify_dev_extents() starts verifying from the dev object id = 1.
>> So typically, the seed will be the first device that gets verified. As
>> btrfs_read_chunk_tree() is called before btrfs_verify_dev_extents() so
>> the btrfs_device is properly initialized before the verify check.
>>
>> 2817 int __cold open_ctree
>>
>> 3073         ret = btrfs_read_chunk_tree(fs_info);  <-- seed init
>> ::
>> 3106         ret = btrfs_verify_dev_extents(fs_info);
> 
> 
> Fair, I missed that btrfs_find_device can also return a device from
> fs_info->seed_list because it searches it as well. > So this indeed means
> all devices are initialized by fill_device_from_item so :

Oh. The btrfs_find_device(). Yep something was missing in what you said
I wasn't sure what. But now it makes sense.

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

Thanks!

-Anand

> Still I think this mechanism ought to be explicitly described in the
> changelog i.e mentioning that btrfs_find_device also returns seed devices.
> 
> 
>>
>>
>> Thanks, Anand
>>
>>>
>>> <snip>
>>>
>>

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Josef Bacik]

On 9/24/20 6:11 AM, Anand Jain wrote:
> btrfs_device::disk_total_bytes is set even for a seed device (the
> comment is wrong).
> 
> The function fill_device_from_item() does the job of reading it from the
> item and updating btrfs_device::disk_total_bytes. So both the missing
> device and the seed devices do have their disk_total_bytes updated.
> 
> So this patch removes the check dev->disk_total_bytes == 0 in the
> function verify_one_dev_extent()
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Ok I understand that logically this check is incorrect, however we pull this 
value from the device item, which could be corrupt.  I'm looking around and I 
don't see a similar check in any of our other code, it should probably go in 
check_dev_item() maybe?  I think that removing this check is ok, but we need to 
make sure we catch the invalid case where total_disk_bytes == 0 somewhere, so 
please add that in the appropriate place.  Thanks,

Josef

Re: [PATCH 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

On 5/10/20 9:22 pm, Josef Bacik wrote:
> On 9/24/20 6:11 AM, Anand Jain wrote:
>> btrfs_device::disk_total_bytes is set even for a seed device (the
>> comment is wrong).
>>
>> The function fill_device_from_item() does the job of reading it from the
>> item and updating btrfs_device::disk_total_bytes. So both the missing
>> device and the seed devices do have their disk_total_bytes updated.
>>
>> So this patch removes the check dev->disk_total_bytes == 0 in the
>> function verify_one_dev_extent()
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Ok I understand that logically this check is incorrect, however we pull 
> this value from the device item, which could be corrupt.  I'm looking 
> around and I don't see a similar check in any of our other code, it 
> should probably go in check_dev_item() maybe?  I think that removing 
> this check is ok, but we need to make sure we catch the invalid case 
> where total_disk_bytes == 0 somewhere, so please add that in the 
> appropriate place.  Thanks,

We could check the upper limit based on the device size. But we can't
check for zero, because while removing the device if there is a power
loss, we could have a device with its total_bytes = 0, that's still
valid. I shall make this check in v2 in read_one_dev() as we need a
valid device->bdev.

But I have a question though- we have csum for everything to determine
offline corruption at the time of reading, so corruptions are taken care
of. An authenticated mount is a holistic fix for the crafted image
problems, and systems that aren't secured appropriately may be exposed
to crafted image problems. So do we have to handle each one of those
corruption/crafted-image cases independently, then?

Thanks, Anand

> 
> Josef

[PATCH v2 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

btrfs_device::disk_total_bytes is set even for a seed device (the
comment is wrong).

The function fill_device_from_item() does the job of reading it from the
item and updating btrfs_device::disk_total_bytes. So both the missing
device and the seed devices do have their disk_total_bytes updated.

Furthermore, while removing the device if there is a power loss, we could
have a device with its total_bytes = 0, that's still valid.

So this patch removes the check dev->disk_total_bytes == 0 in the
function verify_one_dev_extent(), which it does nothing in it.

And take this opportunity to introduce a check if the device::total_bytes
is more than the max device size in read_one_dev().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: add check if the total_bytes is more than the actual device size in
    read_one_dev().
    update change log.

 fs/btrfs/volumes.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2a5397fb4175..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6847,6 +6847,16 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	fill_device_from_item(leaf, dev_item, device);
+	if (device->bdev) {
+		u64 max_total_bytes = i_size_read(device->bdev->bd_inode);
+
+		if (device->total_bytes > max_total_bytes) {
+			btrfs_err(fs_info,
+			"device total_bytes should be below %llu but found %llu",
+				  max_total_bytes, device->total_bytes);
+			return -EINVAL;
+		}
+	}
 	set_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	   !test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
@@ -7579,21 +7589,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	/* It's possible this device is a dummy for seed device */
-	if (dev->disk_total_bytes == 0) {
-		struct btrfs_fs_devices *devs;
-
-		devs = list_first_entry(&fs_info->fs_devices->seed_list,
-					struct btrfs_fs_devices, seed_list);
-		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
-		if (!dev) {
-			btrfs_err(fs_info, "failed to find seed devid %llu",
-				  devid);
-			ret = -EUCLEAN;
-			goto out;
-		}
-	}
-
 	if (physical_offset + physical_len > dev->disk_total_bytes) {
 		btrfs_err(fs_info,
 "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
-- 
2.25.1

[PATCH RESEND v2 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

btrfs_device::disk_total_bytes is set even for a seed device (the
comment is wrong).

The function fill_device_from_item() does the job of reading it from the
item and updating btrfs_device::disk_total_bytes. So both the missing
device and the seed devices do have their disk_total_bytes updated.

Furthermore, while removing the device if there is a power loss, we could
have a device with its total_bytes = 0, that's still valid.

So this patch removes the check dev->disk_total_bytes == 0 in the
function verify_one_dev_extent(), which it does nothing in it.

And take this opportunity to introduce a check if the device::total_bytes
is more than the max device size in read_one_dev().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v2: add check if the total_bytes is more than the actual device size in
    read_one_dev().
    update change log.

 fs/btrfs/volumes.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2a5397fb4175..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6847,6 +6847,16 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	fill_device_from_item(leaf, dev_item, device);
+	if (device->bdev) {
+		u64 max_total_bytes = i_size_read(device->bdev->bd_inode);
+
+		if (device->total_bytes > max_total_bytes) {
+			btrfs_err(fs_info,
+			"device total_bytes should be below %llu but found %llu",
+				  max_total_bytes, device->total_bytes);
+			return -EINVAL;
+		}
+	}
 	set_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	   !test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
@@ -7579,21 +7589,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	/* It's possible this device is a dummy for seed device */
-	if (dev->disk_total_bytes == 0) {
-		struct btrfs_fs_devices *devs;
-
-		devs = list_first_entry(&fs_info->fs_devices->seed_list,
-					struct btrfs_fs_devices, seed_list);
-		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
-		if (!dev) {
-			btrfs_err(fs_info, "failed to find seed devid %llu",
-				  devid);
-			ret = -EUCLEAN;
-			goto out;
-		}
-	}
-
 	if (physical_offset + physical_len > dev->disk_total_bytes) {
 		btrfs_err(fs_info,
 "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
-- 
2.25.1

Re: [PATCH RESEND v2 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Josef Bacik]

On 10/21/20 12:16 AM, Anand Jain wrote:
> btrfs_device::disk_total_bytes is set even for a seed device (the
> comment is wrong).
> 
> The function fill_device_from_item() does the job of reading it from the
> item and updating btrfs_device::disk_total_bytes. So both the missing
> device and the seed devices do have their disk_total_bytes updated.
> 
> Furthermore, while removing the device if there is a power loss, we could
> have a device with its total_bytes = 0, that's still valid.
> 
> So this patch removes the check dev->disk_total_bytes == 0 in the
> function verify_one_dev_extent(), which it does nothing in it.
> 
> And take this opportunity to introduce a check if the device::total_bytes
> is more than the max device size in read_one_dev().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> v2: add check if the total_bytes is more than the actual device size in
>      read_one_dev().
>      update change log.
> 
>   fs/btrfs/volumes.c | 25 ++++++++++---------------
>   1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2a5397fb4175..0c6049f9ace3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6847,6 +6847,16 @@ static int read_one_dev(struct extent_buffer *leaf,
>   	}
>   
>   	fill_device_from_item(leaf, dev_item, device);
> +	if (device->bdev) {
> +		u64 max_total_bytes = i_size_read(device->bdev->bd_inode);
> +
> +		if (device->total_bytes > max_total_bytes) {
> +			btrfs_err(fs_info,
> +			"device total_bytes should be below %llu but found %llu",

"should be less than or equal to"

Thanks,

Josef

Re: [PATCH RESEND v2 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

On 21/10/20 10:35 pm, Josef Bacik wrote:
> On 10/21/20 12:16 AM, Anand Jain wrote:
>> btrfs_device::disk_total_bytes is set even for a seed device (the
>> comment is wrong).
>>
>> The function fill_device_from_item() does the job of reading it from the
>> item and updating btrfs_device::disk_total_bytes. So both the missing
>> device and the seed devices do have their disk_total_bytes updated.
>>
>> Furthermore, while removing the device if there is a power loss, we could
>> have a device with its total_bytes = 0, that's still valid.
>>
>> So this patch removes the check dev->disk_total_bytes == 0 in the
>> function verify_one_dev_extent(), which it does nothing in it.
>>
>> And take this opportunity to introduce a check if the device::total_bytes
>> is more than the max device size in read_one_dev().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> v2: add check if the total_bytes is more than the actual device size in
>>      read_one_dev().
>>      update change log.
>>
>>   fs/btrfs/volumes.c | 25 ++++++++++---------------
>>   1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 2a5397fb4175..0c6049f9ace3 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -6847,6 +6847,16 @@ static int read_one_dev(struct extent_buffer 
>> *leaf,
>>       }
>>       fill_device_from_item(leaf, dev_item, device);
>> +    if (device->bdev) {
>> +        u64 max_total_bytes = i_size_read(device->bdev->bd_inode);
>> +
>> +        if (device->total_bytes > max_total_bytes) {
>> +            btrfs_err(fs_info,
>> +            "device total_bytes should be below %llu but found %llu",
> 
> "should be less than or equal to"
> 

Hm. Do you mean to say..

-               if (device->total_bytes > max_total_bytes) {
+               if (max_total_bytes <= device->total_bytes) {

OR

-               if (device->total_bytes > max_total_bytes) {
+               if (device->total_bytes <= max_total_bytes) {

The former is correct.
As device->total_bytes is the total_bytes as read from the dev_item.
And the max_total_bytes is the actual device size.



Thanks, Anand



> Thanks,
> 
> Josef

[PATCH RESEND v2 1/2] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

btrfs_device::disk_total_bytes is set even for a seed device (the
comment is wrong).

The function fill_device_from_item() does the job of reading it from the
item and updating btrfs_device::disk_total_bytes. So both the missing
device and the seed devices do have their disk_total_bytes updated.

Furthermore, while removing the device if there is a power loss, we could
have a device with its total_bytes = 0, that's still valid.

So this patch removes the check dev->disk_total_bytes == 0 in the
function verify_one_dev_extent(), which it does nothing in it.

And take this opportunity to introduce a check if the device::total_bytes
is more than the max device size in read_one_dev().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v2: add check if the total_bytes is more than the actual device size in
    read_one_dev().
    update change log.

 fs/btrfs/volumes.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2a5397fb4175..0c6049f9ace3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6847,6 +6847,16 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	fill_device_from_item(leaf, dev_item, device);
+	if (device->bdev) {
+		u64 max_total_bytes = i_size_read(device->bdev->bd_inode);
+
+		if (device->total_bytes > max_total_bytes) {
+			btrfs_err(fs_info,
+			"device total_bytes should be below %llu but found %llu",
+				  max_total_bytes, device->total_bytes);
+			return -EINVAL;
+		}
+	}
 	set_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	   !test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
@@ -7579,21 +7589,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	/* It's possible this device is a dummy for seed device */
-	if (dev->disk_total_bytes == 0) {
-		struct btrfs_fs_devices *devs;
-
-		devs = list_first_entry(&fs_info->fs_devices->seed_list,
-					struct btrfs_fs_devices, seed_list);
-		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
-		if (!dev) {
-			btrfs_err(fs_info, "failed to find seed devid %llu",
-				  devid);
-			ret = -EUCLEAN;
-			goto out;
-		}
-	}
-
 	if (physical_offset + physical_len > dev->disk_total_bytes) {
 		btrfs_err(fs_info,
 "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
-- 
2.25.1

[PATCH 2/2] btrfs: fix btrfs_find_device unused arg seed

[Anand Jain]

commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
check if the last arg (seed) is true in the function btrfs_find_device().
This arg tells whether to traverse through the seed device list or not.

This means after the above commit the arg is always true, and the parent
function which set this arg to false aren't effective.

So we don't worry about the parent functions which set the last arg to
true, instead there is only one parent with calling btrfs_find_device
with the last arg false in device_list_add().

But in fact, even the device_list_add() has no purpose that it has to set
the last arg to false. Because the fs_devices always points to the
device's fs_devices. So with the devid+uuid matching, it shall find the
btrfs_device and returns. So naturally, it won't traverse through the
seed fs_devices (if) present.

So this patch makes it official that we don't need the last arg in the
function btrfs_find_device() and it shall always traverse through the
seed device list.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 22 ++++++++++------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 119721eeecf6..c58a99677cf9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -149,10 +149,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
-						src_devid, NULL, NULL, true);
+						src_devid, NULL, NULL);
 		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
 							BTRFS_DEV_REPLACE_DEVID,
-							NULL, NULL, true);
+							NULL, NULL);
 		/*
 		 * allow 'btrfs dev replace_cancel' if src/tgt device is
 		 * missing
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ab408a23ba32..a3550b0fa9b0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1680,7 +1680,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
-	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!device) {
 		btrfs_info(fs_info, "resizer unable to find device %llu",
 			   devid);
@@ -3323,7 +3323,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 
 	rcu_read_lock();
 	dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid,
-				NULL, true);
+				NULL);
 
 	if (!dev) {
 		ret = -ENODEV;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index cf63f1e27a27..a0b5fb331791 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3855,7 +3855,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
 		goto out_free_ctx;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
 		     !is_dev_replace)) {
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
@@ -4031,7 +4031,7 @@ int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid,
 	struct scrub_ctx *sctx = NULL;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (dev)
 		sctx = dev->scrub_ctx;
 	if (sctx)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9be40eece8ed..29995f23aa75 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -822,7 +822,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	} else {
 		mutex_lock(&fs_devices->device_list_mutex);
 		device = btrfs_find_device(fs_devices, devid,
-				disk_super->dev_item.uuid, NULL, false);
+				disk_super->dev_item.uuid, NULL);
 
 		/*
 		 * If this disk has been pulled into an fs devices created by
@@ -2296,10 +2296,10 @@ static struct btrfs_device *btrfs_find_device_by_path(
 	dev_uuid = disk_super->dev_item.uuid;
 	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->metadata_uuid, true);
+					   disk_super->metadata_uuid);
 	else
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->fsid, true);
+					   disk_super->fsid);
 
 	btrfs_release_disk_super(disk_super);
 	if (!device)
@@ -2319,7 +2319,7 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 
 	if (devid) {
 		device = btrfs_find_device(fs_info->fs_devices, devid, NULL,
-					   NULL, true);
+					   NULL);
 		if (!device)
 			return ERR_PTR(-ENOENT);
 		return device;
@@ -2468,7 +2468,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 		read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
 				   BTRFS_FSID_SIZE);
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   fs_uuid, true);
+					   fs_uuid);
 		BUG_ON(!device); /* Logic error */
 
 		if (device->fs_devices->seeding) {
@@ -6450,8 +6450,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
  * If @seed is true, traverse through the seed devices.
  */
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid,
-				       bool seed)
+				       u64 devid, u8 *uuid, u8 *fsid)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *seed_devs;
@@ -6658,7 +6657,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 				   btrfs_stripe_dev_uuid_nr(chunk, i),
 				   BTRFS_UUID_SIZE);
 		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices,
-							devid, uuid, NULL, true);
+							devid, uuid, NULL);
 		if (!map->stripes[i].dev &&
 		    !btrfs_test_opt(fs_info, DEGRADED)) {
 			free_extent_map(em);
@@ -6797,7 +6796,7 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-				   fs_uuid, true);
+				   fs_uuid);
 	if (!device) {
 		if (!btrfs_test_opt(fs_info, DEGRADED)) {
 			btrfs_report_missing_device(fs_info, devid,
@@ -7439,8 +7438,7 @@ int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
 	int i;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL,
-				true);
+	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL);
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	if (!dev) {
@@ -7571,7 +7569,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 	}
 
 	/* Make sure no dev extent is beyond device bondary */
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev) {
 		btrfs_err(fs_info, "failed to find devid %llu", devid);
 		ret = -EUCLEAN;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 48bdca01e237..ebb7df93e63f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -453,7 +453,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
 int btrfs_grow_device(struct btrfs_trans_handle *trans,
 		      struct btrfs_device *device, u64 new_size);
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid, bool seed);
+				       u64 devid, u8 *uuid, u8 *fsid);
 int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
-- 
2.25.1

Re: [PATCH 2/2] btrfs: fix btrfs_find_device unused arg seed

[Nikolay Borisov]

On 24.09.20 г. 13:11 ч., Anand Jain wrote:
> commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
> check if the last arg (seed) is true in the function btrfs_find_device().
> This arg tells whether to traverse through the seed device list or not.
> 
> This means after the above commit the arg is always true, and the parent
> function which set this arg to false aren't effective.
> 
> So we don't worry about the parent functions which set the last arg to
> true, instead there is only one parent with calling btrfs_find_device
> with the last arg false in device_list_add().
> 
> But in fact, even the device_list_add() has no purpose that it has to set
> the last arg to false. Because the fs_devices always points to the
> device's fs_devices. So with the devid+uuid matching, it shall find the
> btrfs_device and returns. So naturally, it won't traverse through the
> seed fs_devices (if) present.
> 
> So this patch makes it official that we don't need the last arg in the
> function btrfs_find_device() and it shall always traverse through the
> seed device list.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

IMO the changelog could be vastly simplified by stating that following
commit 343694eee8d8 the seed argument is no longer used and can simply
be removed.

In any case

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

Re: [PATCH 2/2] btrfs: fix btrfs_find_device unused arg seed

[Nikolay Borisov]

On 24.09.20 г. 13:21 ч., Nikolay Borisov wrote:
> 
> 
> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>> commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
>> check if the last arg (seed) is true in the function btrfs_find_device().
>> This arg tells whether to traverse through the seed device list or not.
>>
>> This means after the above commit the arg is always true, and the parent
>> function which set this arg to false aren't effective.
>>
>> So we don't worry about the parent functions which set the last arg to
>> true, instead there is only one parent with calling btrfs_find_device
>> with the last arg false in device_list_add().
>>
>> But in fact, even the device_list_add() has no purpose that it has to set
>> the last arg to false. Because the fs_devices always points to the
>> device's fs_devices. So with the devid+uuid matching, it shall find the
>> btrfs_device and returns. So naturally, it won't traverse through the
>> seed fs_devices (if) present.
>>
>> So this patch makes it official that we don't need the last arg in the
>> function btrfs_find_device() and it shall always traverse through the
>> seed device list.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> IMO the changelog could be vastly simplified by stating that following
> commit 343694eee8d8 the seed argument is no longer used and can simply
> be removed.
> 
> In any case
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> 


Actually I went back and it seems that I accidentally removed the
portion of btrfs_find_device() which was predicated on seed bool
parameter being true or false. So the correct thing to do is really to
send a patch which adds it back and consider it a fix (i.e adding a
Fixes: tag as well).


OTOH - does it really make any difference? SO what if btrfs_find_device
returns a device from fs_devices->seed_list  in device_list_add (which
is called only from device scan)?

Re: [PATCH 2/2] btrfs: fix btrfs_find_device unused arg seed

[Anand Jain]

On 25/9/20 4:22 pm, Nikolay Borisov wrote:
> 
> 
> On 24.09.20 г. 13:21 ч., Nikolay Borisov wrote:
>>
>>
>> On 24.09.20 г. 13:11 ч., Anand Jain wrote:
>>> commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
>>> check if the last arg (seed) is true in the function btrfs_find_device().
>>> This arg tells whether to traverse through the seed device list or not.
>>>
>>> This means after the above commit the arg is always true, and the parent
>>> function which set this arg to false aren't effective.
>>>
>>> So we don't worry about the parent functions which set the last arg to
>>> true, instead there is only one parent with calling btrfs_find_device
>>> with the last arg false in device_list_add().
>>>
>>> But in fact, even the device_list_add() has no purpose that it has to set
>>> the last arg to false. Because the fs_devices always points to the
>>> device's fs_devices. So with the devid+uuid matching, it shall find the
>>> btrfs_device and returns. So naturally, it won't traverse through the
>>> seed fs_devices (if) present.
>>>
>>> So this patch makes it official that we don't need the last arg in the
>>> function btrfs_find_device() and it shall always traverse through the
>>> seed device list.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> IMO the changelog could be vastly simplified by stating that following
>> commit 343694eee8d8 the seed argument is no longer used and can simply
>> be removed.
>>
>> In any case
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>>
> 
> 
> Actually I went back and it seems that I accidentally removed the
> portion of btrfs_find_device() which was predicated on seed bool
> parameter being true or false. So the correct thing to do is really to
> send a patch which adds it back and consider it a fix (i.e adding a
> Fixes: tag as well).
> 
> 
> OTOH - does it really make any difference? SO what if btrfs_find_device
> returns a device from fs_devices->seed_list  in device_list_add (which
> is called only from device scan)?
> 

There isn't bug as such. device scan is through the btrfs_control 
interface. So it depends on the device fsid to get fs_devices and it 
could never use seed_list with a genuine (non crafted) disk image.

Thanks, Anand

Re: [PATCH 2/2] btrfs: fix btrfs_find_device unused arg seed

[Josef Bacik]

On 9/24/20 6:11 AM, Anand Jain wrote:
> commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
> check if the last arg (seed) is true in the function btrfs_find_device().
> This arg tells whether to traverse through the seed device list or not.
> 
> This means after the above commit the arg is always true, and the parent
> function which set this arg to false aren't effective.
> 
> So we don't worry about the parent functions which set the last arg to
> true, instead there is only one parent with calling btrfs_find_device
> with the last arg false in device_list_add().
> 
> But in fact, even the device_list_add() has no purpose that it has to set
> the last arg to false. Because the fs_devices always points to the
> device's fs_devices. So with the devid+uuid matching, it shall find the
> btrfs_device and returns. So naturally, it won't traverse through the
> seed fs_devices (if) present.
> 
> So this patch makes it official that we don't need the last arg in the
> function btrfs_find_device() and it shall always traverse through the
> seed device list.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH RESEND 2/2] btrfs: fix btrfs_find_device unused arg seed

[Anand Jain]

commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
check if the last arg (seed) is true in the function btrfs_find_device().
This arg tells whether to traverse through the seed device list or not.

This means after the above commit the arg is always true, and the parent
function which set this arg to false aren't effective.

So we don't worry about the parent functions which set the last arg to
true, instead there is only one parent with calling btrfs_find_device
with the last arg false in device_list_add().

But in fact, even the device_list_add() has no purpose that it has to set
the last arg to false. Because the fs_devices always points to the
device's fs_devices. So with the devid+uuid matching, it shall find the
btrfs_device and returns. So naturally, it won't traverse through the
seed fs_devices (if) present.

So this patch makes it official that we don't need the last arg in the
function btrfs_find_device() and it shall always traverse through the
seed device list.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 22 ++++++++++------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 119721eeecf6..c58a99677cf9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -149,10 +149,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
-						src_devid, NULL, NULL, true);
+						src_devid, NULL, NULL);
 		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
 							BTRFS_DEV_REPLACE_DEVID,
-							NULL, NULL, true);
+							NULL, NULL);
 		/*
 		 * allow 'btrfs dev replace_cancel' if src/tgt device is
 		 * missing
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ab408a23ba32..a3550b0fa9b0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1680,7 +1680,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
-	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!device) {
 		btrfs_info(fs_info, "resizer unable to find device %llu",
 			   devid);
@@ -3323,7 +3323,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 
 	rcu_read_lock();
 	dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid,
-				NULL, true);
+				NULL);
 
 	if (!dev) {
 		ret = -ENODEV;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index cf63f1e27a27..a0b5fb331791 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3855,7 +3855,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
 		goto out_free_ctx;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
 		     !is_dev_replace)) {
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
@@ -4031,7 +4031,7 @@ int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid,
 	struct scrub_ctx *sctx = NULL;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (dev)
 		sctx = dev->scrub_ctx;
 	if (sctx)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9be40eece8ed..29995f23aa75 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -822,7 +822,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	} else {
 		mutex_lock(&fs_devices->device_list_mutex);
 		device = btrfs_find_device(fs_devices, devid,
-				disk_super->dev_item.uuid, NULL, false);
+				disk_super->dev_item.uuid, NULL);
 
 		/*
 		 * If this disk has been pulled into an fs devices created by
@@ -2296,10 +2296,10 @@ static struct btrfs_device *btrfs_find_device_by_path(
 	dev_uuid = disk_super->dev_item.uuid;
 	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->metadata_uuid, true);
+					   disk_super->metadata_uuid);
 	else
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->fsid, true);
+					   disk_super->fsid);
 
 	btrfs_release_disk_super(disk_super);
 	if (!device)
@@ -2319,7 +2319,7 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 
 	if (devid) {
 		device = btrfs_find_device(fs_info->fs_devices, devid, NULL,
-					   NULL, true);
+					   NULL);
 		if (!device)
 			return ERR_PTR(-ENOENT);
 		return device;
@@ -2468,7 +2468,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 		read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
 				   BTRFS_FSID_SIZE);
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   fs_uuid, true);
+					   fs_uuid);
 		BUG_ON(!device); /* Logic error */
 
 		if (device->fs_devices->seeding) {
@@ -6450,8 +6450,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
  * If @seed is true, traverse through the seed devices.
  */
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid,
-				       bool seed)
+				       u64 devid, u8 *uuid, u8 *fsid)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *seed_devs;
@@ -6658,7 +6657,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 				   btrfs_stripe_dev_uuid_nr(chunk, i),
 				   BTRFS_UUID_SIZE);
 		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices,
-							devid, uuid, NULL, true);
+							devid, uuid, NULL);
 		if (!map->stripes[i].dev &&
 		    !btrfs_test_opt(fs_info, DEGRADED)) {
 			free_extent_map(em);
@@ -6797,7 +6796,7 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-				   fs_uuid, true);
+				   fs_uuid);
 	if (!device) {
 		if (!btrfs_test_opt(fs_info, DEGRADED)) {
 			btrfs_report_missing_device(fs_info, devid,
@@ -7439,8 +7438,7 @@ int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
 	int i;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL,
-				true);
+	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL);
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	if (!dev) {
@@ -7571,7 +7569,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 	}
 
 	/* Make sure no dev extent is beyond device bondary */
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev) {
 		btrfs_err(fs_info, "failed to find devid %llu", devid);
 		ret = -EUCLEAN;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 48bdca01e237..ebb7df93e63f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -453,7 +453,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
 int btrfs_grow_device(struct btrfs_trans_handle *trans,
 		      struct btrfs_device *device, u64 new_size);
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid, bool seed);
+				       u64 devid, u8 *uuid, u8 *fsid);
 int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
-- 
2.25.1

[PATCH RESEND v2 2/2] btrfs: fix btrfs_find_device unused arg seed

[Anand Jain]

commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
check if the last arg (seed) is true in the function btrfs_find_device().
This arg tells whether to traverse through the seed device list or not.

This means after the above commit the arg is always true, and the parent
function which set this arg to false aren't effective.

So we don't worry about the parent functions which set the last arg to
true, instead there is only one parent with calling btrfs_find_device
with the last arg false in device_list_add().

But in fact, even the device_list_add() has no purpose that it has to set
the last arg to false. Because the fs_devices always points to the
device's fs_devices. So with the devid+uuid matching, it shall find the
btrfs_device and returns. So naturally, it won't traverse through the
seed fs_devices (if) present.

So this patch makes it official that we don't need the last arg in the
function btrfs_find_device() and it shall always traverse through the
seed device list.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
v2: -
 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 22 ++++++++++------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 119721eeecf6..c58a99677cf9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -149,10 +149,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
-						src_devid, NULL, NULL, true);
+						src_devid, NULL, NULL);
 		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
 							BTRFS_DEV_REPLACE_DEVID,
-							NULL, NULL, true);
+							NULL, NULL);
 		/*
 		 * allow 'btrfs dev replace_cancel' if src/tgt device is
 		 * missing
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ab408a23ba32..a3550b0fa9b0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1680,7 +1680,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
-	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!device) {
 		btrfs_info(fs_info, "resizer unable to find device %llu",
 			   devid);
@@ -3323,7 +3323,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 
 	rcu_read_lock();
 	dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid,
-				NULL, true);
+				NULL);
 
 	if (!dev) {
 		ret = -ENODEV;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index cf63f1e27a27..a0b5fb331791 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3855,7 +3855,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
 		goto out_free_ctx;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
 		     !is_dev_replace)) {
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
@@ -4031,7 +4031,7 @@ int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid,
 	struct scrub_ctx *sctx = NULL;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (dev)
 		sctx = dev->scrub_ctx;
 	if (sctx)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9be40eece8ed..29995f23aa75 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -822,7 +822,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	} else {
 		mutex_lock(&fs_devices->device_list_mutex);
 		device = btrfs_find_device(fs_devices, devid,
-				disk_super->dev_item.uuid, NULL, false);
+				disk_super->dev_item.uuid, NULL);
 
 		/*
 		 * If this disk has been pulled into an fs devices created by
@@ -2296,10 +2296,10 @@ static struct btrfs_device *btrfs_find_device_by_path(
 	dev_uuid = disk_super->dev_item.uuid;
 	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->metadata_uuid, true);
+					   disk_super->metadata_uuid);
 	else
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->fsid, true);
+					   disk_super->fsid);
 
 	btrfs_release_disk_super(disk_super);
 	if (!device)
@@ -2319,7 +2319,7 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 
 	if (devid) {
 		device = btrfs_find_device(fs_info->fs_devices, devid, NULL,
-					   NULL, true);
+					   NULL);
 		if (!device)
 			return ERR_PTR(-ENOENT);
 		return device;
@@ -2468,7 +2468,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 		read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
 				   BTRFS_FSID_SIZE);
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   fs_uuid, true);
+					   fs_uuid);
 		BUG_ON(!device); /* Logic error */
 
 		if (device->fs_devices->seeding) {
@@ -6450,8 +6450,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
  * If @seed is true, traverse through the seed devices.
  */
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid,
-				       bool seed)
+				       u64 devid, u8 *uuid, u8 *fsid)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *seed_devs;
@@ -6658,7 +6657,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 				   btrfs_stripe_dev_uuid_nr(chunk, i),
 				   BTRFS_UUID_SIZE);
 		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices,
-							devid, uuid, NULL, true);
+							devid, uuid, NULL);
 		if (!map->stripes[i].dev &&
 		    !btrfs_test_opt(fs_info, DEGRADED)) {
 			free_extent_map(em);
@@ -6797,7 +6796,7 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-				   fs_uuid, true);
+				   fs_uuid);
 	if (!device) {
 		if (!btrfs_test_opt(fs_info, DEGRADED)) {
 			btrfs_report_missing_device(fs_info, devid,
@@ -7439,8 +7438,7 @@ int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
 	int i;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL,
-				true);
+	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL);
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	if (!dev) {
@@ -7571,7 +7569,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 	}
 
 	/* Make sure no dev extent is beyond device bondary */
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev) {
 		btrfs_err(fs_info, "failed to find devid %llu", devid);
 		ret = -EUCLEAN;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 48bdca01e237..ebb7df93e63f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -453,7 +453,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
 int btrfs_grow_device(struct btrfs_trans_handle *trans,
 		      struct btrfs_device *device, u64 new_size);
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid, bool seed);
+				       u64 devid, u8 *uuid, u8 *fsid);
 int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
-- 
2.25.1

Re: [PATCH 0/2] fix verify_one_dev_extent and btrfs_find_device

[Anand Jain]

David,
   ping?

Thanks, Anand


On 24/9/20 6:11 pm, Anand Jain wrote:
> btrfs_find_device()'s last arg %seed is unused, which the commit
> 343694eee8d8 (btrfs: switch seed device to list api) ignored purposely
> or missed.
> 
> But there isn't any regression due to that. And this series makes
> it official that btrfs_find_device() doesn't need the last arg.
> 
> To achieve that patch 1/2 critically reviews the need for the check
> disk_total_bytes == 0 in the function verify_one_dev_extent() and finds
> that, the condition is never met and so deletes the same. Which also
> drops one of the parents of btrfs_find_device() with the last arg false.
> 
> So only device_list_add() is using btrfs_find_device() with the last arg as
> false, which the patch 2/2 finds is not required as well. So
> this patch drops the last arg in btrfs_find_device() altogether.
> 
> Anand Jain (2):
>    btrfs: drop never met condition of disk_total_bytes == 0
>    btrfs: fix btrfs_find_device unused arg seed
> 
>   fs/btrfs/dev-replace.c |  4 ++--
>   fs/btrfs/ioctl.c       |  4 ++--
>   fs/btrfs/scrub.c       |  4 ++--
>   fs/btrfs/volumes.c     | 37 ++++++++++---------------------------
>   fs/btrfs/volumes.h     |  2 +-
>   5 files changed, 17 insertions(+), 34 deletions(-)
> 

[PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device

[Anand Jain]

btrfs_find_device()'s last arg %seed is unused, which the commit
343694eee8d8 (btrfs: switch seed device to list api) ignored purposely
or missed.

But there isn't any regression due to that. And this series makes
it official that btrfs_find_device() doesn't need the last arg.

To achieve that patch 1/2 critically reviews the need for the check
disk_total_bytes == 0 in the function verify_one_dev_extent() and finds
that, the condition is never met and so deletes the same. Which also
drops one of the parents of btrfs_find_device() with the last arg false.

So only device_list_add() is using btrfs_find_device() with the last arg as
false, which the patch 2/2 finds is not required as well. So
this patch drops the last arg in btrfs_find_device() altogether.

Anand Jain (2):
  btrfs: drop never met condition of disk_total_bytes == 0
  btrfs: fix btrfs_find_device unused arg seed

 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 37 ++++++++++---------------------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 34 deletions(-)

-- 
2.25.1

Re: [PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device

[David Sterba]

On Wed, Oct 21, 2020 at 12:16:07PM +0800, Anand Jain wrote:
> btrfs_find_device()'s last arg %seed is unused, which the commit
> 343694eee8d8 (btrfs: switch seed device to list api) ignored purposely
> or missed.
> 
> But there isn't any regression due to that. And this series makes
> it official that btrfs_find_device() doesn't need the last arg.
> 
> To achieve that patch 1/2 critically reviews the need for the check
> disk_total_bytes == 0 in the function verify_one_dev_extent() and finds
> that, the condition is never met and so deletes the same. Which also
> drops one of the parents of btrfs_find_device() with the last arg false.
> 
> So only device_list_add() is using btrfs_find_device() with the last arg as
> false, which the patch 2/2 finds is not required as well. So
> this patch drops the last arg in btrfs_find_device() altogether.
> 
> Anand Jain (2):
>   btrfs: drop never met condition of disk_total_bytes == 0
>   btrfs: fix btrfs_find_device unused arg seed

I missed this update because you replied to the original mail and this
threads under the replies and is so easy to miss that it will inevitably
lead to delayed reviews. This is not supposed to be hard, if you have
another iteration, send it as a new mail thread where the 0/N mail has
other patches as replies. You can also use the workflow scripts to
create or update the issue so we'll notice that. Right now there's stale
https://github.com/btrfs/linux/issues/65 that is labeled with comments
so nobody will probably lookd at it again until the next iteration
arrives.

Re: [PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device

[Anand Jain]

On 30/10/20 5:02 am, David Sterba wrote:
> On Wed, Oct 21, 2020 at 12:16:07PM +0800, Anand Jain wrote:
>> btrfs_find_device()'s last arg %seed is unused, which the commit
>> 343694eee8d8 (btrfs: switch seed device to list api) ignored purposely
>> or missed.
>>
>> But there isn't any regression due to that. And this series makes
>> it official that btrfs_find_device() doesn't need the last arg.
>>
>> To achieve that patch 1/2 critically reviews the need for the check
>> disk_total_bytes == 0 in the function verify_one_dev_extent() and finds
>> that, the condition is never met and so deletes the same. Which also
>> drops one of the parents of btrfs_find_device() with the last arg false.
>>
>> So only device_list_add() is using btrfs_find_device() with the last arg as
>> false, which the patch 2/2 finds is not required as well. So
>> this patch drops the last arg in btrfs_find_device() altogether.
>>
>> Anand Jain (2):
>>    btrfs: drop never met condition of disk_total_bytes == 0
>>    btrfs: fix btrfs_find_device unused arg seed
> 
> I missed this update because you replied to the original mail and this
> threads under the replies and is so easy to miss that it will inevitably
> lead to delayed reviews. This is not supposed to be hard, if you have
> another iteration, send it as a new mail thread where the 0/N mail has
> other patches as replies. 

ok. For now I am using this option, as..

> You can also use the workflow scripts to
> create or update the issue so we'll notice that. 

I found workflow script can't handle the update? Instead it creates a 
new issue?

> Right now there's stale
> https://github.com/btrfs/linux/issues/65 that is labeled with comments
> so nobody will probably lookd at it again until the next iteration
> arrives.

Should I resend the latest update?

Thanks Anand

Re: [PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device

[David Sterba]

On Fri, Oct 30, 2020 at 05:14:06AM +0800, Anand Jain wrote:
> > You can also use the workflow scripts to
> > create or update the issue so we'll notice that. 
> 
> I found workflow script can't handle the update? Instead it creates a 
> new issue?

The script btrfs-create-issue asks if there's an existing issue to
update.

[PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device

[Anand Jain]

(Resend: I am marking all the patches as v2, not just 1/2).

v2: as in 1/2, 2/2 no change.

btrfs_find_device()'s last arg %seed is unused, which the commit
343694eee8d8 (btrfs: switch seed device to list api) ignored purposely
or missed.

But there isn't any regression due to that. And this series makes
it official that btrfs_find_device() doesn't need the last arg.

To achieve that patch 1/2 critically reviews the need for the check
disk_total_bytes == 0 in the function verify_one_dev_extent() and finds
that, the condition is never met and so deletes the same. Which also
drops one of the parents of btrfs_find_device() with the last arg false.

So only device_list_add() is using btrfs_find_device() with the last arg as
false, which the patch 2/2 finds is not required as well. So
this patch drops the last arg in btrfs_find_device() altogether.

Anand Jain (2):
  btrfs: drop never met condition of disk_total_bytes == 0
  btrfs: fix btrfs_find_device unused arg seed

 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 37 ++++++++++---------------------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 34 deletions(-)

-- 
2.25.1