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

[David Sterba <dsterba@suse.cz>]

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.

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

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