Re: [PATCH 01/15] btrfs: add btrfs_sysfs_add_device helper

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

On 3/9/20 4:40 pm, Nikolay Borisov wrote:
> 
> 
> On 3.09.20 г. 3:57 ч., Anand Jain wrote:
>> btrfs_sysfs_add_devices_dir() adds device link and devid kobject
>> (sysfs entries) for a device or all the devices in the btrfs_fs_devices.
>> In preparation to add these sysfs entries for the seed as well, add
>> a btrfs_sysfs_add_device() helper function and avoid code duplication.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/sysfs.c | 79 ++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 53 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 190e59152be5..3381a91d7deb 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -1271,44 +1271,71 @@ static struct kobj_type devid_ktype = {
>>   	.release	= btrfs_release_devid_kobj,
>>   };
>>   
>> -int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
>> -				struct btrfs_device *one_device)
>> +static int btrfs_sysfs_add_device(struct btrfs_device *device)
>>   {
>> -	int error = 0;
>> -	struct btrfs_device *dev;
>> +	int ret;
>>   	unsigned int nofs_flag;
>> +	struct kobject *devices_kobj;
>> +        struct kobject *devinfo_kobj;
> 
> Whitespace damage

  oops.


> 
>>   
>> -	nofs_flag = memalloc_nofs_save();
>> -	list_for_each_entry(dev, &fs_devices->devices, dev_list) {
>> +	/*
>> +	 * make sure we use the fs_info::fs_devices to fetch the kobjects
>> +	 * even for the seed fs_devices
>> +	 */
>> +	devices_kobj = device->fs_devices->fs_info->fs_devices->devices_kobj;
>> +	devinfo_kobj = device->fs_devices->fs_info->fs_devices->devinfo_kobj;
> 
> This function and its callers are called after the fs_info of devices is
> initialized so can't you simply do 'device->fs_info->fs_devices->'...
> reduces a level of pointer chasing.
> 

  Oh. Right. Will fix.

>> +	ASSERT(devices_kobj);
>> +	ASSERT(devinfo_kobj);
> <snip>
> 
> 
>>   
>> -	return error;
>> +int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
>> +				struct btrfs_device *one_device)
>> +{
>> +	int ret;
> 
> That variable can be defined inside the list_for_each-entry as it's
> being used only in that context.

ok.

Thanks!
Anand

>> +
>> +	if (one_device)
>> +		return btrfs_sysfs_add_device(one_device);
>> +
>> +	list_for_each_entry(one_device, &fs_devices->devices, dev_list) {
>> +		ret = btrfs_sysfs_add_device(one_device);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   void btrfs_kobject_uevent(struct block_device *bdev, enum kobject_action action)
>>