Re: [PATCH 4/4] btrfs: sysfs, add devid/dev_state kobject and attribute

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

On 5/12/19 11:14 PM, David Sterba wrote:
> On Thu, Dec 05, 2019 at 10:38:15PM +0800, Anand Jain wrote:
>>> So the values copy the device state macros, that's probably ok.
>>    Yep.
> 
> Although, sysfs files should print one value per file, which makes sense
> in many cases, so eg. missing should exist separately too for quick
> checks for the most common device states. The dev_state reflects the
> internal state and is likely useful only for debugging.
> 

  I agree. Its better to create an individual attribute for each of the
  device states. For instance..

    under the 'UUID/devinfo/<devid>' kobject
    attributes will be:
      missing
      in_fs_metadata
      replace_target

   cat missing
    0
   cat in_fs_metadata
    1

   ..etc

  which seems to be more or less standard for block devices.

  Will fix it in v2.

>>>> +static ssize_t btrfs_sysfs_dev_state_show(struct kobject *kobj,
>>>> +					  struct kobj_attribute *a, char *buf)
>>>> +{
>>>> +	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
>>>> +						   devid_kobj);
>>>> +
>>>> +	btrfs_dev_state_to_str(device, buf, PAGE_SIZE);
>>>
>>> The device access is unprotected, you need at least RCU but that still
>>> does not prevent from the device being freed by deletion.
>>
>>    We need RCU let me fix. Device being deleted is fine, there
>>    is nothing to lose, another directory lookup will show that
>>    UUID/devinfo/<devid> is gone is the device is deleted.
> 
> The device can be gone from the list but the sysfs files can still
> exist.
> 
>      CPU1                                   CPU2
> 
> btrfs_rm_device
>                                          open file
>    btrfs_sysfs_rm_device_link
>      btrfs_free_device
>        kfree(device)
>                                          call read, access freed device
> 

   I completely missed the sysfs synchronization with device delete.
   As in the other email discussion, I think a new rwlock shall suffice.
   And as its lock is only between device delete and sysfs so it shall
   be light weight without affecting the other device_list_mutex holders.

Thanks,
Anand

>>> The
>>> device_list_mutex is quite heavy and allowing a DoS by repeatedly
>>> reading the file contents is not something we want to allow.
>>>
>>
>>     Yes we don't have to use device_list_mutex here, as its read,
>>     a refresh/re-read will refresh the dev_state.
> 
> The point is not to synchronize the device state values but the device
> object itself.