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

[Nikolay Borisov <nborisov@suse.com>]

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

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

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