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

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

On 12/5/19 10:21 PM, David Sterba wrote:
> On Thu, Dec 05, 2019 at 07:27:06PM +0800, Anand Jain wrote:
>> A new sysfs RW attribute
> 
> Why RW? Most attributes/files in sysfs are supposed to be read-only, but
> you actually define the attribute as read-only with the macro
> BTRFS_ATTR.
> 

  Oh. Change log has to be fixed here.

  But I am also working on a new patch to make this RW, to set the
  new read_preferred state on the device.

>>    UUID/devinfo/<devid>/dev_state
>> is added here. The dev_state here reflects the state of the device from
>> the kernel parameter btrfs_device::dev_state and this attribute is born
>> after the device is mounted and goes along with the dynamic nature of the
>> device add and delete. This attribute gets deleted at unmount.
>>
>> For example:
>> pwd
>>   /sys/fs/btrfs/6e1961f1-5918-4ecc-a22f-948897b409f7/devinfo
>> cat 1/dev_state
>>   IN_FS_METADATA MISSING
>> cat 2/dev_state
>>   WRITABLE IN_FS_METADATA
> 
> So the values copy the device state macros, that's probably ok.
> 
  Yep.

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

Thanks, Anand