Re: [PATCH 0/4] btrfs, sysfs cleanup and add dev_state

[Anand Jain <anandsuveer@gmail.com>]

On 12/6/19 9:46 PM, Anand Jain wrote:
> 
> 
> On 5/12/19 11:00 PM, David Sterba wrote:
>> On Thu, Dec 05, 2019 at 07:27:02PM +0800, Anand Jain wrote:
>>> Anand Jain (4):
>>>    btrfs: sysfs, use btrfs_sysfs_remove_fsid in fail return in add_fsid
>>>    btrfs: sysfs, add UUID/devinfo kobject
>>>    btrfs: sysfs, rename device_link add,remove functions
>>>    btrfs: sysfs, add devid/dev_state kobject and attribute
>>
>> So we can start adding things to devinfo, I did a quick test how the
>> sysfs directory looks like, the base structure seems ok.
>>
>> Unlike other sources for sysfs file data (like superblock), the devices
>> can appear and disappear during the lifetime of the filesystem and as
>> pointed out in the patches, some synchronization is needed.
>>
>> But it could be more tricky. Reading from the sysfs files should not
>> block normal operation (no device_list_mutex) but also must not lead to
>> use-after-free in case the device gets deleted.
>>
>> I haven't found a simple locking scheme that would avoid accessing a
>> freed device structure, the sysfs callback can happen at any time and
>> the structure can be freed already.
>>
>> So this means that btrfs_sysfs_dev_state_show cannot access it directly
>> (using offsetof(kobj, ...)). The safe (but not necessarily the best) way
>> I have so far is to track the device kobjects in the superblock and add
>> own lock for accessing this structure.
>>
>> This avoids increasing contention of device_list_mutex, each sysfs
>> callback needs to take the lock first, lookup the device and print the
>> value if it's found. Otherwise we know the device is gone.
> 
> 
> 
>> The lock is rwlock_t, sysfs callbacks take read-side, device deletion
>> takes write possibly outside of the device_list_mutex before the device
>> is actually going to be deleted. This relies on fairness of the lock so
>> the write will happen eventually (even if there are many readers).
>>
> 
>   Yeah this makes sense to me. I completely forgot about the %device
>   getting deleted while sysfs is reading. Let me fix in the patch 4/4.
> 

  Ah. No we don't need the lock. Sysfs kobject delete/put called by the
  delete thread waits for the sysfs read open to close. And after this
  operation we are freeing the %device. So its synchronized there is no
  race.

Thanks, Anand