Re: [PATCH v10 04/41] btrfs: get zone information of zoned block devices

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

Also, %device->fs_info is not protected. It is better to avoid using
fs_info when we are still at open_fs_devices(). Yeah, the unknown part
can be better. We need to fix it as a whole. For now, you can use
something like...

-------------------------
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 1223d5b0e411..e857bb304d28 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -130,19 +130,11 @@ int btrfs_get_dev_zone_info(struct btrfs_device 
*device)
          * (device <unknown>) ..."
          */

-       rcu_read_lock();
-       if (device->fs_info)
-               btrfs_info(device->fs_info,
-                       "host-%s zoned block device %s, %u zones of %llu 
bytes",
-                       bdev_zoned_model(bdev) == BLK_ZONED_HM ? 
"managed" : "aware",
-                       rcu_str_deref(device->name), zone_info->nr_zones,
-                       zone_info->zone_size);
-       else
-               pr_info("BTRFS info: host-%s zoned block device %s, %u 
zones of %llu bytes",
-                       bdev_zoned_model(bdev) == BLK_ZONED_HM ? 
"managed" : "aware",
-                       rcu_str_deref(device->name), zone_info->nr_zones,
-                       zone_info->zone_size);
-       rcu_read_unlock();
+       btrfs_info_in_rcu(NULL,
+               "host-%s zoned block device %s, %u zones of %llu bytes",
+               bdev_zoned_model(bdev) == BLK_ZONED_HM ? "managed" : 
"aware",
+               rcu_str_deref(device->name), zone_info->nr_zones,
+               zone_info->zone_size);

         return 0;
  ---------------------------

Thanks, Anand


On 12/11/20 8:57 pm, Naohiro Aota wrote:
> On Thu, Nov 12, 2020 at 02:57:42PM +0800, Anand Jain wrote:
>>
>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 8840a4fa81eb..ed55014fd1bd 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -2462,6 +2462,11 @@ static void __init btrfs_print_mod_info(void)
>>>  #endif
>>>  #ifdef CONFIG_BTRFS_FS_REF_VERIFY
>>>              ", ref-verify=on"
>>> +#endif
>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>> +            ", zoned=yes"
>>> +#else
>>> +            ", zoned=no"
>>>  #endif
>>
>> IMO, we don't need this, as most of the generic kernel will be compiled
>> with the CONFIG_BLK_DEV_ZONED defined.
>> For review purpose we may want to know if the mounted device
>> is a zoned device. So log of zone device and its type may be useful
>> when we have verified the zoned devices in the open_ctree().
>>
>>> @@ -374,6 +375,7 @@ void btrfs_free_device(struct btrfs_device *device)
>>>      rcu_string_free(device->name);
>>>      extent_io_tree_release(&device->alloc_state);
>>>      bio_put(device->flush_bio);
>>
>>> +    btrfs_destroy_dev_zone_info(device);
>>
>> Free of btrfs_device::zone_info is already happening in the path..
>>
>> btrfs_close_one_device()
>>   btrfs_destroy_dev_zone_info()
>>
>> We don't need this..
>>
>> btrfs_free_device()
>>  btrfs_destroy_dev_zone_info()
> 
> Ah, yes, I once had it only in btrfs_free_device() and noticed that it does
> not free the device zone info on umount. So, I added one in
> btrfs_close_one_device() and forgot to remove the other one. I'll drop it
> from btrfs_free_device().
> 
>>
>>
>>> @@ -2543,6 +2551,14 @@ int btrfs_init_new_device(struct btrfs_fs_info 
>>> *fs_info, const char *device_path
>>>      }
>>>      rcu_assign_pointer(device->name, name);
>>> +    device->fs_info = fs_info;
>>> +    device->bdev = bdev;
>>> +
>>> +    /* Get zone type information of zoned block devices */
>>> +    ret = btrfs_get_dev_zone_info(device);
>>> +    if (ret)
>>> +        goto error_free_device;
>>> +
>>>      trans = btrfs_start_transaction(root, 0);
>>>      if (IS_ERR(trans)) {
>>>          ret = PTR_ERR(trans);
>>
>> It should be something like goto error_free_zone from here.
>>
>>
>>> @@ -2707,6 +2721,7 @@ int btrfs_init_new_device(struct btrfs_fs_info 
>>> *fs_info, const char *device_path
>>>          sb->s_flags |= SB_RDONLY;
>>>      if (trans)
>>>          btrfs_end_transaction(trans);
>>
>>
>> error_free_zone:
> 
> And, I'll do something like this.
> 
>>> +    btrfs_destroy_dev_zone_info(device);
>>>  error_free_device:
>>>      btrfs_free_device(device);
>>>  error:
>>
>> As mentioned we don't need btrfs_destroy_dev_zone_info()
>> again in  btrfs_free_device(). Otherwise we end up calling
>> btrfs_destroy_dev_zone_info twice here.
>>
>>
>> Thanks, Anand