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

Anand Jain
2019-12-09T14:05:50Z

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