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

[Anand Jain <anandsuveer@gmail.com>]

On 12/6/19 9:49 PM, Anand Jain wrote:
> 
> 
> 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.

This is fixed 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.

  Looked into this further, actually we don't need any lock here
  the device delete thread which calls kobject_put() makes sure
  sysfs read is closed. So an existing sysfs read thread will have
  to complete before device free.


       CPU1                                   CPU2

  btrfs_rm_device
                                           open file
     btrfs_sysfs_rm_device_link
                                           call read, access freed device
       sysfs waits for the open file
       to close.
       btrfs_free_device
         kfree(device)


Thanks
Anand


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