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

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

On 6/11/20 6:36 am, David Sterba wrote:
> On Tue, Nov 03, 2020 at 01:49:42PM +0800, Anand Jain wrote:
>> btrfs_device::disk_total_bytes is set even for a seed device (the
>> comment is wrong).
> 
> That's where I'm a bit lost. It was added in 1b3922a8bc74 ("btrfs: Use
> real device structure to verify dev extent").
> 

  The call chain where we update the btrfs_device::disk_total_bytes is as
  below..


    read_one_dev()
::

 From the section below we get the cloned copy of the seed device.
-----
         if (memcmp(fs_uuid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE)) {
                 fs_devices = open_seed_devices(fs_info, fs_uuid);
                 if (IS_ERR(fs_devices))
                         return PTR_ERR(fs_devices);
         }

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


  Further down in read_one_dev() at and in the 
fill_device_from_item(..., device) we update the 
btrfs_device::disk_total_bytes.

fill_device_from_item(..., device)
::
         device->disk_total_bytes = btrfs_device_total_bytes(leaf, 
dev_item);

Hope this clarifies.

V1 email discussion has more information.

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


> 
> Ok, that's the condition that the commit mentioned above used to detect
> the device and to avoid doing the tree-checker verification.

Ok. Please look at what did the commit 1b3922a8bc74 do? It re-ran the 
btrfs_find_device(seed_devs,..., false), which anyway the 
btrfs_find_device(sprout_devs,..., true) has run just before. In both of 
these btrfs_find_device() runs, the dev returned will be the same. The 
fix makes no sense to the problem as in the commit.


> 
>> So this patch removes the check dev->disk_total_bytes == 0 in the
>> function verify_one_dev_extent(), which it does nothing in it.
> 
> Removing a check that supposedly does notghing, but the referenced
> commit says otherwise.
> 

IMO the reason for the problem found in that commit was wrong. Qu 
commit's email thread has some discussion. But nothing more on the problem.

Also Nikolay had the same question here was my reply.
https://www.spinics.net/lists/linux-btrfs/msg105645.html


>> And take this opportunity to introduce a check if the device::total_bytes
>> is more than the max device size in read_one_dev().
> 
> If this is not related to the the check removal, then it should be an
> independent patch explaing in full what is being fixed. As I read it
> this should be independent as it's checking the upper bound.
> 

  That came from the Josef comments, please refer to the v1 email 
discussion. Most of the above concerns are already discussed there. I am 
ok to move it to a new patch if required.

Thanks, Anand