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

Anand Jain
2019-12-06T13:49:19Z

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.