| Message ID | 5f8aa8a03a1712adba0023fc1efa18623571c588.1599091832.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups | expand |
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) >
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) >>
>> - 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. Patch 6 uses ret in seed list loop. So its ok. Thanks, Anand
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; - 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; + ASSERT(devices_kobj); + ASSERT(devinfo_kobj); - if (one_device && one_device != dev) - continue; + nofs_flag = memalloc_nofs_save(); - if (dev->bdev) { - struct hd_struct *disk; - struct kobject *disk_kobj; + if (device->bdev) { + struct hd_struct *disk; + struct kobject *disk_kobj; - disk = dev->bdev->bd_part; - disk_kobj = &part_to_dev(disk)->kobj; + disk = device->bdev->bd_part; + disk_kobj = &part_to_dev(disk)->kobj; - error = sysfs_create_link(fs_devices->devices_kobj, - disk_kobj, disk_kobj->name); - if (error) - break; + ret = sysfs_create_link(devices_kobj, disk_kobj, + disk_kobj->name); + if (ret) { + btrfs_warn(device->fs_info, + "sysfs create device link failed %d devid %llu", + ret, device->devid); + goto out; } + } - init_completion(&dev->kobj_unregister); - error = kobject_init_and_add(&dev->devid_kobj, &devid_ktype, - fs_devices->devinfo_kobj, "%llu", - dev->devid); - if (error) { - kobject_put(&dev->devid_kobj); - break; - } + init_completion(&device->kobj_unregister); + ret = kobject_init_and_add(&device->devid_kobj, &devid_ktype, + devinfo_kobj, "%llu", device->devid); + if (ret) { + kobject_put(&device->devid_kobj); + btrfs_warn(device->fs_info, + "sysfs devinfo init failed %d devid %llu", + ret, device->devid); } + +out: memalloc_nofs_restore(nofs_flag); + return ret; +} - return error; +int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices, + struct btrfs_device *one_device) +{ + int ret; + + 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)
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(-)