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.
